Fork me on GitHub
#clojure-dev
<
2018-06-28
>
mikerod00:06:57

I haven’t seen a post anywhere here. What prompted the change for java 8 minimum in clj 1.10?

mikerod00:06:20

In the past I thought the Java min was intentionally held back until there were compelling reasons to force it up higher. Just curious if there is rationale somewhere. Maybe it was just to use newer asm etc

Alex Miller (Clojure team)00:06:14

Based on Clojure survey, almost everyone is now 8+ already. 6 and 7 are long eol’ed and 8 and 9 are not far off

Alex Miller (Clojure team)00:06:41

And there were bugs like the static interface call that could only be fixed with newer asm

mikerod00:06:00

Ok. So holding back was starting to be problematic as well.

mikerod00:06:33

I know that 6 is ancient history now. 😛

Alex Miller (Clojure team)00:06:07

Yes, our build box was getting increasingly fragile as all of the infrastructure was requiring newer stuff

8
seancorfield02:06:28

https://github.com/ptaoussanis/nippy/issues/108 -- not sure if this is a bug in the Alpha 5 ASM stuff or an issue with the Clojure code in Nippy.

seancorfield02:06:40

We multi-version test against 1.9 and 1.10 master and our build just failed with this... We can pin to Alpha 4 for now, but wanted to let folks here know.

ghadi02:06:23

interesting

ghadi02:06:16

Do you know which case statement in nippy is failing to compile @seancorfield?

ghadi02:06:16

I'll check it out tomorrow, we made a very large jump in the ASM version, might be something funky

ghadi02:06:28

Thanks for the report!

seancorfield02:06:27

I haven't had a chance yet to poke at the case statement (which is using Encore anyway).

ghadi02:06:48

Interesting, huge statement

seancorfield02:06:09

Some of Peter's libs have some really gnarly code in them 👀

hiredman02:06:39

I am not sure that case is the cause there, the last expr parser frame is DefExpr

seancorfield02:06:19

Caused by: java.lang.IllegalArgumentException
	at clojure.asm.commons.GeneratorAdapter.cast(GeneratorAdapter.java:785)
	at clojure.lang.Compiler$CaseExpr.emitExprForInts(Compiler.java:8812)
	at clojure.lang.Compiler$CaseExpr.doEmit(Compiler.java:8735)
	at clojure.lang.Compiler$CaseExpr.emit(Compiler.java:8712)
	at clojure.lang.Compiler$BodyExpr.emit(Compiler.java:6151)
	at clojure.lang.Compiler$LetExpr.doEmit(Compiler.java:6510)
	at clojure.lang.Compiler$LetExpr.emit(Compiler.java:6460)
	at clojure.lang.Compiler$BodyExpr.emit(Compiler.java:6151)
	at clojure.lang.Compiler$TryExpr.emit(Compiler.java:2212)
	at clojure.lang.Compiler$BodyExpr.emit(Compiler.java:6151)
	at clojure.lang.Compiler$LetExpr.doEmit(Compiler.java:6510)
	at clojure.lang.Compiler$LetExpr.emit(Compiler.java:6460)

hiredman02:06:44

ah of course, and that line in defexpr is wrapping that in a new exception

Alex Miller (Clojure team)02:06:39

would be great to get a ticket for tracking, thanks for raising it

seancorfield02:06:27

FWIW, if you have even one of those cases present, it fails to compile it seems...

seancorfield02:06:14

I'm going to see if Encore compiles and runs with 1.10.0-alpha5. Then I'll open a JIRA ticket.

Alex Miller (Clojure team)02:06:03

I think there’s a java.jdbc failure out there on the build box too @seancorfield btw. I’ve been fighting bigger fires (which I think are now resolved) and kicked off a full rebuild so we’ll see what happens when the dust settles there

seancorfield04:06:30

This is due to SQLite having different drivers for Java 6 and Java 8. Now the build's on Java 8, I'll update those dependencies.

seancorfield04:06:04

Hmm, perhaps not. But some other test dependencies definitely need updating now.

seancorfield23:06:55

I updated several of the JDBC drivers used for testing to move up to the Java 8 compatible versions and kicked off a full matrix build -- all passed!

seancorfield01:06:58

