Community
Participate
Working Groups
Build 3.3rc2. Due to APT(jsr269) enabling by default, the compiler will try to load 1.6 classes during its initialization process when attempting to compile in 1.6 compliant mode. Unless the compiler runs itself on a 1.6 VM, it will crash with the following exception: Exception in thread "main" java.lang.UnsupportedClassVersionError: Bad version number in .class file at java.lang.ClassLoader.defineClass1(Native Method) at java.lang.ClassLoader.defineClass(ClassLoader.java:620) at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:124) at java.net.URLClassLoader.defineClass(URLClassLoader.java:260) at java.net.URLClassLoader.access$100(URLClassLoader.java:56) at java.net.URLClassLoader$1.run(URLClassLoader.java:195) at java.security.AccessController.doPrivileged(Native Method) at java.net.URLClassLoader.findClass(URLClassLoader.java:188) at java.lang.ClassLoader.loadClass(ClassLoader.java:306) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:268) at java.lang.ClassLoader.loadClass(ClassLoader.java:251) at java.lang.ClassLoader.loadClassInternal(ClassLoader.java:319) at java.lang.Class.forName0(Native Method) at java.lang.Class.forName(Class.java:164) at org.eclipse.jdt.internal.compiler.batch.Main.initializeAnnotationProcessorManager(Main.java:3385) at org.eclipse.jdt.internal.compiler.batch.Main.performCompilation(Main.java:3332) at org.eclipse.jdt.internal.compiler.batch.Main.compile(Main.java:1545) at Compile.main(Compile.java:101) The net effect is that one cannot cross compile for 1.6 on a <1.6 JVM. Only workaround is to turn off APT by specifying "-proc:none" on command line.
The APT portion shouldn't be enabled unless a JRE1.6 is detected, since doomed.
Do we want to reject the case, silently ignore apt if the VM is not a 1.6 VM or report a warning?
We should compile with no APT on a lesser VM. Now logging a warning in the console seems nice to have (if problematic at this stage, I would make this a subsequent request. The problem now is the exception and the inability to run the compiler).
Created attachment 69722 [details] Proposed fix
Created attachment 69723 [details] Better patch The only change with the first patch is the error message reported to the user.
Risk: No risk. This will enhance the user experience for cross compilation without crashing if the VM is not a 1.6 VM. Except for annotations processing, a 1.6 VM is not required to target 1.6 .class files. Regarding the patch, if the VM version cannot be properly detected , the catch block is used to properly disable annotations. On all VMs I tried (IBM, Sun, Harmony, BEA), the catch block is never reached. Benefits: This looks like a regression from 3.2. Our compiler being used more and more by other projects (Tomcat, GCJ,...), we don't want to break users when they target 1.6 without using a 1.6 VM. So this low risk fix will fix this undesirable behavior. Philippe, Jérome, Maxime and Kent, please review
Patch looks good. Change is important to preserve cross-compile ability for 1.6 (regression from 3.2; due to enabling of jsr269 - mandated for java6 compliance).
A few details: - you adopted the 'key=message' convention for this new entry into messages.properties, while it seems that the 'key = message' (white space around the equal sign) convention prevails for the existing entries; - checkVMVersion doc should state that its parameter must be one of the ClassFileConstants.JDK* constants; these have a format different from, say, the one for the java.class.version property, hence it's not trivial to guess what format to use; - I do not quite see why checkVMVersion makes abstraction of the minor version number returned by the environment; if it's correct, a comment would help; - we may want to open a fup bug to avoid the logging of a warning when the -proc:none flag is provided (no need to report that the APT will be disabled when the user explicitly asked so anyway); - conversely, what happens if a user sets an explicit -processor option? - when no explicit -proc option is given, the effect of the patch looks very like 'moving the default to -proc:none'; the doc and the error message could make this more explicit; - the doc for annotation processing suffers a few spelling and syntax mistakes, but this is not in the scope of this bug; - I am puzzled by the fact that initializeAnnotationProcessorManager can silently fail, but another case of that behavior exists beside this patch, hence I'd consider that it is not new. As far as removing the exception is concerned, the patch will do it, and the concerned I have expressed can be addressed separately, hence I vote +1.
checkVMVersion(long) always take the same parameter (ClassFileConstants.JDK1_6), so it looks a simpler solution would have been to introduce getVMVersion() and compare it to ClassFileConstants.JDK1_6. But this is being picky. So +1 for 3.3RC4.
(In reply to comment #8) > A few details: > - you adopted the 'key=message' convention for this new entry into > messages.properties, while it seems that the 'key = message' (white space > around the equal sign) convention prevails for the existing entries; I can change that. > - checkVMVersion doc should state that its parameter must be one of the > ClassFileConstants.JDK* constants; these have a format different from, say, > the one for the java.class.version property, hence it's not trivial to guess > what format to use; I'll add it to the doc. > - I do not quite see why checkVMVersion makes abstraction of the minor version > number returned by the environment; if it's correct, a comment would help; The minor number seems to be irelevant for me. We have to make sure that the major number is supported. Also I never see a java compiler changing the minor number from 0. > - we may want to open a fup bug to avoid the logging of a warning when the > -proc:none flag is provided (no need to report that the APT will be disabled > when the user explicitly asked so anyway); if -proc:none is specified, the checkVMVersion method is not run. > - conversely, what happens if a user sets an explicit -processor option? I don't see what this would change. > - when no explicit -proc option is given, the effect of the patch looks very > like 'moving the default to -proc:none'; the doc and the error message could > make this more explicit; The message says that apt has been disabled. This is exactly what it does since according to the jsr269, apt needs to be enabled by default. > - the doc for annotation processing suffers a few spelling and syntax mistakes, > but this is not in the scope of this bug; Could you please open a bug report for this? Doc changes can still be done for RC4. > - I am puzzled by the fact that initializeAnnotationProcessorManager can > silently fail, but another case of that behavior exists beside this patch, > hence I'd consider that it is not new. Could you please detail what you mean? Thanks.
(In reply to comment #10) > (In reply to comment #8) > > A few details: ... > > - I do not quite see why checkVMVersion makes abstraction of the minor version > > number returned by the environment; if it's correct, a comment would help; > The minor number seems to be irelevant for me. We have to make sure that the > major number is supported. Also I never see a java compiler changing the minor > number from 0. So would you please making a comment in the source? > > > - we may want to open a fup bug to avoid the logging of a warning when the > > -proc:none flag is provided (no need to report that the APT will be disabled > > when the user explicitly asked so anyway); > if -proc:none is specified, the checkVMVersion method is not run. But according to your patch you will still log a warning (Main:3389 of the patched version)? > > - conversely, what happens if a user sets an explicit -processor option? > I don't see what this would change. I would suggest that we then raise an explicit error, since there is a contradiction between specifying processors and not using a suitable (>=6) runtime. > > - when no explicit -proc option is given, the effect of the patch looks very > > like 'moving the default to -proc:none'; the doc and the error message could > > make this more explicit; > The message says that apt has been disabled. This is exactly what it does since > according to the jsr269, apt needs to be enabled by default. More precisely, it could be worded in a way that explicitly says that it behaves as if '-proc:none' had been specified. I admit it's a matter of taste though. > > - the doc for annotation processing suffers a few spelling and syntax mistakes, > > but this is not in the scope of this bug; > Could you please open a bug report for this? Doc changes can still be done for > RC4. I'll do. > > - I am puzzled by the fact that initializeAnnotationProcessorManager can > > silently fail, but another case of that behavior exists beside this patch, > > hence I'd consider that it is not new. > Could you please detail what you mean? I mean that Main:3447 and Main:3456 of the patched source silently switch to no APT, with no echo to the caller. I do not quite like it, but, since line Main:3447 is not related to this bug/patch, I won't argue about the rationale behind that, and I'll 'excuse' the patch for doing something that the code already did in other circumstances. BTW, these remarks only reflect my not so humble opinion, and please feel absolutely free to ignore them.
(In reply to comment #11) > So would you please making a comment in the source? Yes, I can mention it. > But according to your patch you will still log a warning (Main:3389 of the > patched version)? If -proc:none is specified, this.compilerOptions.processAnnotations is false. So I don't see how I log a warning. I tried and I don't have a warning. > I would suggest that we then raise an explicit error, since there is a > contradiction between specifying processors and not using a suitable (>=6) > runtime. Sounds sufficient to do what we do now with the warnings. We might reconsider according to the feedback from the users.
(In reply to comment #12) ... > If -proc:none is specified, this.compilerOptions.processAnnotations is false. > So I don't see how I log a warning. I tried and I don't have a warning. Missed the correlation, apologies.
Created attachment 69990 [details] Released patch with doc changes Released for 3.3RC4. To verify it, please use the ecj.jar file from the download page.
Released for 3.3RC4.
Olivier please add 2 regression tests for this bug: one for the catch(UnsupportedClassVersionError) case, and one for the checkVMVersion(...) case.
Added regression test org.eclipse.jdt.core.tests.compiler.regression.BatchCompilerTest#test144 for the checkVMVersion call. I could not find a VM that would allow the catch block to be reachable. This is added in case the java.class.version property has a wrong format or an unknown value. Since it is not reached, it doesn't "cost" anything.
(In reply to comment #17) > Added regression test > org.eclipse.jdt.core.tests.compiler.regression.BatchCompilerTest#test144 for > the checkVMVersion call. test144 is not a regression test. If I revert the fix, the test doesn't fail with an UnsupportedClassVersionError.
(In reply to comment #18) > (In reply to comment #17) > > Added regression test > > org.eclipse.jdt.core.tests.compiler.regression.BatchCompilerTest#test144 for > > the checkVMVersion call. > test144 is not a regression test. If I revert the fix, the test doesn't fail > with an UnsupportedClassVersionError. > Please ignore, I didn't see your previous (sorry about that)
Verified for 3.3RC4 using I20070606-0010.
