Patchwork gnu: Add clojure.

login
register
mail settings
Submitter Alex Vong
Date July 26, 2016, 12:45 p.m.
Message ID <87h9bc36wg.fsf@gmail.com>
Download mbox | patch
Permalink /patch/13984/
State New
Headers show

Comments

Alex Vong - July 26, 2016, 12:45 p.m.
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
Ricardo Wurmus - July 26, 2016, 8 p.m.
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

Patch

From b4094a8acf9f7b41085f5c3aa0d548638ea8cb48 Mon Sep 17 00:00:00 2001
From: Alex Vong <alexvong1995@gmail.com>
Date: Tue, 22 Dec 2015 01:06:33 +0800
Subject: [PATCH] fix gcj build

---
 build.xml                          | 29 ++++++++++++++++++++++-------
 src/jvm/clojure/lang/Compiler.java | 26 +++++++++++++-------------
 src/jvm/clojure/lang/Numbers.java  | 35 ++++++++++++++++++++++++-----------
 src/jvm/clojure/lang/Ratio.java    | 22 ++++++++++++++++++++--
 4 files changed, 79 insertions(+), 33 deletions(-)

diff --git a/build.xml b/build.xml
index cae30b2..2427d68 100644
--- a/build.xml
+++ b/build.xml
@@ -40,8 +40,10 @@ 
   <target name="compile-java" depends="init"
           description="Compile Java sources.">
     <javac srcdir="${jsrc}" destdir="${build}" includeJavaRuntime="yes"
-           includeAntRuntime="false"
-           debug="true" source="1.6" target="1.6"/>
+           includeAntRuntime="false" compiler="gcj"
+           debug="true" source="1.6" target="1.6">
+      <compilerarg value="-O3"/>
+    </javac>
   </target>
 
   <target name="compile-clojure"
@@ -49,7 +51,9 @@ 
     <java classname="clojure.lang.Compile"
           classpath="${maven.compile.classpath}:${build}:${cljsrc}"
           failonerror="true"
+          jvm="gij"
           fork="true">
+      <jvmarg value="-noverify"/>
       <sysproperty key="clojure.compile.path" value="${build}"/>
          <!--<sysproperty key="clojure.compiler.elide-meta" value="[:doc :file :line :added]"/>-->
          <!--<sysproperty key="clojure.compiler.disable-locals-clearing" value="true"/>-->
@@ -88,13 +92,18 @@ 
           description="Compile the subset of tests that require compilation."
           unless="maven.test.skip">
     <mkdir dir="${test-classes}"/>
-    <javac srcdir="${jtestsrc}" destdir="${test-classes}" includeJavaRuntime="yes"
-           debug="true" source="1.6" target="1.6" includeantruntime="no"/>
+    <javac srcdir="${jtestsrc}" destdir="${test-classes}"
+           includeJavaRuntime="yes" compiler="gcj"
+           debug="true" source="1.6" target="1.6" includeantruntime="no">
+      <compilerarg value="-O3"/>
+    </javac>
     <echo>Direct linking = ${directlinking}</echo>
     <java classname="clojure.lang.Compile"
           classpath="${test-classes}:${test}:${build}:${cljsrc}"
           failonerror="true"
-	  fork="true">
+          jvm="gij"
+          fork="true">
+      <jvmarg value="-noverify"/>
       <sysproperty key="clojure.compile.path" value="${test-classes}"/>
         <!--<sysproperty key="clojure.compiler.elide-meta" value="[:doc]"/>-->
         <!--<sysproperty key="clojure.compiler.disable-locals-clearing" value="true"/>-->
@@ -110,7 +119,10 @@ 
           description="Run clojure tests without recompiling clojure."
           depends="compile-tests"
           unless="maven.test.skip">
-    <java classname="clojure.main" failonerror="true" fork="true">
+    <java classname="clojure.main" failonerror="true"
+          jvm="gij"
+          fork="true">
+      <jvmarg value="-noverify"/>
       <sysproperty key="clojure.test-clojure.exclude-namespaces"
                    value="#{clojure.test-clojure.compilation.load-ns}"/>
       <sysproperty key="clojure.compiler.direct-linking" value="${directlinking}"/>
@@ -129,7 +141,10 @@ 
           description="Run test generative tests without recompiling clojure."
           depends="compile-tests"
           unless="maven.test.skip">
-    <java classname="clojure.main" failonerror="true" fork="true">
+    <java classname="clojure.main" failonerror="true"
+          jvm="gij"
+          fork="true">
+      <jvmarg value="-noverify"/>
       <sysproperty key="clojure.compiler.direct-linking" value="${directlinking}"/>
       <classpath>
         <pathelement path="${maven.test.classpath}"/>