Do you think there will be an Alpha 6 soon with some sort of fix to the GeneratorAdapter issue?

seancorfield01:06:12

I'd love to test our stuff on 1.10...

seancorfield01:06:26

(I mean, on the new Java 8 / ASM stuff)

Alex Miller (Clojure team)01:06:07

Probably not for a couple weeks. Rich is on vacation and I’ll be in the woods with a bunch of scouts next week.

seancorfield02:06:38

'k, I may build a local fixed version for a bit of testing. I'm in England 7/5-7/17 so I won't get much done with it... but every little helps!

seancorfield02:06:32

OK, I'll take a look at that too.

Alex Miller (Clojure team)02:06:40

I’m still trying to figure out what exactly the bytecode bump means for contribs and builds

Alex Miller (Clojure team)02:06:08

most contribs don’t AOT or have java classes so most are unaffected

Alex Miller (Clojure team)02:06:42

the remainder are using a parent pom that compiles with 1.6 bytecode target (and depending which Clojure version they use, either 1.6 or 1.8)

Alex Miller (Clojure team)02:06:45

well all of them build using a clojure version < 1.10 so really they’re all 1.6 bytecode oriented, which maybe is a good place to stay for now

Alex Miller (Clojure team)02:06:02

I might release a new parent pom that targets 1.8 and then contribs can decide when to start using it

seancorfield02:06:07

With the compiler exception above, I've repro'd it with a smaller case inside Encore -- so it's not Nippy's problem.

seancorfield02:06:53

