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@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