rascal icon indicating copy to clipboard operation
rascal copied to clipboard

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

Open tdegueul opened this issue 5 years ago • 27 comments

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

  • Insert a @javaClass{...} public void java printCP(); in BasicM3Tests.rsc where:
public void printCP(IEvaluatorContext ctx) {
    Class cls = org.eclipse.jdt.core.dom.AST.class;
    URL location = cls.getResource("/" + cls.getName().replace(".", "/") + ".class");
    ctx.getStdOut().println("location=" + location);
}
  • Build (mvn clean package) a new rascal.jar
  • Import rascal-eclipse into Eclipse, change the rascal.jar to the one build above, and run a new Eclipse instance with the new rascal-eclipse
  • See what happens in the Rascal console

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
[...]

tdegueul avatar Sep 09 '19 13:09 tdegueul

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

jurgenvinju avatar Sep 09 '19 13:09 jurgenvinju

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?

jurgenvinju avatar Sep 09 '19 13:09 jurgenvinju

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 avatar Sep 09 '19 13:09 tdegueul

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 avatar Sep 09 '19 14:09 tdegueul

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.

tdegueul avatar Sep 09 '19 14:09 tdegueul

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.

DavyLandman avatar Sep 09 '19 15:09 DavyLandman

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 avatar Sep 10 '19 06:09 jurgenvinju

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.

  • We have our own ClassLoader multiplexer which provides itself as parent to the classloaders it receives at configuration time.
  • The order in which classes are loaded depends on the order they are registered with the multi-plexer, but also on the order in which classes are dependend on each other and at which time they are (lazily) loaded by different classloader mechanisms.
  • Classes are always loaded first from their local class loader, and then if not found they bubble up to the parent classloader, until they finally arrive at our multiplexer. It then starts to dispatch in order of configuration, and loads the requested class via the first classloader that has the class. Further dependend classes are then loaded by the classloader that was last found, until that fails again and we bubble up to the parent again.
  • Due to the modularity features of OSGI, there may be several instances of the same class present in memory at any given time, so while debugging the multiplexer you might see the class from rascal.jar loaded, but still the builtin will use the class from the eclipse-jdt.jar plugin

jurgenvinju avatar Sep 10 '19 06:09 jurgenvinju

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

  • it is an embedded jar of the rascal-eclipse plugin
  • the Rascal eclipse plugin is an OSGI Equinox Bundle
  • one of the contributors to that Bundle is the embedded jar

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 avatar Sep 10 '19 06:09 jurgenvinju

Alternative solutions:

  • make equinox the default execution platform for Rascal, also for the commandline version. Then it becomes 100% transparent and we don't have to re-package JDT. (this has enormous consequences otherwise, and I would consider it a "last resort")
  • 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.
  • and the first idea: hack so that rascal.jar gets its own class loader by digging it out the the rascal-eclipse bundle and configuring it before the others in the multi-plexer.

jurgenvinju avatar Sep 10 '19 07:09 jurgenvinju

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 avatar Sep 10 '19 07:09 jurgenvinju

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.

jurgenvinju avatar Sep 10 '19 07:09 jurgenvinju

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.

tdegueul avatar Sep 10 '19 07:09 tdegueul

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 ;)

DavyLandman avatar Sep 10 '19 07:09 DavyLandman

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 avatar Sep 10 '19 07:09 jurgenvinju

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 avatar Sep 10 '19 07:09 jurgenvinju

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 avatar Sep 10 '19 07:09 jurgenvinju

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

jurgenvinju avatar Sep 10 '19 07:09 jurgenvinju

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.

jurgenvinju avatar Sep 10 '19 07:09 jurgenvinju

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.

DavyLandman avatar Sep 10 '19 07:09 DavyLandman

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 avatar Sep 10 '19 07:09 jurgenvinju

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.

jurgenvinju avatar Sep 10 '19 07:09 jurgenvinju

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.

DavyLandman avatar Sep 10 '19 07:09 DavyLandman

  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 avatar Sep 10 '19 11:09 jurgenvinju

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

  • races on on-disk caches of code indexes which are (perhaps) constructed as side-effects of the compilation process
  • races on disk caused by not sharing the Eclipse core instance classes, including the resources. So the JDTs locking mechanisms are not working between our instance of the JDT and the installed instance of the JDT plugin.
  • home-grown dependency injection/extension point mechanisms for the JDT based on file resources via IResource instead of via Java resources (and other behavior coded using Java reflection)
  • Murphy will tell you about other stuff ;-)

jurgenvinju avatar Sep 10 '19 11:09 jurgenvinju

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 avatar Sep 10 '19 11:09 jurgenvinju

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.

jurgenvinju avatar Apr 19 '23 09:04 jurgenvinju