Re: [PATCH] gnu: Add clojure.

[Alex Vong <alexvong1995@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5816 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-fix-gcj-build.patch --]
[-- Type: text/x-diff, Size: 9227 bytes --]

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