GNU bug report logs - #34572
Add Drawpile

Previous Next

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.

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

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

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

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

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

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

Previous Next

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