Re: [PATCH] Add powertabeditor.

[ludo@gnu.org (Ludovic Courtès)]

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.

[...]

> +    (inputs
> +     `(("gcc" ,gcc-4.8 "lib") ;for libiberty.a
> +       ("binutils" ,binutils) ;for libbfd
> +       ("zlib" ,zlib)))
> +    (synopsis "C++11 library for generating stack traces")
> +    (description
> +     "Withershins is a simple cross-platform C++11 library for generating
> +stack traces.")
> +    (license license:expat)))

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?

Otherwise LGTM.

> From d426c47462be1dcc5ff4bbe9d1b154bbbb0761b9 Mon Sep 17 00:00:00 2001
> From: Ricardo Wurmus <rekado@elephly.net>
> Date: Mon, 25 May 2015 22:14:39 +0200
> Subject: [PATCH 2/6] gnu: Add RapidJSON.
>
> * gnu/packages/web.scm (rapidjson): New variable.

OK.

> From 337f0790e7917f7ae2b394310c8543256756f0fe Mon Sep 17 00:00:00 2001
> From: Ricardo Wurmus <rekado@elephly.net>
> Date: Thu, 28 May 2015 09:43:53 +0200
> Subject: [PATCH 3/6] gnu: Add pugixml.
>
> * gnu/packages/xml.scm (pugixml): New variable.

OK.

> From 67ec5348f611d58299ae2ab0b8575e817e0f1272 Mon Sep 17 00:00:00 2001
> From: Ricardo Wurmus <rekado@elephly.net>
> Date: Thu, 28 May 2015 09:44:30 +0200
> Subject: [PATCH 4/6] gnu: Add rtmidi.
>
> * gnu/packages/audio.scm (rtmidi): New variable.

OK.

> +     `(("autoconf" ,autoconf)
> +       ("automake" ,automake)
> +       ("libtool" ,libtool)

Too bad they don’t provide a ‘make dist’-generated tarball.

> From ab277b32cd5ace5ab257ec31abd719b2ee2470dd Mon Sep 17 00:00:00 2001
> From: Ricardo Wurmus <rekado@elephly.net>
> Date: Wed, 3 Jun 2015 22:53:56 +0200
> Subject: [PATCH 5/6] gnu: catch-framework: Update to 1.1.3.
>
> * gnu/packages/check.scm (catch-framework): Update to 1.1.3.

OK.

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

> +       #: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’.

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

> +       #: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?

> +    (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’.

Otherwise LGTM!

Thanks,
Ludo’.