Ludovic Courtès <ludo@gnu.org> writes:

> Ricardo Wurmus <rekado@elephly.net> skribis:
>
>> From da77c25e8c32243ca2bd77bd76de41312aafaac1 Mon Sep 17 00:00:00 2001
>> From: Ricardo Wurmus <rekado@elephly.net>
>> Date: Mon, 25 May 2015 22:13:27 +0200
>> Subject: [PATCH 1/6] gnu: Add withershins.
>>
>> * gnu/packages/code.scm (withershins): New variable.

[...]

> BFD is GPLv3+ so the whole thing and its users are GPLv3+ once combined.
> I guess ‘license’ should be gpl3+, with a comment saying that the source
> is Expat?

Okay, I added a comment and set the license to gpl3+.

>> From 98e2ab304ef345178ab1caad27d6e4412d19c476 Mon Sep 17 00:00:00 2001
>> From: Ricardo Wurmus <rekado@elephly.net>
>> Date: Thu, 4 Jun 2015 10:01:11 +0200
>> Subject: [PATCH 6/6] gnu: Add powertabeditor.
>>
>> * gnu/packages/music.scm (powertabeditor): New variable.
>
> [...]
>
>> +    (name "powertabeditor")
>> +    (version "2.0.0-alpha7")
>
> I suppose the stable version is way too old or non-existent?

There is no stable version.  Power Tab Editor 2.0 is intended as the
successor to the old Windows-only Power Tab Editor 1.7, whose announced
successor turned out to be vapourware.  PTE2.0 will be the first version
of the independently developed successor.  The latest release is
2.0.0-alpha7.

>> +       #:configure-flags
>> +       (list (string-append "-DCMAKE_INSTALL_RPATH='"
>> +                            (string-join (map (match-lambda
>> +                                                ((name . directory)
>> +                                                 (string-append directory "/lib")))
>> +                                              %build-inputs)
>> +                                         ";")
>> +                            "'"))
>
> Could you add a comment explaining why this is needed?  Ideally this
> would be handled by ‘cmake-build-system’.

It should not be needed in theory, but without it CMake would forget the
RUNPATH by the time the validation is run.  Setting the
CMAKE_INSTALL_RPATH variable explicitly was the only way I could make it
retain the RUNPATH.

> I think the single quotes aren’t needed, are they?  Also, the semicolon
> is surprising here.

You are right.  The semicolon is permitted according to CMake
documentation (I didn't see it mention the more common colon, but it
seems to be permitted as well).

>> +       #:phases
>> +       (modify-phases %standard-phases
>> +         (add-before
>> +          'configure 'remove-third-party-libs
>> +          (lambda _
>> +            (substitute* "CMakeLists.txt"
>> +              (("include_directories\\(\\$\\{PROJECT_SOURCE_DIR\\}/external/.*") "")
>> +              ;; TODO: tests cannot be built:
>> +              ;; test/test_main.cpp:28:12: error: ‘Session’ is not a member of ‘Catch’
>> +              (("add_subdirectory\\(test\\)") "")
>> +              (("add_subdirectory\\(external\\)") ""))
>
> Shouldn’t this and...
>
>> +            (delete-file-recursively "external")
>
> ... this, and possibly...
>
>> +            #t))
>> +         (add-after
>> +          'unpack 'add-install-target
>> +          (lambda _
>> +            (substitute* "source/CMakeLists.txt"
>> +              (("qt5_use_modules")
>> +               "install(TARGETS powertabeditor RUNTIME DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)
>> +install(FILES data/tunings.json DESTINATION ${CMAKE_INSTALL_PREFIX}/share/powertabeditor/)
>> +qt5_use_modules"))
>
> ... this be done in a snippet?

In that case everything I'm doing in the 'remove-third-party-libs phase
(except for the substitutions depending on build inputs) should be done
in a snippet, I think.  The only reason why these substitutions are
required is because we're not using the bundled sources.

>> +    (description
>> +     "PTE2.0 is the successor to the famous Power Tab Editor.  It is
>> +compatible with PTE1.7 and Guitar Pro.")
>
> Isn’t “PTE” and “Power Tab Editor” the same thing?  Furthermore, the
> package name is ‘powertabeditor’, not ‘pte’.

I updated the description.  Attached is a new patch.
Thank you for the review!