Hi Ricardo,
Thanks for the review again, the package definition is now simplier.
Ricardo Wurmus <rek...@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
>From b4094a8acf9f7b41085f5c3aa0d548638ea8cb48 Mon Sep 17 00:00:00 2001
From: Alex Vong <alexvong1...@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
