From mboxrd@z Thu Jan 1 00:00:00 1970 From: Theodoros Foradis Subject: Re: [PATCH v2 1/1] gnu: Add plantuml. Date: Wed, 02 Nov 2016 02:07:55 +0200 Message-ID: <874m3qu4kk.fsf@openmailbox.org> 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> <87pomiakvk.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]:52833) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c1j98-00042f-5D for guix-devel@gnu.org; Tue, 01 Nov 2016 20:11:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c1j93-0005Pm-5p for guix-devel@gnu.org; Tue, 01 Nov 2016 20:11:14 -0400 Received: from smtp3.openmailbox.org ([62.4.1.37]:60068) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c1j92-0005P8-SA for guix-devel@gnu.org; Tue, 01 Nov 2016 20:11:09 -0400 In-reply-to: <87pomiakvk.fsf@elephly.net> 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: Guix-devel Ricardo Wurmus writes: > Theodoros Foradis writes: > >>>> +(define-public plantuml > > [=E2=80=A6] > >>>> + (modify-phases %standard-phases >>>> + (add-before 'build 'delete-extra-from-cp > > BTW: the phase name is a little hard to understand. We don=E2=80=99t m= ind > slightly longer phase names if that improves readability. > I'll change that to delete-extra-from-classpath. >>>> + (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. > The jar is not bundled. With those substitutions, I comment out all the extra jars (not included anyway) from the classpath.=20 >>>> + #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-buil= d.xml) >>>> + "plantuml.jar" >>>> + (string-append (assoc-ref outputs "out") >>>> + "/share/java")))) >>> >>> I don=E2=80=99t understand this. Do you only use =E2=80=9Cdefault-bu= ild.xml=E2=80=9D to add an >>> install target? In the previous phase you use the included =E2=80=9C= build.xml=E2=80=9D. >>> I find this a little odd and think it would be clearer to just instal= l >>> the files manually instead of using =E2=80=9Cdefault-build.xml=E2=80=9D= here. >> Right, I should install manually instead. Generating the default-build.xml is unneeded. >> The build.xml that our ant-build-system generates, does not generate a >> correct manifest attribute with the Main-Class, so the produced jar fi= le >> 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=E2=80=99m a little confused. When you write =E2=80=9Cdefault build.= xml=E2=80=9D you mean the > included =E2=80=9Cbuild.xml=E2=80=9D, not the one generated by the proc= edure > =E2=80=9Cdefault-build.xml=E2=80=9D, right?) Right. > > Should the =E2=80=9Cdefault-build.xml=E2=80=9D procedure be changed to = (conditionally) > add the =E2=80=9CMain-Class=E2=80=9D attribute? > I am not very knowledgeable about the ant-build-system, but I think it would be a useful addition, as runnable jars need that "Main-Class" attibute. > In this case I think using =E2=80=9Cdefault-build.xml=E2=80=9D 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=E2=80=99s something you could do with Scheme directly: > > (replace 'install > (lambda* (#:key outputs #:allow-other-keys) > (for-each (lambda =E2=80=A6 install-file =E2=80=A6) > (find-files =E2=80=A6 "\\.jar$")) > #t)) > > I find that a lot clearer than accessing a private procedure from > =E2=80=9Cant-build-system=E2=80=9D. > Thanks for the hint. I'll skip the "default-build.xml" and do it with Scheme directly. >>> >>>> + (add-after 'install 'make-wrapper >>>> + (lambda* (#:key inputs outputs #:allow-other-ke= ys) >>> >>> Please check the indentation for all phases. That=E2=80=99s 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 =E2=80=9C.dir-locals.el=E2=80=9D file that Emacs sh= ould 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=E2=80=99t fix > indentation manually.) > > ~~ Ricardo Thanks for letting me know. I thought I had been using that ".dir-locals.el", and that this indentation wasn't included, but I should have made some mistake. Regards, --=20 Theodoros Foradis