From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [PATCH v2 1/1] gnu: Add plantuml. Date: Sat, 29 Oct 2016 23:46:55 +0200 Message-ID: <87pomiakvk.fsf@elephly.net> References: <20161027101329.GD18185@macbook42.flashner.co.il> <20161027111853.4405-1-theodoros.for@openmailbox.org> <20161027111853.4405-2-theodoros.for@openmailbox.org> <87oa25g0ot.fsf@elephly.net> <87d1iku3f4.fsf@openmailbox.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:38472) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c0bT2-0001B8-Uq for guix-devel@gnu.org; Sat, 29 Oct 2016 17:47:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c0bSz-0008AU-RN for guix-devel@gnu.org; Sat, 29 Oct 2016 17:47:09 -0400 Received: from sender163-mail.zoho.com ([74.201.84.163]:21385) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c0bSz-0008A3-JB for guix-devel@gnu.org; Sat, 29 Oct 2016 17:47:05 -0400 In-reply-to: <87d1iku3f4.fsf@openmailbox.org> 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" To: Theodoros Foradis Cc: guix-devel Theodoros Foradis writes: >>> +(define-public plantuml […] >>> + (modify-phases %standard-phases >>> + (add-before 'build 'delete-extra-from-cp BTW: the phase name is a little hard to understand. We don’t mind slightly longer phase names if that improves readability. >>> + (lambda _ >>> + (substitute* "build.xml" >>> + (("1.6") "1.7") >>> + ((">> + (("j2v8_macosx_x86_64-3.1.7.jar\" />") >>> "-->")) Another thing I forgot: is this jar bundled? If so, it should be removed in a snippet. >>> + #t)) >>> + (add-before 'install 'gen-install >>> + (lambda* (#:key outputs #:allow-other-keys) >>> + (mkdir-p "build/jar") >>> + (system* "mv" "plantuml.jar" "build/jar") >>> + ((@@ (guix build ant-build-system) default-build.xml) >>> + "plantuml.jar" >>> + (string-append (assoc-ref outputs "out") >>> + "/share/java")))) >> >> I don’t understand this. Do you only use “default-build.xml” to add an >> install target? In the previous phase you use the included “build.xml”. >> I find this a little odd and think it would be clearer to just install >> the files manually instead of using “default-build.xml” here. > > The build.xml that our ant-build-system generates, does not generate a > correct manifest attribute with the Main-Class, so the produced jar file > cannot be run. Instead of generating the required text manually, I use > the default build.xml for the build phase. > > The default build.xml does not include an install phase, so I generate > it after compilation, with our ant-build-system. > > Feedback is welcome, if there is a better way to do this, before I > reformat the patch. (I’m a little confused. When you write “default build.xml” you mean the included “build.xml”, not the one generated by the procedure “default-build.xml”, right?) Should the “default-build.xml” procedure be changed to (conditionally) add the “Main-Class” attribute? In this case I think using “default-build.xml” just for the install phase is a little over the top. All it does is copy the jars to the target directory: (target (@ (name "install")) (copy (@ (todir "${dist.dir}")) (fileset (@ (dir "${jar.dir}")) (include (@ (name "**/*.jar")))))) That’s something you could do with Scheme directly: (replace 'install (lambda* (#:key outputs #:allow-other-keys) (for-each (lambda … install-file …) (find-files … "\\.jar$")) #t)) I find that a lot clearer than accessing a private procedure from “ant-build-system”. >> >>> + (add-after 'install 'make-wrapper >>> + (lambda* (#:key inputs outputs #:allow-other-keys) >> >> Please check the indentation for all phases. That’s too far to the >> right. >> > > I will fix that. This is the default indentation emacs does with this, > so I forgot to fix it manually. Actually, we have a “.dir-locals.el” file that Emacs should respect. Using the settings in that file will tell Emacs to indent these things correctly. (Many of us here are using Emacs and we don’t fix indentation manually.) ~~ Ricardo