From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: [PATCH] Add powertabeditor. Date: Tue, 09 Jun 2015 15:32:28 +0200 Message-ID: <871thl3t3n.fsf@gnu.org> References: <874mml87yh.fsf@elephly.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:48763) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2Jdt-0007HX-OO for guix-devel@gnu.org; Tue, 09 Jun 2015 09:32:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z2Jdp-0004sA-GD for guix-devel@gnu.org; Tue, 09 Jun 2015 09:32:37 -0400 In-Reply-To: <874mml87yh.fsf@elephly.net> (Ricardo Wurmus's message of "Sat, 06 Jun 2015 12:08:54 +0200") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Ricardo Wurmus Cc: Guix-devel Ricardo Wurmus skribis: > From da77c25e8c32243ca2bd77bd76de41312aafaac1 Mon Sep 17 00:00:00 2001 > From: Ricardo Wurmus > 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 =E2=80=98license=E2=80=99 should be gpl3+, with a comment saying th= at the source is Expat? Otherwise LGTM. > From d426c47462be1dcc5ff4bbe9d1b154bbbb0761b9 Mon Sep 17 00:00:00 2001 > From: Ricardo Wurmus > 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 > 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 > 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=E2=80=99t provide a =E2=80=98make dist=E2=80=99-generated = tarball. > From ab277b32cd5ace5ab257ec31abd719b2ee2470dd Mon Sep 17 00:00:00 2001 > From: Ricardo Wurmus > 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 > 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=3D'" > + (string-join (map (match-lambda > + ((name . directory) > + (string-append director= y "/lib"))) > + %build-inputs) > + ";") > + "'")) Could you add a comment explaining why this is needed? Ideally this would be handled by =E2=80=98cmake-build-system=E2=80=99. I think the single quotes aren=E2=80=99t needed, are they? Also, the semic= olon is surprising here. > + #:phases > + (modify-phases %standard-phases > + (add-before > + 'configure 'remove-third-party-libs > + (lambda _ > + (substitute* "CMakeLists.txt" > + (("include_directories\\(\\$\\{PROJECT_SOURCE_DIR\\}/exter= nal/.*") "") > + ;; TODO: tests cannot be built: > + ;; test/test_main.cpp:28:12: error: =E2=80=98Session=E2=80= =99 is not a member of =E2=80=98Catch=E2=80=99 > + (("add_subdirectory\\(test\\)") "") > + (("add_subdirectory\\(external\\)") "")) Shouldn=E2=80=99t 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 ${CMA= KE_INSTALL_PREFIX}/bin) > +install(FILES data/tunings.json DESTINATION ${CMAKE_INSTALL_PREFIX}/shar= e/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=E2=80=99t =E2=80=9CPTE=E2=80=9D and =E2=80=9CPower Tab Editor=E2=80=9D = the same thing? Furthermore, the package name is =E2=80=98powertabeditor=E2=80=99, not =E2=80=98pte=E2=80=99. Otherwise LGTM! Thanks, Ludo=E2=80=99.