Re: [PATCH] Add powertabeditor.

[Ricardo Wurmus <rekado@elephly.net>]

[-- Attachment #1: Type: text/plain, Size: 4234 bytes --]


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!


[-- Attachment #2: 0001-gnu-Add-powertabeditor.patch --]
[-- Type: text/x-patch, Size: 6037 bytes --]

From 18eaadd221e098dfd769cc6aa9c8920730c2674f Mon Sep 17 00:00:00 2001
From: Ricardo Wurmus <rekado@elephly.net>
Date: Thu, 4 Jun 2015 10:01:11 +0200
Subject: [PATCH] gnu: Add powertabeditor.

* gnu/packages/music.scm (powertabeditor): New variable.
---
 gnu/packages/music.scm | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/gnu/packages/music.scm b/gnu/packages/music.scm
index 5795ecb..a42d5c3 100644
--- a/gnu/packages/music.scm
+++ b/gnu/packages/music.scm
@@ -22,9 +22,15 @@
   #:use-module (guix download)
   #:use-module ((guix licenses) #:prefix license:)
   #:use-module (guix build-system gnu)
+  #:use-module (guix build-system cmake)
   #:use-module (gnu packages)
   #:use-module (gnu packages audio)
+  #:use-module (gnu packages base) ;libbdf
+  #:use-module (gnu packages boost)
   #:use-module (gnu packages bison)
+  #:use-module (gnu packages code)
+  #:use-module (gnu packages check)
+  #:use-module (gnu packages compression)
   #:use-module (gnu packages docbook)
   #:use-module (gnu packages flex)
   #:use-module (gnu packages fonts)
@@ -45,9 +51,11 @@
   #:use-module (gnu packages perl)
   #:use-module (gnu packages pkg-config)
   #:use-module (gnu packages python)
+  #:use-module (gnu packages qt)
   #:use-module (gnu packages rsync)
   #:use-module (gnu packages texinfo)
   #:use-module (gnu packages texlive)
+  #:use-module (gnu packages web)
   #:use-module (gnu packages xml)
   #:use-module (gnu packages xiph)
   #:use-module (gnu packages zip))
@@ -224,6 +232,96 @@ sessions.  Solfege is also designed to be extensible so you can easily write
 your own lessons.")
     (license license:gpl3+)))
 
+(define-public powertabeditor
+  (package
+    (name "powertabeditor")
+    (version "2.0.0-alpha7")
+    (source (origin
+              (method url-fetch)
+              (uri (string-append
+                    "https://github.com/powertab/powertabeditor/archive/"
+                    version ".tar.gz"))
+              (file-name (string-append name "-" version ".tar.gz"))
+              (sha256
+               (base32
+                "1yp6ck2r72c2pfq31z1kpw1j639rndrifj85l3cbj2kdf8rdzhkk"))
+              (modules '((guix build utils)))
+              (snippet
+               '(begin
+                  ;; Remove bundled sources for external libraries
+                  (delete-file-recursively "external")
+                  (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\\)") ""))
+                  (substitute* "test/CMakeLists.txt"
+                    (("include_directories\\(\\$\\{PROJECT_SOURCE_DIR\\}/external/.*") ""))
+
+                  ;; Add install target
+                  (substitute* "source/CMakeLists.txt"
+                    (("qt5_use_modules")
+                     (string-append
+                      "install(TARGETS powertabeditor "
+                      "RUNTIME DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)\n"
+                      "install(FILES data/tunings.json DESTINATION "
+                      "${CMAKE_INSTALL_PREFIX}/share/powertabeditor/)\n"
+                      "qt5_use_modules")))
+                  #t))))
+    (build-system cmake-build-system)
+    (arguments
+     `(#:tests? #f ; no "check" target
+       #:modules ((guix build cmake-build-system)
+                  (guix build utils)
+                  (ice-9 match))
+       #:configure-flags
+       ;; CMake appears to lose the RUNPATH for some reason, so it has to be
+       ;; explicitly set with CMAKE_INSTALL_RPATH.
+       (list (string-append "-DCMAKE_INSTALL_RPATH="
+                            (string-join (map (match-lambda
+                                                ((name . directory)
+                                                 (string-append directory "/lib")))
+                                              %build-inputs) ";")))
+       #:phases
+       (modify-phases %standard-phases
+         (add-before
+          'configure 'remove-third-party-libs
+          (lambda* (#:key inputs #:allow-other-keys)
+            ;; Link with required static libraries, because we're not
+            ;; using the bundled version of withershins.
+            (substitute* '("source/CMakeLists.txt"
+                           "test/CMakeLists.txt")
+              (("target_link_libraries\\((powertabeditor)" _ target)
+               (string-append "target_link_libraries(" target " "
+                              (assoc-ref inputs "binutils")
+                              "/lib/libbfd.a "
+                              (assoc-ref inputs "gcc")
+                              "/lib/libiberty.a "
+                              "dl")))
+            #t)))))
+    (inputs
+     `(("boost" ,boost)
+       ("alsa-lib" ,alsa-lib)
+       ("qt" ,qt)
+       ("withershins" ,withershins)
+       ("gcc" ,gcc-4.8 "lib") ;for libiberty.a (for withershins)
+       ("binutils" ,binutils) ;for -lbfd and -liberty (for withershins)
+       ("timidity" ,timidity++)
+       ("pugixml" ,pugixml)
+       ("rtmidi" ,rtmidi)
+       ("rapidjson" ,rapidjson)
+       ("zlib" ,zlib)))
+    (native-inputs
+     `(("catch" ,catch-framework)
+       ("pkg-config" ,pkg-config)))
+    (home-page "http://powertabs.net")
+    (synopsis "Guitar tablature editor")
+    (description
+     "Power Tab Editor 2.0 is the successor to the famous original Power Tab
+Editor.  It is compatible with Power Tab Editor 1.7 and Guitar Pro.")
+    (license license:gpl3+)))
+
 (define-public tuxguitar
   (package
     (name "tuxguitar")
-- 
2.2.1