diff --git a/src/jvm/clojure/lang/Compiler.java b/src/jvm/clojure/lang/Compiler.java
index 6066825..d40099b 100644
--- a/src/jvm/clojure/lang/Compiler.java
+++ b/src/jvm/clojure/lang/Compiler.java
@@ -5347,19 +5347,19 @@  public static class FnMethod extends ObjMethod{
 					                  registerLocal(p, null, new MethodParamExpr(pc), true)
 					                           : registerLocal(p, state == PSTATE.REST ? ISEQ : tagOf(p), null, true);
 					argLocals = argLocals.cons(lb);
-					switch(state)
-						{
-						case REQ:
-							method.reqParms = method.reqParms.cons(lb);
-							break;
-						case REST:
-							method.restParm = lb;
-							state = PSTATE.DONE;
-							break;
-
-						default:
-							throw Util.runtimeException("Unexpected parameter");
-						}
+					if (state == PSTATE.REQ)
+                                            {
+                                                method.reqParms = method.reqParms.cons(lb);
+                                            }
+                                        else if (state == PSTATE.REST)
+                                            {
+                                                method.restParm = lb;
+                                                state = PSTATE.DONE;
+                                            }
+                                        else
+                                            {
+                                                throw Util.runtimeException("Unexpected parameter");
+                                            }
 					}
 				}
 			if(method.reqParms.count() > MAX_POSITIONAL_ARITY)
diff --git a/src/jvm/clojure/lang/Numbers.java b/src/jvm/clojure/lang/Numbers.java
index 623743b..9b828de 100644
--- a/src/jvm/clojure/lang/Numbers.java
+++ b/src/jvm/clojure/lang/Numbers.java
@@ -15,6 +15,7 @@  package clojure.lang;
 import java.math.BigInteger;
 import java.math.BigDecimal;
 import java.math.MathContext;
+import java.math.RoundingMode;
 
 public class Numbers{
 
@@ -941,23 +942,35 @@  final static class BigDecimalOps extends OpsP{
 
 	public Number divide(Number x, Number y){
 		MathContext mc = (MathContext) MATH_CONTEXT.deref();
-		return mc == null
-		       ? toBigDecimal(x).divide(toBigDecimal(y))
-		       : toBigDecimal(x).divide(toBigDecimal(y), mc);
+                RoundingMode mode = mc.getRoundingMode();
+                int vals[] =
+                    {
+                        BigDecimal.ROUND_UP,
+                        BigDecimal.ROUND_DOWN,
+                        BigDecimal.ROUND_CEILING,
+                        BigDecimal.ROUND_FLOOR,
+                        BigDecimal.ROUND_HALF_UP,
+                        BigDecimal.ROUND_HALF_DOWN,
+                        BigDecimal.ROUND_HALF_EVEN,
+                        BigDecimal.ROUND_UNNECESSARY
+                    };
+                int i;
+
+                for (i = 0; i < vals.length; ++i)
+                    if (mode == RoundingMode.valueOf(vals[i]))
+                        return mc == null
+                            ? toBigDecimal(x).divide(toBigDecimal(y))
+                            : toBigDecimal(x).divide(toBigDecimal(y), vals[i]);
+
+                throw new IllegalArgumentException("invalid rounding mode");
 	}
 
 	public Number quotient(Number x, Number y){
-		MathContext mc = (MathContext) MATH_CONTEXT.deref();
-		return mc == null
-		       ? toBigDecimal(x).divideToIntegralValue(toBigDecimal(y))
-		       : toBigDecimal(x).divideToIntegralValue(toBigDecimal(y), mc);
+		return toBigDecimal(x).divideToIntegralValue(toBigDecimal(y));
 	}
 
 	public Number remainder(Number x, Number y){
-		MathContext mc = (MathContext) MATH_CONTEXT.deref();
-		return mc == null
-		       ? toBigDecimal(x).remainder(toBigDecimal(y))
-		       : toBigDecimal(x).remainder(toBigDecimal(y), mc);
+		return toBigDecimal(x).remainder(toBigDecimal(y));
 	}
 
 	public boolean equiv(Number x, Number y){
diff --git a/src/jvm/clojure/lang/Ratio.java b/src/jvm/clojure/lang/Ratio.java
index 6c7a9bb..c7d2ff5 100644
--- a/src/jvm/clojure/lang/Ratio.java
+++ b/src/jvm/clojure/lang/Ratio.java
@@ -15,6 +15,7 @@  package clojure.lang;
 import java.math.BigInteger;
 import java.math.BigDecimal;
 import java.math.MathContext;
+import java.math.RoundingMode;
 
 public class Ratio extends Number implements Comparable{
 final public BigInteger numerator;
@@ -63,8 +64,25 @@  public BigDecimal decimalValue(){
 public BigDecimal decimalValue(MathContext mc){
 	BigDecimal numerator = new BigDecimal(this.numerator);
 	BigDecimal denominator = new BigDecimal(this.denominator);
-
-	return numerator.divide(denominator, mc);
+        RoundingMode mode = mc.getRoundingMode();
+        int vals[] =
+            {
+                BigDecimal.ROUND_UP,
+                BigDecimal.ROUND_DOWN,
+                BigDecimal.ROUND_CEILING,
+                BigDecimal.ROUND_FLOOR,
+                BigDecimal.ROUND_HALF_UP,
+                BigDecimal.ROUND_HALF_DOWN,
+                BigDecimal.ROUND_HALF_EVEN,
+                BigDecimal.ROUND_UNNECESSARY
+            };
+        int i;
+
+        for (i = 0; i < vals.length; ++i)
+            if (mode == RoundingMode.valueOf(vals[i]))
+                return numerator.divide(denominator, vals[i]);
+
+        throw new IllegalArgumentException("invalid rounding mode");
 }
 
 public BigInteger bigIntegerValue(){
-- 
2.6.3