usethesource / rascal

The implementation of the Rascal meta-programming language (including interpreter, type checker, parser generator, compiler and JVM based run-time system)
http://www.rascal-mpl.org
Other
396 stars 79 forks source link

The version of JDT used @ run time is dependent on the local Eclipse installation #1256

Closed tdegueul closed 2 months ago

tdegueul commented 4 years ago

With @lmove, we observed that the BasicM3Tests are currently failing when run from a Rascal console in Eclipse, but succeed when run from the command line (java -jar rascal.jar shell).

Diving deeper, I observed that the tests fail in a different way in newer (Eclipse 2019-06) and older (Eclipse Mars.2) versions of Eclipse. It turns out that the version of JDT used at run time by the EclipseJavaCompiler is the one that is currently installed in Eclipse. It seems that the JDT bundle currently installed in Eclipse takes precedence in the classpath builder of the Rascal console. The JDT packaged in rascal.jar is ignored.

Diagnosis

On the command line (java -jar rascal.jar)

rascal>import lang::rascal::tests::library::lang::java::m3::BasicM3Tests;
ok
rascal>printCP();
location=jar:file:/home/dig/repositories/rascal/target/rascal-0.13.0-SNAPSHOT.jar!/org/eclipse/jdt/core/dom/AST.class
ok
rascal>

In Eclipse 2019-06

rascal>import lang::rascal::tests::library::lang::java::m3::BasicM3Tests;
ok
rascal>printCP();
location=bundleresource://216.fwk1604125387/org/eclipse/jdt/core/dom/AST.class
ok
rascal>

Which bundle is bundleresource://216.fwk1604125387/?

OSGi Console> ss
[...]
216 ACTIVE      org.eclipse.jdt.core_3.18.0.v20190522-0428
                Fragments=214, 215
[...]

In Eclipse Mars.2 (2016)

rascal>import lang::rascal::tests::library::lang::java::m3::BasicM3Tests;
ok
rascal>printCP();
location=bundleresource://259.fwk1860754643/org/eclipse/jdt/core/dom/AST.class
ok
rascal>

Which bundle is bundleresource://259.fwk1860754643/?

OSGi Console²> ss
[...]
259 ACTIVE      org.eclipse.jdt.core_3.11.2.xx-201806261338-e45
                Fragments=258, 257
[...]
jurgenvinju commented 4 years ago

ouch. that's confusing. what behavior would you prefer?

jurgenvinju commented 4 years ago

it matter for which project you opened a console. If it was for the Rascal project or the Rascal-eclipse project, you will get different bundle resolvers than for example for a clean Rascal project. The difference is that Rascal might depend on the JDT bundle somehow via the MANIFEST.MF file. Could you find out what your builtin prints between these two different instances?

tdegueul commented 4 years ago

Picking whichever JDT version is currently installed in Eclipse (current behavior) implies that the behavior of Rascal programs changes according to where/who runs them. It should be fixed, i.e. we should always pick the one packaged at build time in rascal.jar.

tdegueul commented 4 years ago

It doesn't matter which project I run the Rascal console on (rascal, rascal-eclipse, some-rascal-project, or no project at all): it resolves the JDT bundle in all cases.

tdegueul commented 4 years ago

Actually this is more of a rascal-eclipse issue. The issue is probably around those lines: the resulting classpath mixes rascal.jar with the bundles of the current Eclipse installation; there should be a way to prioritize rascal.jar in the resolution. Wdyt?

I'll have a look there and move the issue to rascal-eclipse when necessary.

DavyLandman commented 4 years ago

Great detective work!

Regardless on how the class path is build (depending on which context) we should not let a jar dependency be replaced by a bundle dependency.

We literally have the classes for jdt packaged inside our jar. So I'm quite confused how we even get the classloader to pick the classes from a bundle.

here should be a way to prioritize rascal.jar in the resolution. Wdyt?

So yes, looks like a plan.

jurgenvinju commented 4 years ago

i.e. we should always pick the one packaged at build time in rascal.jar

There is one exception to that, and that is when we are developing the JDT m3 extractors themselves, then we should use the compile-time dependend version, so that we test the code that we release.

jurgenvinju commented 4 years ago

here should be a way to prioritize rascal.jar in the resolution. Wdyt? So yes, looks like a plan.

Agreed.

The classloading for builtin functions is quite a complex manner.

jurgenvinju commented 4 years ago

Finding the rascal.jar and putting it first on the classloading path is not some simple thing:

So, to make that rascal.jar be the one to resolve JDT classes, we basically have to turn that Bundle inside out and load a specialized classloader in front inside the multiplexer.

jurgenvinju commented 4 years ago

Alternative solutions:

jurgenvinju commented 4 years ago

I think the first idea (hack the classloader) would be most brittle, but most doable in the short-term. The other two solutions are major efforts.

jurgenvinju commented 4 years ago

The plan is to extract the JDT specific code into a new package/project "java-air" after the compiler is finished. At that time we could plan to resolve the JDT bundle dependencies again, and remove the temporary hack/solution in the rascal-eclipse configuration of the classloader multiplexer.

