GNU bug report logs - #34572
Add Drawpile

Previous Next

Package: guix-patches;

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.

View this report as an mbox folder, status mbox, maintainer mbox


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):

From: <pkill9 <at> runbox.com>
To: "guix-patches" <guix-patches <at> gnu.org>
Subject: Add Drawpile
Date: Tue, 19 Feb 2019 11:59:44 +0000 (GMT)
[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):

From: Ricardo Wurmus <rekado <at> elephly.net>
To: pkill9 <at> runbox.com
Cc: 34572 <at> debbugs.gnu.org
Subject: Re: [bug#34572] Add Drawpile
Date: Wed, 20 Feb 2019 22:01:30 +0100
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):

From: <pkill9 <at> runbox.com>
To: "Ricardo Wurmus" <rekado <at> elephly.net>
Cc: 34572 <34572 <at> debbugs.gnu.org>
Subject: Re: [bug#34572] Add Drawpile
Date: Thu, 21 Feb 2019 12:51:36 +0000 (GMT)
[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):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: pkill9 <at> runbox.com
Cc: 34572 <34572 <at> debbugs.gnu.org>
Subject: Re: [bug#34572] Add Drawpile
Date: Thu, 21 Feb 2019 14:58:33 +0100
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):

From: <pkill9 <at> runbox.com>
To: "Tobias Geerinckx-Rice" <me <at> tobias.gr>
Cc: 34572 <34572 <at> debbugs.gnu.org>
Subject: Re: [bug#34572] Add Drawpile
Date: Fri, 22 Feb 2019 07:28:24 +0000 (GMT)
[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):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: <pkill9 <at> runbox.com>
Cc: Tobias Geerinckx-Rice <me <at> tobias.gr>, 34572 <34572-done <at> debbugs.gnu.org>
Subject: Re: bug#34572: Add Drawpile
Date: Fri, 06 Aug 2021 00:20:03 -0400
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 3 years and 311 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.