ludo@gnu.org (Ludovic Courtès) writes:

> Kei Kebreau <kei@openmailbox.org> skribis:
>
>> Here is an updated patch for GNU Denemo.
>
> Nice work!
>

Thank you! :)

>> Everything seems fine except for grafting (i.e. disabling grafting
>> renders the issue invisible). For some reason, "find-files"
>> does not recognize a file with a Unicode-encoded filename when called
>> inside "rename-matching-files" from guix/build/graft.scm. When
>> "find-files" is used on its own, the file is recognized properly.
>> Is anyone familiar with the grafting code available to help figure out
>> what is happening to the file name?
>
> Problem is that the grafting code (‘graft-derivation/shallow’ in (guix
> grafts)) is running in the C locale, so it expects file names to be
> ASCII.  I’ll look into it.
>

I saw your email about renaming the file with the Unicode name. Your
method worked fine.

> Some comments on the package:
>
>> From 6bd5843bef06a02ecf1235090350562c8b096aca Mon Sep 17 00:00:00 2001
>> From: Kei Kebreau <kei@openmailbox.org>
>> Date: Thu, 8 Dec 2016 14:00:43 -0500
>> Subject: [PATCH] gnu: Add denemo.
>>
>> * gnu/packages/music.scm (denemo): New variable.
>
> [...]
>
>> +    (arguments
>> +     `(#:phases
>> +       (modify-phases %standard-phases
>> +         (replace 'check
>> +           (lambda _
>> +             (zero? (system* "make" "-C" "tests" "check")))))))
>
> Is this really needed?  Perhaps leave a comment explaining whether/why
> “make check” at the top level is broken (and perhaps report it as a bug
> upstream!).
>

Maybe because upstream doesn't test it? Denemo documentation says to use
this command to run the testsuite
(http://denemo.org/hacking-sources/#Test_suite). I've added a comment
explaining that.

>> +    (native-inputs
>> +     `(("autoconf" ,autoconf)
>> +       ("automake" ,automake)
>
> This is not needed (or it’s a bug too ;-)).
>

Indeed it is not. I've removed these inputs from the new patch. As a
side note, lilypond was required as a runtime dependency so I moved it
to propagated-inputs.

>> +    (license (list license:cc-by-sa3.0
>> +                   license:lgpl2.1+
>> +                   license:gpl2
>> +                   license:gpl2+
>> +                   license:gpl3
>> +                   license:gpl3+))))
>
> I think ‘gpl3+’ is enough here since it “wins”.  You can leave a comment
> explaining where the other licenses appear, though.
>

Looking over the plethora of libre licenses included in the source, I
decided to just use the overarching gpl3+ from COPYING.

> Thanks!
>

Thank you for the review! The new patch is attached.

> Ludo’.