tdegueul commented 4 years ago

Diving a little deeper, it turns out that rascal-eclipse itself declares an unversioned MANIFEST.MF dependency towards org.eclipse.jdt (it needs it @ run time to use the JavaCore utilities and parse the configuration of Eclipse Java projects) and that all dependencies of rascal-eclipse are dynamically registered in the multiplexer. This is where the wrong JDT version comes from.

Dedicating a classloader to rascal.jar and putting it in front of all other classloaders sounds alright to me; i.e. your first solution.

Btw, could you please move the issue to https://github.com/usethesource/rascal-eclipse where it belongs? I don't have the rights.

DavyLandman commented 4 years ago

make equinox the default execution platform for Rascal, also for the commandline version

We have had this in the past, and weren't happy with it, now rascal is pure maven, and thus makes us less coupled to osgi&eclipse. (As far as I know, osgi is used for eclipse and for big enterprise systems).

do not package JDT inside rascal.jar anymore at all; factor all Rascal-JDT code out into a separate package, which re-packages JDT directly in its own flat jar with the Rascal-specific code. The builtin Java methods which activate the JDT will find the AST classes locally (whatever classloader is used) and that should solve the issue once and for all.

  1. We want to run jdt m3 code outside of eclipse
  2. I have to say I'm amazed at the stability of the API, we compile against an 3y old version of jdt, and we have binary compatibility with the latest jdt inside eclipse. But I do not think this is a wide gamble to continue.

I think the first idea (hack the classloader) would be most brittle, but most doable in the short-term.

Is it brittle, yes, but I could be wrong, but isn't it the only correct one?

If you want a drastic solution: run rascal in its own separate process and communicate via a tcp protocol. Aka, LSP ;)

jurgenvinju commented 4 years ago

Is it brittle, yes, but I could be wrong, but isn't it the only correct one?

The solution where we factor out the "java-air" part and merge the JDT into that jar (by merging jdt-dependencies-repackaged into that new project) is the only "correct" solution IMHO. That way we have the same dependencies at build-time and after deployment, and we do not depend on classloader's complex/implicit semantics for the AST classes of JDT.

The current most practical solution is brittle because it depends on how Equinox handles embedded jars, the way we wire the classloader multiplexer, and the way it is configured inside of Rascal-eclipse. But it should work...

jurgenvinju commented 4 years ago

Also the "most practical" solution has an impact on all other Rascal projects; if we run into a project which does not want our version of Jetty but another one, it will basically be impossible to fix without breaking the current issue again. That goes for all of Rascal's "fundamental" dependencies and currently those are quite a lot.

jurgenvinju commented 4 years ago

for reference, these are the jars we will force everybody that uses Rascal to depend on for their own Rascal projects:

[INFO]    com.github.jnr:jnr-unixsocket:jar:0.17:provided
[INFO]    org.apache.lucene:lucene-join:jar:7.5.0:compile
[INFO]    org.yaml:snakeyaml:jar:1.14:compile
[INFO]    org.jruby:jruby-core:jar:9.1.12.0:provided
[INFO]    io.usethesource:vallang:jar:0.11.0-SNAPSHOT:compile
[INFO]    com.beust:jcommander:jar:1.72:provided
[INFO]    org.apache.commons:commons-math:jar:2.2:compile
[INFO]    com.google.code.gson:gson:jar:2.3.1:compile
[INFO]    com.jcraft:jzlib:jar:1.1.3:provided
[INFO]    io.usethesource:capsule:jar:0.6.3:compile
[INFO]    org.ow2.asm:asm-all:jar:5.0.4:compile
[INFO]    org.hamcrest:hamcrest-core:jar:1.3:compile
[INFO]    org.rascalmpl:rascal-p2-dependencies-repackaged:jar:0.4.0:compile
[INFO]    org.jruby:jruby-complete:jar:9.1.15.0:provided
[INFO]    org.jruby:jruby:jar:9.1.12.0:provided
[INFO]    org.nanohttpd:nanohttpd:jar:2.3.1:compile
[INFO]    org.jruby:dirgra:jar:0.3:provided
[INFO]    org.apache.lucene:lucene-sandbox:jar:7.5.0:compile
[INFO]    com.github.ben-manes.caffeine:caffeine:jar:2.7.0:compile
[INFO]    org.apache.lucene:lucene-core:jar:7.5.0:compile
[INFO]    com.github.jnr:jffi:jar:native:1.2.16:provided
[INFO]    org.jruby:jruby-stdlib:jar:9.1.12.0:provided
[INFO]    org.jdom:jdom2:jar:2.0.6:compile
[INFO]    org.apache.lucene:lucene-highlighter:jar:7.5.0:compile
[INFO]    com.github.jnr:jffi:jar:1.2.16:provided
[INFO]    com.github.jnr:jnr-constants:jar:0.9.9:provided
[INFO]    org.apache.commons:commons-compress:jar:1.18:compile
[INFO]    com.github.luben:zstd-jni:jar:1.4.0-1:compile
[INFO]    commons-lang:commons-lang:jar:2.6:compile
[INFO]    com.ibm.icu:icu4j:jar:58.1:compile
[INFO]    com.github.jnr:jnr-enxio:jar:0.16:provided
[INFO]    com.headius:unsafe-fences:jar:1.0:provided
[INFO]    com.github.jnr:jnr-posix:jar:3.0.41:provided
[INFO]    jline:jline:jar:2.14.4:compile
[INFO]    org.jruby.joni:joni:jar:2.1.11:provided
[INFO]    org.checkerframework:checker-qual:jar:2.9.0:compile
[INFO]    com.headius:options:jar:1.4:provided
[INFO]    com.google.errorprone:error_prone_annotations:jar:2.3.3:compile
[INFO]    com.martiansoftware:nailgun-server:jar:0.9.1:provided
[INFO]    junit:junit:jar:4.12:compile
[INFO]    org.apache.lucene:lucene-analyzers-common:jar:7.5.0:compile
[INFO]    org.tukaani:xz:jar:1.8:compile
[INFO]    org.jruby.extras:bytelist:jar:1.0.15:provided
[INFO]    org.jruby.jcodings:jcodings:jar:1.0.18:provided
[INFO]    com.github.jnr:jnr-netdb:jar:1.1.6:provided
[INFO]    org.apache.lucene:lucene-memory:jar:7.5.0:compile
[INFO]    org.asciidoctor:asciidoctorj:jar:1.6.0-alpha.6:provided
[INFO]    com.github.jnr:jnr-x86asm:jar:1.0.2:provided
[INFO]    joda-time:joda-time:jar:2.8.2:provided
[INFO]    com.headius:invokebinder:jar:1.7:provided
[INFO]    org.apache.lucene:lucene-queryparser:jar:7.5.0:compile
[INFO]    org.apache.lucene:lucene-queries:jar:7.5.0:compile
jurgenvinju commented 4 years ago

