[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] gnu: Add denemo.
From: |
Kei Kebreau |
Subject: |
Re: [PATCH] gnu: Add denemo. |
Date: |
Fri, 09 Dec 2016 22:09:02 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
address@hidden (Ludovic Courtès) writes:
> Kei Kebreau <address@hidden> 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 <address@hidden>
>> 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’.
0001-gnu-Add-denemo.patch
Description: Text document
signature.asc
Description: PGP signature