user=> (require '[taoensso.encore :as enc])
nil
user=> (def int-val 47)
#'user/int-val
user=> (let [a-val 100] (enc/case-eval a-val int-val "yes" "no"))
"no"
user=> (let [a-val (byte 100)] (enc/case-eval a-val int-val "yes" "no"))

CompilerException java.lang.IllegalArgumentException, compiling:(/private/var/folders/p1/30gnjddx6p193frh670pl8nh0000gn/T/form-init3995799245105199844.clj:1:1) 

seancorfield02:06:21

I'll see if I can narrow that further...

seancorfield02:06:01

Ah, yes

(case (byte 100) 47 "yes" "no")

seancorfield02:06:01

(! 803)-> clj -A:master
Downloading: org/clojure/clojure/1.10.0-master-SNAPSHOT/maven-metadata.xml from 
Clojure 1.10.0-master-SNAPSHOT
user=> (case (byte 100) 1 2 3)
CompilerException java.lang.IllegalArgumentException, compiling:(NO_SOURCE_PATH:1:1) 
OK, JIRA issue coming up for that.

Alex Miller (Clojure team)02:06:51

getting the reflector feels

seancorfield02:06:12

(I used my :master alias with clj which should be the same as 1.10.0-alpha5 right now)

seancorfield02:06:19

Just to be sure

(! 804)-> clj -Sdeps '{:deps {org.clojure/clojure {:mvn/version "1.10.0-alpha5"}}}'
Clojure 1.10.0-alpha5
user=> (case (byte 100) 1 2 3)
CompilerException java.lang.IllegalArgumentException, compiling:(NO_SOURCE_PATH:1:1) 

ghadi02:06:36

damn, that's bad

seancorfield02:06:57

Do I win a prize? 🙂

andy.fingerhut02:06:49

Looks like the cast() method in asm is being called with from=Type.BYTE and to=http://Type.INT. The older version of asm's cast() method was falling through doing nothing in that case. The newer version throws an exception.

andy.fingerhut02:06:48

The cast() method defined inside class GeneratorAdaptor, to be more precise.

seancorfield03:06:41

Since gen.cast(exprType, Type.INT_TYPE); used to be a no-op for INT, SHORT, BYTE and now it throws an exception for SHORT and BYTE.

seancorfield03:06:32

There only appear to be the only two calls to gen.cast() (lines 8812 and 8845)

seancorfield03:06:28

If line 8812 was if (exprType == Type.LONG_TYPE) gen.cast(exprType, Type.INT_TYPE); then the previous behavior would be restored.

seancorfield03:06:05

I'll add that to the ticket @alexmiller

Alex Miller (Clojure team)03:06:24

thx, will look at it when fresh

dpsutton03:06:28

I take it trying to get that copied dev updated is a no go?

Alex Miller (Clojure team)03:06:01

I can’t parse that sentence?

seancorfield03:06:09

Glad it's not just me 🙂

andy.fingerhut03:06:25

Are you asking whether it is reasonable to file a bug with asm lib and ask them to change it so that Clojure doesn't need to change?

seancorfield03:06:26

A one-line change to the Clojure compiler should fix it. Maybe I'll see if I can do the build/test thing locally and create a patch for this...

dpsutton03:06:52

yes. thanks andy

seancorfield03:06:06

OK, should have a patch on that ticket in a few minutes...

dpsutton03:06:42

interesting. the behavior changed in a commit with message "Reformat the source code with google-java-format 1.3" and now doesn't know how to convert a byte to an int. which i understand is a no-op? the previous behavior?

arrdem03:06:37

Huh. @dpsutton do you have the sha for it?

dpsutton03:06:17

wow gitlab is super slow

seancorfield03:06:07

Patch attached to CLJ-2367.

seancorfield03:06:04

When I ran the tests (via mvn package) I saw this failure

[java] FAIL in (gen-interface-source-file) (genclass.clj:151)
     [java] expected: (= "examples.clj" (str sourceFile))
     [java]   actual: (not (= "examples.clj" ""))
before and after my change...

ghadi04:06:03

what version of java did you build with @seancorfield?

seancorfield04:06:31

(! 833)-> java -version
java version "1.8.0_31"
Java(TM) SE Runtime Environment (build 1.8.0_31-b13)
Java HotSpot(TM) 64-Bit Server VM (build 25.31-b07, mixed mode)

razum2um08:06:02

Can someone clarify why a new dynamic classloader is created for every eval (https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L7037) ?

razum2um08:06:45

Right now I’m investigating why pomegranate doesn’t work as expected (add-dependencies) succeed and a classloader is modified, but following require (in repl) fails and found out that - pomegranate resolves via context class loader, but repl has quite different chain of dynamic classloaders (born from @clojure.lang.Compiler/LOADER. Its most common parent to context classloader - only sun.misc.Launcher$AppClassLoader.

razum2um08:06:53

Apparently, old versions of pomegranate modified AppClassLoader, but since java 9, update of dynapath it’s no more the case

razum2um08:06:27

I know, pomegranate has option to pass the classloader explicit and it works, but I find this cumbersome: (cemerick.pomegranate/add-dependencies ... :classloader (-> (fn []) class .getClassLoader .getParent))

bronsa08:06:12

@razum2um binding a new dynamic classloader per eval is critical for allowing redefinition

razum2um09:06:52

@bronsa ok, got it, but why when deriving a new one the Compiler.LOADER is preffered, not the Thread.currentThread().getContextClassLoader one? here https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/RT.java#L2172L2175

razum2um09:06:16

what should break if I switch the order?

bronsa09:06:34

if you do that you won’t get a proper classloader tree anymore and loading of AOT libraries might fail

bronsa09:06:21

DCL lives in a quite complex balance in order to support redefinition, JIT loading and AOT loading

bronsa09:06:56

clojure’s DCL breaks the classloader spec infact, as javadoc says >>When requested to find a class or resource, a ClassLoader instance will delegate the search for the class or resource to its parent class loader before attempting to find the class or resource itself

bronsa09:06:00

which clojure can’t do

razum2um09:06:18

Yep, exactly what I feel, so per java standard it should look parent-first, but am I correct, that DCL going to nest during code require and lookup vice-versa: child-first, right?

razum2um09:06:34

do you have an idea how to fix pomegranate then (it not modify AppClassLoader)? can we have a top DCL (common ancestor) which is always present and we’ll modify it instead?

Alex Miller (Clojure team)16:06:14

fyi, ran into the same thing while playing with add-lib for clj

Alex Miller (Clojure team)16:06:59

considering possible change in inserting a DCL in the repl env. not that this would help you at the moment.

dominicm16:06:18

Having the common ancestor would solve a feature request in cider-nrepl for hotloading deps too.

Alex Miller (Clojure team)18:06:25

yeah, I’ve chatted with Rich about this a bit.

16