Bugzilla – Full Text Bug Listing
| Summary: | JSR-199 EclipseCompiler: --release 8 with JAVA_HOME pointing to Java 11 JDK generates calls to Java 11 library API | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Thomas Wolf <twolf> | ||||
| Component: | Core | Assignee: | Jay Arthanareeswaran <jarthana> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | Alexander, jarthana, loskutov, piotr.zygielo, stephan.herrmann | ||||
| Version: | 4.21 | ||||||
| Target Milestone: | 4.21 M1 | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| See Also: |
https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/181952 https://bugs.eclipse.org/bugs/show_bug.cgi?id=574229 https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/182280 https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=25d339c3f5d58a95bc58915cac82a2b86d2f215f https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=d6ba5f54ae01cbba0db87564433b1a8a0c95ec16 https://bugs.eclipse.org/bugs/show_bug.cgi?id=574754 |
||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
Thomas, I'm not sure bug 571771 is related, please try ecj 3.26. (In reply to Andrey Loskutov from comment #1) > Thomas, I'm not sure bug 571771 is related, please try ecj 3.26. I had tried with ECJ 3.26.0.v20210603-0432 from pluginRepository https://repo.eclipse.org/content/repositories/eclipse-staging/ too. Same behavior. I'm not a maven expert, but I assume you can specify somehow -target 8 and - source 8? Doesn't this work? You could ask tycho folks what they did to get --release to work. (In reply to Andrey Loskutov from comment #3) > I'm not a maven expert, but I assume you can specify somehow -target 8 and - > source 8? Doesn't this work? If you look at the pom.xml of the small example project, it _does_ set -source and -target. With ECJ, however, one must not specify -source or -target if --release is given. plexus-compiler-eclipse does that correctly. The command options for ECJ shown when maven is run with the -X option look correct to me. (Note that plexus-compiler-eclipse uses the JSR-199 EclipseCompiler.) -source and -target are not equivalent to --release. Without the release option, both compilations would (should!) fail in the tests precisely because they'd reference a method that exists only in the Java 11 library. Avoiding that is the point of --release. Before --release, you'd use -bootclasspath for that, but that means you have to hardcode some JDK-internal paths into the pom. With Java 11, --release is the way to go. (In reply to Andrey Loskutov from comment #3) > You could ask tycho folks what they did to get --release to work. I did take a quick look at their commits and found only [1]. Apparently they didn't do anything special except passing the option to the compiler. [1] https://github.com/eclipse/tycho/commit/9556b00 (In reply to Thomas Wolf from comment #4) > (In reply to Andrey Loskutov from comment #3) > > You could ask tycho folks what they did to get --release to work. > > I did take a quick look at their commits and found only [1]. Apparently they > didn't do anything special except passing the option to the compiler. > > [1] https://github.com/eclipse/tycho/commit/9556b00 I don't know if you can somehow debug what maven/tycho does and compare arguments they pass to ecj? If tycho works with ecj and maven doesn't - why do you think the bug is in ecj and not in maven? (In reply to Andrey Loskutov from comment #5) > I don't know if you can somehow debug what maven/tycho does and compare > arguments they pass to ecj? If tycho works with ecj and maven doesn't - why > do you think the bug is in ecj and not in maven? As I said: running maven -X shows --release 8 being passed to ECJ. If ECJ then still generates code that references a Java-11-only method from the standard libraries, that's a bug in ECJ. I'm not 100% sure it's a bug. I even asked in comment 0 whether I was doing something wrong. (In reply to Thomas Wolf from comment #6) > As I said: running maven -X shows --release 8 being passed to ECJ. I assume this is not the only argument passed? So can you compare *all* arguments and order of arguments passed by tycho to ecj and by maven? There is no magic, same input to ecj must result in same output, at least in theory. If tycho gets right output from ecj and maven doesn't, it must be the input from maven that is wrong. I'm not sure if they run ecj "in process" or spawn a new jvm for compilation - and if that is the difference between maven / tycho? (In reply to Andrey Loskutov from comment #7) > If tycho gets right output from ecj and maven doesn't, it must be > the input from maven that is wrong. Unfortunately it's not that simple. ECJ has at least two entry points: * the BatchCompiler and its Main used inter alia in the IDE and by the ant adapter Gunnar's change fixed, and also by tycho, and * the JSR-199 EclipseCompiler which has its own handling of paths in EclipseCompilerImpl.handleLocations() and in EclipseFileManager. So it is entirely possible that the JSR-199 EclipseCompiler does something differently than the BatchCompiler. Wouldn't be the first time. plexus-compiler-eclipse uses the JSR-199 entry point. Both tycho-compiler-plugin and maven-compiler-plugin run the compiler in-process in my tests. (In reply to Thomas Wolf from comment #8) > (In reply to Andrey Loskutov from comment #7) > > If tycho gets right output from ecj and maven doesn't, it must be > > the input from maven that is wrong. > > Unfortunately it's not that simple. ECJ has at least two entry points: > > * the BatchCompiler and its Main used inter alia in the IDE and by the ant > adapter Gunnar's change fixed, and also by tycho, and > * the JSR-199 EclipseCompiler which has its own handling of paths in > EclipseCompilerImpl.handleLocations() and in EclipseFileManager. > > So it is entirely possible that the JSR-199 EclipseCompiler does something > differently than the BatchCompiler. Wouldn't be the first time. OK, now you have it. So bug title should probably say that EclipseCompilerImpl doesn't properly support --release. With that, next step would be to provide maven free test case that does what maven tries to do and fails to produce right bits. New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/181952 (In reply to Andrey Loskutov from comment #9) > next step > would be to provide maven free test case that does what maven tries to do > and fails to produce right bits. And then next step would be to fix it myself? Sorry, I don't know the compiler that well, and I can't invest the time needed to learn to know it that well. It's rather unlikely that I would be able to figure this one out. (In reply to Eclipse Genie from comment #10) > New Gerrit change created: > https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/181952 There you go. It _is_ a bug in EclipseCompiler, EclipseCompilerImpl, or in EclipseFileManager. At least locally that test fails; I hope it does so in the CI build, too. (In reply to Thomas Wolf from comment #11) > (In reply to Eclipse Genie from comment #10) > > New Gerrit change created: > > https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/181952 > > There you go. It _is_ a bug in EclipseCompiler, EclipseCompilerImpl, or in > EclipseFileManager. At least locally that test fails; I hope it does so in > the CI build, too. Yes, this new test fails in exactly the expected way. Over to the JDT experts. Thanks for the test case. I quickly debugged this to narrow down which code area might be to blame. I'm looking at this call stack: FileSystem.getJrtClasspath(String, String, AccessRuleSet, Map<String,String>) line: 274 Util.collectPlatformLibraries(File) line: 1169 Util.collectVMBootclasspath(List<Classpath>, File) line: 1138 EclipseCompilerImpl(Main).handleBootclasspath(ArrayList<String>, String) line: 3560 EclipseCompilerImpl.handleLocations() line: 704 Here EclipseCompilerImpl(Main) knows about releaseVersion="8", but FileSystem.getJrtClasspath does not have this information and hence ignores it. While Jay took responsibility for most work in this area, I would think that others could find a solution for this one, too. See that org.eclipse.jdt.internal.compiler.tool.EclipseCompilerImpl.handleLocations() in another branch directly dispatches to either FileSystem.getJrtClasspath() or FileSystem.getOlderSystemRelease(). It's the latter method that should be called, but never is. (In reply to Stephan Herrmann from comment #13) > I would think that others could find a solution for this one, too. Thanks for the pointer, Stephan. Surely someone else could figure out a solution, too. I don't feel comfortable with touching that code. I notice that with a ForwardingJavaFileManager the problem does not occur. A ForwardingJavaFileManager is a JavaFileManager, but not a StandardJavaFileManager. That means we enter the else-if at line EclipseCompilerImpl.handleLocations():552, which appears to do what you said should be done. An EclipseFileManager is a StandardJavaFileManager, but it returns null from getLocation(StandardLocation.PLATFORM_CLASS_PATH). So we enter the if-block at EclipseCompilerImpl.handleLocations():517 and then do nothing, skipping the else-if at EclipseCompilerImpl.handleLocations():551/552. The code you pointed out at EclipseCompilerImpl.handleLocations():704 may also need fixing, but the comment is about a special case on an IBM JVM, and it also seems to double as a last-resort attempt to get _some_ useable bootclasspath if none was found earlier. IMO something is already broken with the if on line 516/517, or with EclipseFileManager.getLocation(StandardLocation.PLATFORM_CLASS_PATH) returning null. This whole handling different types of file managers differently is beyond my very limited understanding. Is that some kind of optimization? Or is the code structured this way in an attempt to avoid picking up a partial bootclasspath on an IBM JVM and then not doing the last-resort case at line 704? I'd prefer it if somebody who has a clearer understanding of this area tried to fix it. I would only be stabbing around in the dark. Thanks for submitting the bug - I observed similar in https://www.eclipse.org/lists/jdt-dev/msg01748.html. (In reply to Piotr Żygieło from comment #15) > Thanks for submitting the bug - I observed similar in > https://www.eclipse.org/lists/jdt-dev/msg01748.html. You got there three months earlier. I wish you had already opened a bug report back then :-)! Your case works from the command line because that uses BatchCompiler, not the JSR-199 EclipseCompiler entry point. Maybe I should not have rewritten plexus-compiler-eclipse to use JSR-199. It looked like a good idea, and in theory it still is, but with hindsight it was probably not a smart move because while it solved some problems plexus-compiler-eclipse now uses an apparently rarely used and not sufficiently tested code path of ECJ. (In reply to Stephan Herrmann from comment #13) > FileSystem.getJrtClasspath(String, String, AccessRuleSet, > Map<String,String>) line: 274 > Util.collectPlatformLibraries(File) line: 1169 > Util.collectVMBootclasspath(List<Classpath>, File) line: 1138 > EclipseCompilerImpl(Main).handleBootclasspath(ArrayList<String>, String) > line: 3560 > EclipseCompilerImpl.handleLocations() line: 704 > > Here EclipseCompilerImpl(Main) knows about releaseVersion="8", but > FileSystem.getJrtClasspath does not have this information and hence ignores > it. > > While Jay took responsibility for most work in this area, I would think that > others could find a solution for this one, too. Thanks Stephan for investigating! I will take this up, probably next week or later during M1. (In reply to Thomas Wolf from comment #16) > Your case works from the command line because that uses BatchCompiler, not > the JSR-199 EclipseCompiler entry point. > > Maybe I should not have rewritten plexus-compiler-eclipse to use JSR-199. It > looked like a good idea, and in theory it still is, but with hindsight it > was probably not a smart move because while it solved some problems > plexus-compiler-eclipse now uses an apparently rarely used and not > sufficiently tested code path of ECJ. Is there a way to make it work with a pre-refactoring-to-JST-199 version of plexus-compiler-eclipse, using custom compiler arguments to set --release, simply being passed through to a recent version of the ECJ batch compiler? I am just asking out of curiosity and because it would be a nice way to compare the outcomes, both compiling with Maven. I was recently fixing the long neglected basic functionality of plexus-compiler-aspectj which never really worked with anything more recent than Java 1.2 targets, simply because it was not maintained for 15 years. The existing solution uses AJDT classes, not the ECJ-derived AspectJ batch compiler. AJDT is an abstraction, not unlike JSR-199. I was toying with the idea to actually make plexus-compiler-aspectj simply extend plexus-compiler-eclipse and add AspectJ-specific stuff. But that would also mean, we would immediately be bitten by this bug when compiling with '--release 8', too. (In reply to Alexander Kriegisch from comment #18) > Is there a way to make it work with a pre-refactoring-to-JST-199 version of > plexus-compiler-eclipse, using custom compiler arguments to set --release, > simply being passed through to a recent version of the ECJ batch compiler? plexus-compiler-eclipse 2.8.5 was the last version that used BatchCompiler, since 2.8.6 JSR-199 is used. Interestingly this appears to be a valid work-around. I thought it wouldn't work because it'll pass all of -source, -target, and --release to ECJ, but somehow it seems to work all the same. Work-around: use plexus-compiler 2.8.5, comment out <release> and use <compilerArgs> <arg>--release</arg> <arg>8</arg> </compilerArgs> instead. I tried with the example project from the attachment using ECJ 3.23.0, 3.25.0, and 3.26.0, and they all can run on Java 11 and generate correctly for Java 8. If I look at [1] (the INSIDE_RELEASE, INSIDE_SOURCE, and INSIDE_TARGET (at line 2684) cases), it seems to me that ECJ would balk on a command line passing arguments in the sequence --release, -source, -target, but will accept a command line -source, -target, --release. I wonder if that is intentional? [1] https://github.com/eclipse/eclipse.jdt.core/blob/4904c58/org.eclipse.jdt.core/batch/org/eclipse/jdt/internal/compiler/batch/Main.java#L2740 Just because I was curious about the JSR-199 compiler, never having used it before, I compiled your sample code with it from the command line and was surprised to find that '--release N' works. The test runs just fine: ------------------------------------------------------------------------ jdt-core-574181> java -cp ..\..\..\.m2\repository\org\eclipse\jdt\ecj\3.25.0\ecj-3.25.0.jar;..\..\..\.m2\repository\junit\junit\4.13.2\junit-4.13.2.jar org.eclipse.jdt.internal.compiler.tool.EclipseCompilerImpl --release 8 -d bin src/main/java/jdktest/Buffers.java src/test/java/jdktest/BuffersTest.java jdt-core-574181> "c:\Program Files\Java\jdk1.8.0_281\bin\java.exe" -cp bin;..\..\..\.m2\repository\junit\junit\4.13.2\junit-4.13.2.jar;..\..\..\.m2\repository\org\hamcrest\hamcrest-core\1.3\hamcrest-core-1.3.jar org.junit.runner.JUnitCore jdktest.BuffersTest JUnit version 4.13.2 . Time: 0,016 OK (1 test) ------------------------------------------------------------------------ Is this issue really about the JSR-199 compiler or maybe about a certain way in which it is used? New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/182280 (In reply to Alexander Kriegisch from comment #20) > Is this issue really about the JSR-199 compiler or maybe about a certain way > in which it is used? It's the latter. Platform libraries can come from two StandardLocation: PLATFORM_CLASS_PATH and SYSTEM_MODULES. The first one was getting the --release option properly but the second one wasn't. In my patch, I am passing the same system library that was used in PLATFORM_CLASS_PATH to SYSTEM_MODULES as well. (In reply to Eclipse Genie from comment #21) > New Gerrit change created: > https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/182280 In any case this change does _not_ fix the problem shown by the failing test case from https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/181952 . (In reply to Thomas Wolf from comment #23) > (In reply to Eclipse Genie from comment #21) > > New Gerrit change created: > > https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/182280 > > In any case this change does _not_ fix the problem shown by the failing test > case from https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/181952 . I see. Thanks for the tests. I was thrown off track by the specified stack track in the discussion. I see in this scenario we are passing an already constructed file manager. Will see what's going on. (In reply to Alexander Kriegisch from comment #20) > Just because I was curious about the JSR-199 compiler, never having used it > before, I compiled your sample code with it from the command line and was > surprised to find that '--release N' works. The test runs just fine: > > ------------------------------------------------------------------------ > > jdt-core-574181> java -cp > ..\..\..\.m2\repository\org\eclipse\jdt\ecj\3.25.0\ecj-3.25.0.jar;..\..\..\. > m2\repository\junit\junit\4.13.2\junit-4.13.2.jar > org.eclipse.jdt.internal.compiler.tool.EclipseCompilerImpl --release 8 -d > bin src/main/java/jdktest/Buffers.java src/test/java/jdktest/BuffersTest.java > > jdt-core-574181> "c:\Program Files\Java\jdk1.8.0_281\bin\java.exe" -cp > bin;..\..\..\.m2\repository\junit\junit\4.13.2\junit-4.13.2.jar;..\..\..\. > m2\repository\org\hamcrest\hamcrest-core\1.3\hamcrest-core-1.3.jar > org.junit.runner.JUnitCore jdktest.BuffersTest > JUnit version 4.13.2 > . > Time: 0,016 > > OK (1 test) > > ------------------------------------------------------------------------ > > Is this issue really about the JSR-199 compiler or maybe about a certain way > in which it is used? I would think this enters the compiler via Main.main(), not via the JSR-199 EclipseCompiler.getTask().call()? Given that the behavior is different depending on the kind of file manager used, and given that EclipseCompilerImpl.handleLocations() treats them differently, I think all these JSR-199 tests should be run at least twice: once with a plain JavaFileManager (e.g., a ForwardingJavaFileManager), and once with a StandardJavaFileManager (e.g., the EclipseFileManager returned by EclipseCompiler.getStandardFileManager()). >> jdt-core-574181> java -cp >> ..\..\..\.m2\repository\org\eclipse\jdt\ecj\3.25.0\ecj-3.25.0.jar; >> ..\..\..\.m2\repository\junit\junit\4.13.2\junit-4.13.2.jar >> org.eclipse.jdt.internal.compiler.tool.EclipseCompilerImpl >> --release 8 -d bin src/main/java/jdktest/Buffers.java >> src/test/java/jdktest/BuffersTest.java > I would think this enters the compiler via Main.main(), not via the JSR-199 > EclipseCompiler.getTask().call()? True. I actually did not check, but EclipseCompilerImpl.main is actually Main.main, because the former class extends the latter. I find this a somewhat strange implementation, also because the class name implies it would extend EclipseCompiler, which it does not. Unfortunate naming... (In reply to Alexander Kriegisch from comment #27) > True. I actually did not check, but EclipseCompilerImpl.main is actually > Main.main, because the former class extends the latter. I find this a > somewhat strange implementation, also because the class name implies it > would extend EclipseCompiler, which it does not. Unfortunate naming... The bulk of the code that processes the compiler arguments is inside Main.java and it is only logical that other compiler interfaces make use of this. EclipseCompiler is Eclipse's implementation of the JavaCompiler and EclipseCompilerImpl is sort of the bridge/translator between EclipseCompiler and Main. I can't comment on the naming, which was done long ago. Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/182280 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=25d339c3f5d58a95bc58915cac82a2b86d2f215f Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/181952 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=d6ba5f54ae01cbba0db87564433b1a8a0c95ec16 I also noticed we were always using the current JVM to get the version and fixed it now. Converted some of the tests (where possible) in CompilerToolJava9Tests to use both StandardJavaFileManager and ForwardingJavaFileManager. I understand this is the least tested part of the compiler and could use more tests around module locations (we have just about two dozens of tests). But without a owner for this area, this is the best we could do for the time being. All concerned, please test it out with the next I build. At least the small test project from attachment 286589 [details] now works. Works also for building JGit (running mvn and thus ECJ on Java 11, --release 8, and tests run Java 8) for a single-threaded build (mvn clean install -T 1). By default "mvn clean install" for JGit tells me it uses a multi-threaded build with 4 threads, and that fails quickly due to bug 574450. (In reply to Thomas Wolf from comment #32) > At least the small test project from attachment 286589 [details] now works. > > Works also for building JGit (running mvn and thus ECJ on Java 11, --release > 8, and tests run Java 8) for a single-threaded build (mvn clean install -T > 1). > > By default "mvn clean install" for JGit tells me it uses a multi-threaded > build with 4 threads, and that fails quickly due to bug 574450. Thanks Thomas for trying out. Bug 574450 seems like a tough nut to crack. (In reply to Jay Arthanareeswaran from comment #33) > Thanks Thomas for trying out. Bug 574450 seems like a tough nut to crack. I think bug 574450 is very similar to bug 563501. Unless I'm mistaken, it's not a general concurrency or threading problem but rather that an "old release" jar is inadvertently cached and thus inadvertently shared between multiple instances of EclipseCompilerImpl. |
Created attachment 286589 [details] Small test project I'm having troubles setting up a maven build such that it works properly if the system JDK is Java 11, but I compile against a Java 1.8 target and run the tests on a Java 1.8 JDK. This is quite possible with maven toolchains, and works nicely with javac, and a similar setup also works nicely in a tycho build using tycho-compiler-plugin. But with plain maven-compiler-plugin I cannot get it to work. Even through the --release 8 argument is passed to ECJ, it generates code that references the Java 11 ByteBuffer.flip() instead of the Java 8 Buffer.flip(). A small reproducer Eclipse maven project using ECJ 3.25.0 is attached. Works: $ mvn clean package -Pjavac Compiles using javac with --release 8, runs tests on a Java 1.8 JDK Fails: $ mvn clean package Compiles using ECJ with --release 8, runs tests on a Java 1.8 JDK In addition to the attached ZIP I use a $HOME/.m2/toolchains.xml with content <?xml version="1.0" encoding="UTF8"?> <toolchains> <toolchain> <type>jdk</type> <provides> <id>JavaSE-1.8</id> <version>1.8</version> </provides> <configuration> <jdkHome>/Library/Java/JavaVirtualMachines/jdk1.8.0_191.jdk/Contents/Home</jdkHome> </configuration> </toolchain> <toolchain> <type>jdk</type> <provides> <id>JavaSE-11</id> <version>11</version> </provides> <configuration> <jdkHome>/Library/Java/JavaVirtualMachines/adoptopenjdk-11.jdk/Contents/Home</jdkHome> </configuration> </toolchain> </toolchains> Am I doing something wrong? How can I set up a maven build that works when the system JDK is Java 11, but I need to build against a Java 8 target?