Re: [PATCH] gnu: Add clojure.

[Ricardo Wurmus]

Hi Alex,

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

You only attached the patch to the Clojure sources.  Could you please
also attach the latest patch to add the clojure package?

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

In this case I think it’s okay to not carve it out of the Clojure source
archive.  Once we need an ASM package in the future we can revisit this
decision and see if we can express one in terms of the other.

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

Ah, that’s because you are not using the “build.xml” file that the
“ant-build-system” would generate for you.  That’s correct — we only let
the “ant-build-system” generate a “build.xml” file with standard targets
when there is none or when the provided file cannot be used.

Adding a comment to explain why the install phase needs to be replaced
is sufficient in this case.

>>> +           (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?

I mean that the closure of the “ghc-pandoc” package is big.  Few
markdown files *actually* need the features of the markdown
implementation provided by pandoc; in many cases one of the simpler
implementations can be used.  Especially for packages that provide
programming languages I have a preference for keeping the list of
build-time inputs reasonably small.

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

Good catch!  Please use “(zero? (system* …))” to make sure that the
phase fails when the ant target fails.

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

Great!

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

The signature would not make it into the repository if you sent the
commit as a patch.  The committer to the central repository at Savannah
is the one who signs the commit — this does not mean that the committer
claims authorship, of course.

Thanks again for your work.  Please send the missing patch some time :)

~~ Ricardo