Hi Ricardo,

Thanks for the review again, the package definition is now simplier.

Ricardo Wurmus <rekado@elephly.net> writes:

> Hi Alex,
>
> Usually, we will split bundled libraries.  For bundled “jar” archives
> this is necessary in any case as a “jar” file is a binary.
>
> If the libraries are bundled in source form (not as “jar” archives) and
> if they are closely tied to clojure (or if they were forked from
> upstream libraries to better fit clojure), we could make an exception.
>
> Packaging Java libraries for Guix still isn’t very easy as we lack a
> bootstrapped set of core libraries, but you might be able to use the
> “ant-build-system” to get closer to that goal.  I also have a couple of
> packages for Java core libraries that haven’t yet been pushed.  If you
> intend to work on this I can share my current work in progress.
>
Yes, the ASM library is included as source (not jar) and is one majar
version behind upstream (4 vs 5). Also, this SO question says it is indeed a
fork (https://stackoverflow.com/questions/21642115/how-does-the-clojure-compiler-generates-jvm-bytecode).

> Here are some more comments about the patch you sent:
>
>> +           (replace 'install
>> +             (lambda* (#:key outputs #:allow-other-keys)
>> +               (let ((java-dir (string-append (assoc-ref outputs "out")
>> +                                              "/share/java/")))
>> +                 ;; Do not install clojure.jar to avoid collisions.
>> +                 (install-file (string-append "clojure-" ,version ".jar")
>> +                               java-dir)
>> +                 #t)))
>
> You don’t need this.  The “ant-build-system” allows you to override the
> name of the “jar” archive.  You can change the name to “(string-append
> "clojure-" ,version ".jar")” there without needing to override the
> install phase.
>
Actually, build.xml does not provide any install target, so we have to
roll our own. I should have made this clear. I've added a comment to
clarify this point.

>> +           (add-after 'build 'build-doc
>> +             (lambda _
>> +               (let* ((markdown-regex "(.*)\\.(md|markdown|txt)")
>> +                      (gsub regexp-substitute/global)
>> +                      (markdown->html (lambda (src-name)
>> +                                        (zero? (system*
>> +                                                "pandoc"
>> +                                                "--output" (gsub #f
>> + markdown-regex
>> +                                                                 src-name
>> +                                                                 1 ".html")
>> +                                                "--verbose"
>> +                                                "--from" "markdown_github"
>> +                                                "--to" "html"
>> +                                                src-name)))))
>> +                 (every markdown->html
>> +                        (find-files "./" markdown-regex)))))
>
> Why is this needed?  Is there no target for building the documentation?
> If you added “pandoc” to the inputs only for building the documentation
> please reconsider this decision.  The closure of the “pandoc” package is
> *massive* as it depends on countless Haskell packages.  You would make
> the “clojure” package dependent on both Java (which is large) and an
> even larger set of packages consisting of GHC and numerous packages.
>
> Couldn’t you just install the markdown files as they are?
>
Sure, we could just install the markdown files as it. Though I am
curious to know what do you mean the closure is massive. Isn't pandoc
only needed at build-time, so the user doesn't need to download the
ghc-pandoc substitute? Also, I realize I over look the `javadoc' target,
which builds documentations in addition to those markdown file. So, I
change the target to the following:

;;; The javadoc target is not built by default.
(add-after 'build 'build-doc
  (lambda _
    (system* "ant" "javadoc")))

>> +           (add-after 'install 'install-doc
>> +             (lambda* (#:key outputs #:allow-other-keys)
>> +               (let ((doc-dir (string-append (assoc-ref outputs "out")
>> +                                             "/share/doc/clojure-"
>> +                                             ,version "/"))
>> +                     (copy-file-to-dir (lambda (file dir)
>> +                                         (copy-file file
>> +                                                    (string-append dir
>> +                                                                   file)))))
>> +                 (for-each delete-file
>> +                           (find-files "doc/clojure/"
>> +                                       ".*\\.(md|markdown|txt)"))
>> +                 (copy-recursively "doc/clojure/" doc-dir)
>> +                 (for-each (cut copy-file-to-dir <> doc-dir)
>> +                           (filter (cut string-match ".*\\.(html|txt)" <>)
>> +                                   (scandir "./")))
>> +                 #t))))))
>
> Similar comments here.  Why delete the markdown documentation?  I’d much
> prefer to have the original plain text files.
>
With the new build-doc target, we now only need to copy
`doc/' to `share/doc/'
`target/javadoc/' to `share/doc/javadoc/' and
other top-level markdown/html files to `doc/',
another simplification.

> What do you think?
>
Finally, I want to ask do I need to sign my commit? I sign my commit and
do a `magit-format-patch', but it seems the patch does not contain info
of the signature.

> ~~ Ricardo

Thanks,
Alex