So, I think the temporary solution is ok for now, but in the long run it will come back to haunt us.

jurgenvinju commented 4 years ago

Btw, could you please move the issue to https://github.com/usethesource/rascal-eclipse where it belongs? I don't have the rights.

rascal-eclipse does not have issues configured on github. We were loosing track of all the issues so we kept them all here. Since we can close issues by referring to other github projects, it's ok.

DavyLandman commented 4 years ago

for reference, these are the jars we will force everybody that uses Rascal to depend on for their own Rascal projects:

True, but using shading (which we already do) we can solve this by moving all packages into a nested packages. So everything gets a org.rascalmpl.dependency prefix, I've seen this done to other big projects to avoid this. It would also solve our JDT problem in a way 😈... But this does require quite some shading config.

jurgenvinju commented 4 years ago

It would also solve our JDT problem in a way

Please don't go there. We do not know how much reflection the JDT people have used or will use to keep their APIs backward compatible.

jurgenvinju commented 4 years ago

So everything gets a org.rascalmpl.dependency prefix,

Certainly something to consider for our "simple" dependencies that end up being in the core of the Rascal run-time. But let's not do this for the JDT. It's way way to complex, and we don't want it in the core of Rascal anyway. It's our most-used and most successful library, but it's not a core Rascal feature.

DavyLandman commented 4 years ago

But let's not do this for the JDT. It's way way to complex

You are right, for jdt, way to dangerous (I did use the 😈). But for other stuff, something to consider in the future.

and we don't want it in the core of Rascal anyway. It's our most-used and most successful library, but it's not a core Rascal feature.

I understand, but that won't solve this problem? It would still be maven dependency that is fullfilled by a osgi bundle. I think that is the core issue (maybe I'm wrong), and Thomas had an idea on how to make that more predictable.

jurgenvinju commented 4 years ago
  1. Thomas' idea (to fix and co-evolve the JDT version number) will not work because Eclipse can not deal with loading JDT twice with different versions, even though they are building on top of OSGI. It would only work in a clean OSGI setting, but pre-OSGI JDT features are still leaking into the Eclipse environment after all this time.
  2. For the factored-library solution it will solve the current problem by making the JDT AST classes local to the same URLClassLoader as the builtin functions (i.e. extractM3FromDirectory). Every class loader is guaranteed to first load classes from its own instance, before it defaults to its parent. So if we package the AST classes in the same jar as our ASTConverter and JarConverter classes, then I'm inclined to believe that we'd load the classes always from that java-air.jar.
  3. However, since we've never tried running the AST classes from rascal.jar in the Eclipse context, there could still be the same kind of mess which is also causing Eclipse not to allow different instances of the JDT module to load in Equinox correctly. It's not expected, but let's not assume it just works implicitly ;-)
jurgenvinju commented 4 years ago

Things that might go wrong when we start running "another JDT" inside Eclipse:

jurgenvinju commented 4 years ago

Unless somebody fixed all these issues in the meantime and it's now possible to run Eclipse loading two instances of the JDT in two different modules.. This knowledge is pretty old..

jurgenvinju commented 1 year ago

Given that we use JDT now more in VScode than in Eclipse, I think it's time to park this issue. We will also factor out rascal-java into a separate project from the standard library, in another issue.