From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [PATCH] gnu: Add clojure. Date: Tue, 26 Jul 2016 22:00:56 +0200 Message-ID: <87y44okw3r.fsf@elephly.net> References: <87r3b7gc4d.fsf@gmail.com> <8737ndiln5.fsf@elephly.net> <87h9bs4amm.fsf@gmail.com> <87twfe7n6t.fsf@elephly.net> <87h9bc36wg.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:37845) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bS8XS-0005Pp-Md for guix-devel@gnu.org; Tue, 26 Jul 2016 16:01:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bS8XO-0000ne-IR for guix-devel@gnu.org; Tue, 26 Jul 2016 16:01:13 -0400 Received: from sender163-mail.zoho.com ([74.201.84.163]:24529) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bS8XO-0000nJ-A5 for guix-devel@gnu.org; Tue, 26 Jul 2016 16:01:10 -0400 In-reply-to: <87h9bc36wg.fsf@gmail.com> 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: Alex Vong Cc: guix-devel@gnu.org 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