GNU bug report logs -
#34572
Add Drawpile
Previous Next
Reported by: <pkill9 <at> runbox.com>
Date: Tue, 19 Feb 2019 12:01:01 UTC
Severity: normal
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 34572 in the body.
You can then email your comments to 34572 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
guix-patches <at> gnu.org
:
bug#34572
; Package
guix-patches
.
(Tue, 19 Feb 2019 12:01:01 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
<pkill9 <at> runbox.com>
:
New bug report received and forwarded. Copy sent to
guix-patches <at> gnu.org
.
(Tue, 19 Feb 2019 12:01:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Drawpile is a collaborative drawing program - https://drawpile.net
[0001-gnu-Add-drawpile.patch (text/x-patch, attachment)]
Information forwarded
to
guix-patches <at> gnu.org
:
bug#34572
; Package
guix-patches
.
(Wed, 20 Feb 2019 21:48:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 34572 <at> debbugs.gnu.org (full text, mbox):
Hi pkill9,
> * gnu/packages/graphics.scm (drawpile): New variable.
Thank you for your patch.
[…]
> +(define-public drawpile
> + (package
> + (name "drawpile")
> + (version "2.0.11")
> + (source
> + (origin
> + (method url-fetch)
> + (uri (string-append "https://drawpile.net/files/src/drawpile-" version ".tar.gz"))
Please break this line.
> + (sha256
> + (base32
> + "0h018rxhc0lwpqwmlihalz634nd0xaafk4p2b782djjd87irnjpk"))))
> + (build-system cmake-build-system)
> + (native-inputs
> + `(("pkg-config" ,pkg-config)
> + ("qttools" ,qttools)))
> + (inputs
> + `(("qtbase" ,qtbase)
> + ("qtsvg" ,qtsvg)
> + ("qtmultimedia" ,qtmultimedia)
> + ("qtcolorwidgets" ,qtcolorwidgets)
> + ("karchive" ,karchive)
> + ("giflib" ,giflib) ; optional
> + ("kdnssd" ,kdnssd) ; optional
> + ("miniupnpc" ,(@ (gnu packages upnp) miniupnpc)) ; optional
> + ("libmicrohttpd" ,(@ (gnu packages gnunet) libmicrohttpd)) ; optional
> + ("libsodium" ,(@ (gnu packages crypto) libsodium)))) ; optional
Please don’t use these inline module references.
> + (arguments
> + `(#:configure-flags (list "-DTESTS=on" "-DCMAKE_BUILD_TYPE=Release" "-DTOOLS=on"
> + (string-append "-DLIBQTCOLORWIDGETS_LIBRARY="
> + (assoc-ref %build-inputs "qtcolorwidgets")
> + "/lib/libQtColorWidgets-Qt52.so"))))
Please put “(list” on a new line and then break after every item, so
that you can add comments as to why these flags are required.
I don’t think you need "-DCMAKE_BUILD_TYPE=Release" as we’re building
with “RelWithDebInfo” by default.
> + (home-page "https://drawpile.net")
> + (synopsis "Collaborative drawing")
“Collaborative drawing program” would be better.
> + (license license:gpl3)))
This looks like gpl3+. See for example:
https://github.com/drawpile/Drawpile/blob/master/src/server/initsys_systemd.cpp#L9
Please check the licenses more carefully.
Could you please send an updated patch?
--
Ricardo
Information forwarded
to
guix-patches <at> gnu.org
:
bug#34572
; Package
guix-patches
.
(Thu, 21 Feb 2019 12:52:01 GMT)
Full text and
rfc822 format available.
Message #11 received at 34572 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi Rekado, thanks for looking at this patch. I've updated it.
> Please don’t use these inline module references.
Sorry, I forgot to remove them (I'm moving my package definitions to Guix upstream which have the modules inline).
> Please check the licenses more carefully.
Will do.
On Wed, 20 Feb 2019 22:01:30 +0100, Ricardo Wurmus <rekado <at> elephly.net> wrote:
>
> Hi pkill9,
>
> > * gnu/packages/graphics.scm (drawpile): New variable.
>
> Thank you for your patch.
>
> […]
> > +(define-public drawpile
> > + (package
> > + (name "drawpile")
> > + (version "2.0.11")
> > + (source
> > + (origin
> > + (method url-fetch)
> > + (uri (string-append "https://drawpile.net/files/src/drawpile-" version ".tar.gz"))
>
> Please break this line.
>
> > + (sha256
> > + (base32
> > + "0h018rxhc0lwpqwmlihalz634nd0xaafk4p2b782djjd87irnjpk"))))
> > + (build-system cmake-build-system)
> > + (native-inputs
> > + `(("pkg-config" ,pkg-config)
> > + ("qttools" ,qttools)))
> > + (inputs
> > + `(("qtbase" ,qtbase)
> > + ("qtsvg" ,qtsvg)
> > + ("qtmultimedia" ,qtmultimedia)
> > + ("qtcolorwidgets" ,qtcolorwidgets)
> > + ("karchive" ,karchive)
> > + ("giflib" ,giflib) ; optional
> > + ("kdnssd" ,kdnssd) ; optional
> > + ("miniupnpc" ,(@ (gnu packages upnp) miniupnpc)) ; optional
> > + ("libmicrohttpd" ,(@ (gnu packages gnunet) libmicrohttpd)) ; optional
> > + ("libsodium" ,(@ (gnu packages crypto) libsodium)))) ; optional
>
> Please don’t use these inline module references.
>
> > + (arguments
> > + `(#:configure-flags (list "-DTESTS=on" "-DCMAKE_BUILD_TYPE=Release" "-DTOOLS=on"
> > + (string-append "-DLIBQTCOLORWIDGETS_LIBRARY="
> > + (assoc-ref %build-inputs "qtcolorwidgets")
> > + "/lib/libQtColorWidgets-Qt52.so"))))
>
> Please put “(list” on a new line and then break after every item, so
> that you can add comments as to why these flags are required.
>
> I don’t think you need "-DCMAKE_BUILD_TYPE=Release" as we’re building
> with “RelWithDebInfo” by default.
>
> > + (home-page "https://drawpile.net")
> > + (synopsis "Collaborative drawing")
>
> “Collaborative drawing program” would be better.
>
> > + (license license:gpl3)))
>
> This looks like gpl3+. See for example:
>
> https://github.com/drawpile/Drawpile/blob/master/src/server/initsys_systemd.cpp#L9
>
> Please check the licenses more carefully.
>
> Could you please send an updated patch?
>
> --
> Ricardo
[0001-gnu-Add-drawpile.patch (text/x-patch, attachment)]
Information forwarded
to
guix-patches <at> gnu.org
:
bug#34572
; Package
guix-patches
.
(Thu, 21 Feb 2019 13:59:01 GMT)
Full text and
rfc822 format available.
Message #14 received at 34572 <at> debbugs.gnu.org (full text, mbox):
Pkill -9,
Thanks for this patch, and your many others.
pkill9 wrote:
> + #:use-module (gnu packages crypto) ; libsodium
> + #:use-module (gnu packages gnunet) ; libmicrohttpd
In my experience, what little value such comments add is quickly
lost. Anyone adding new inputs will not update (or even notice)
them.
> + ("giflib" ,giflib) ; optional
> + ("kdnssd" ,kdnssd) ; optional
> + ("miniupnpc" ,miniupnpc) ; optional
> + ("libmicrohttpd" ,libmicrohttpd) ; optional
> + ("libsodium" ,libsodium))) ; optional
Same here: nothing wrong with these, I guess, but *many* package
dependencies are optionally detected at build time and this isn't
usually pointed out unless there's something more interesting
going on.
> + (arguments
> + `(#:configure-flags
> + (list "-DTESTS=on" ; build unit tests.
General remark: no full stop after inline comments.
;; Foo bar.
(foo bar) ; foo bar
> + "-DTOOLS=on" ; build dprec2txt command line tool.
> + (string-append "-DLIBQTCOLORWIDGETS_LIBRARY="
> + (assoc-ref %build-inputs
> "qtcolorwidgets")
> +
> "/lib/libQtColorWidgets-Qt52.so"))))
What about using FIND-FILES "\*.so$" here instead of hard-coding
"52"? Overkill?
Kind regards,
T G-R
Information forwarded
to
guix-patches <at> gnu.org
:
bug#34572
; Package
guix-patches
.
(Fri, 22 Feb 2019 07:29:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 34572 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Thanks Tobias, I removed those inline comments and removed the full stops after the other inline comments.
Haven't changed the qtcolorwidgets library config flag for now, I think drawpile upstream needs to improve the code that looks for qtcolorwidgets (https://github.com/drawpile/Drawpile/blob/master/config/FindQtColorWidgets.cmake) but I don't know how. Hardcoding the path isn't ideal but I'll leave it for now.
On Thu, 21 Feb 2019 14:58:33 +0100, Tobias Geerinckx-Rice <me <at> tobias.gr> wrote:
> Pkill -9,
>
> Thanks for this patch, and your many others.
>
> pkill9 wrote:
> > + #:use-module (gnu packages crypto) ; libsodium
> > + #:use-module (gnu packages gnunet) ; libmicrohttpd
>
> In my experience, what little value such comments add is quickly
> lost. Anyone adding new inputs will not update (or even notice)
> them.
>
> > + ("giflib" ,giflib) ; optional
> > + ("kdnssd" ,kdnssd) ; optional
> > + ("miniupnpc" ,miniupnpc) ; optional
> > + ("libmicrohttpd" ,libmicrohttpd) ; optional
> > + ("libsodium" ,libsodium))) ; optional
>
> Same here: nothing wrong with these, I guess, but *many* package
> dependencies are optionally detected at build time and this isn't
> usually pointed out unless there's something more interesting
> going on.
>
> > + (arguments
> > + `(#:configure-flags
> > + (list "-DTESTS=on" ; build unit tests.
>
> General remark: no full stop after inline comments.
>
> ;; Foo bar.
> (foo bar) ; foo bar
>
> > + "-DTOOLS=on" ; build dprec2txt command line tool.
> > + (string-append "-DLIBQTCOLORWIDGETS_LIBRARY="
> > + (assoc-ref %build-inputs
> > "qtcolorwidgets")
> > +
> > "/lib/libQtColorWidgets-Qt52.so"))))
>
> What about using FIND-FILES "\*.so$" here instead of hard-coding
> "52"? Overkill?
>
> Kind regards,
>
> T G-R
[0001-gnu-Add-drawpile.patch (text/x-patch, attachment)]
Reply sent
to
Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
:
You have taken responsibility.
(Fri, 06 Aug 2021 04:21:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
<pkill9 <at> runbox.com>
:
bug acknowledged by developer.
(Fri, 06 Aug 2021 04:21:02 GMT)
Full text and
rfc822 format available.
Message #22 received at 34572-done <at> debbugs.gnu.org (full text, mbox):
Hello,
We now have drawpile 2.1.17 in Guix. It seems this patch failed between
the cracks. Sorry!
Closing.
Maxim
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org
.
(Fri, 03 Sep 2021 11:24:09 GMT)
Full text and
rfc822 format available.
This bug report was last modified 2 years and 227 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.