Re: [PATCH v2 1/1] gnu: Add plantuml.

[Theodoros Foradis <theodoros.for@openmailbox.org>]

Ricardo Wurmus writes:

> Theodoros Foradis <theodoros.for@openmailbox.org> 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.
>

I'll change that to delete-extra-from-classpath.

>>>> +                     (lambda _
>>>> +                       (substitute* "build.xml"
>>>> +                         (("1.6") "1.7")
>>>> +                         (("<attribute name=\"Class-Path\"") "<!--")
>>>> +                         (("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. 

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

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

Right.

>
> Should the “default-build.xml” procedure be changed to (conditionally)
> add the “Main-Class” 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 “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”.
>

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

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,
-- 
Theodoros Foradis