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

[Ricardo Wurmus]

Theodoros Foradis <address@hidden> 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")
>>> +                         (("<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.

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