Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Full Java 8 support #246

Closed
cushon opened this issue Oct 31, 2014 · 26 comments
Closed

Full Java 8 support #246

cushon opened this issue Oct 31, 2014 · 26 comments

Comments

@cushon
Copy link
Collaborator

@cushon cushon commented Oct 31, 2014

Original issue created by eaftan@google.com on 2014-03-27 at 04:19 PM


Right now we have a build for javac 8 but are missing two important items needed for full support:

  1. Our Jenkins continuous build does not run Java 8 yet, only 6 and 7.

  2. We don't yet distribute the error-prone 8 jars to Sonatype. We probably want to distribute two sets of jars, one for versions 6 and 7, and one for 8. It would be nice if Maven could figure out which dependency to use based on the version of javac installed.

@cushon
Copy link
Collaborator Author

@cushon cushon commented Oct 31, 2014

Original comment posted by t.broyer on 2014-03-27 at 05:16 PM


Apparently, the error_prone module does change behavior depending on what's in the classpath (if the javac8 jar is there, I suspect the class construction to fail, falling back to javac7), so how about shading everything in a single jar?
If I made a wrong assumption, then how about making it true, loading the appropriate class depending on the current JVM version?

@cushon
Copy link
Collaborator Author

@cushon cushon commented Oct 31, 2014

Original comment posted by t.broyer on 2014-03-27 at 05:17 PM


Apparently, the error_prone module doesn't change behavior depending on what's in the classpath (if the javac8 jar is there, I suspect the class construction to fail, falling back to javac7), so how about shading everything in a single jar?
If I made a wrong assumption, then how about making it true, loading the appropriate class depending on the current JVM version?

@cushon
Copy link
Collaborator Author

@cushon cushon commented Oct 31, 2014

Original comment posted by eaftan@google.com on 2014-04-07 at 10:07 PM


Issue #240 has been merged into this issue.

@cushon
Copy link
Collaborator Author

@cushon cushon commented Oct 31, 2014

Original comment posted by eaftan@google.com on 2014-04-07 at 10:12 PM


Alex has added the Java 8 continuous build, so we need to investigate the Maven integration. Thanks Thomas for the pointer, we'll take a look.


Labels: Priority-High

@cushon
Copy link
Collaborator Author

@cushon cushon commented Oct 31, 2014

Original comment posted by michael....@aexp.com on 2014-07-30 at 05:54 PM


This maybe related to this Errorprone Gradle issue: tbroyer/gradle-errorprone-plugin#5

@cushon
Copy link
Collaborator Author

@cushon cushon commented Oct 31, 2014

Original comment posted by YogurtEarl on 2014-07-30 at 05:55 PM


It may be related to this Gradle Errorprone issue: tbroyer/gradle-errorprone-plugin#5

@cushon
Copy link
Collaborator Author

@cushon cushon commented Oct 31, 2014

Original comment posted by YogurtEarl on 2014-07-30 at 09:04 PM


Any ETA? :)

@cushon
Copy link
Collaborator Author

@cushon cushon commented Oct 31, 2014

Original comment posted by eaftan@google.com on 2014-08-22 at 05:44 PM


Issue #251 has been merged into this issue.

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Dec 21, 2014

AFAICT this has now been fixed with error-prone using its own javac rather than relying on the one from the executing JVM.

All we're waiting for now is a new release to Central. It's been a while since 1.1.2 was release, anything blocking 1.1.3?

@eaftan
Copy link
Contributor

@eaftan eaftan commented Dec 29, 2014

We have to rework the Maven plugin because of the switch to using our own bundled javac rather than the one installed on the user's system. We're aiming to get a release out at the end of January, once all of us are back from holiday travel. Sorry for the delay.

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Jan 5, 2015

But the maven plugin is another project that won't automatically update to 1.1.3 when it's released, right?

@TomDmitriev
Copy link

@TomDmitriev TomDmitriev commented Jan 5, 2015

I don't understand why a new version of error-prone must be released along with a new version of the maven plugin either. What about other build tools, particularly gradle (https://github.com/tbroyer/gradle-errorprone-plugin)? Is there any additionaly work required for it to support error-prone 1.1.3?

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Jan 5, 2015

@TomDmitriev Yes, there's some work needed: tbroyer/gradle-errorprone-plugin#6, but that's another story (also, we should avoid putting tools.jar in the classpath, or move it at the end so it doesn't conflict with the javac bundled into errorprone; this is probably what needs to be changed in the maven plugin too, the difference there being that the plugin is tied to a specific –though overridable– version of errorprone, contrary to the gradle plugin).

@eaftan It looks like the error_prone_core.jar (built locally) contains much more classes than necessary (junit? javax.tools?)

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Jan 5, 2015

@eaftan Actually, error_prone_core has to be in the bootclasspath or the classloader needs a custom loadClass when not running Java 8, as at some point com.sun.tools.javac.file.Locations (from error_prone_core) tries to access javax.tools.StandardLocation.NATIVE_HEADER_OUTPUT which only exists since Java 8 and will be loaded from the current JVM (default classloader priority). If the shaded dependencies could be moved into their own package, that would prevent such conflicts; not sure if it's feasible though.

@cushon
Copy link
Collaborator Author

@cushon cushon commented Jan 6, 2015

We'll update the maven plugin once 1.1.3 is out.

There are a couple of alternatives to putting javac on the bootclasspath or doing custom classloading, but they have disadvantages. Repackaging the dependencies under javax.tools.* prevents error-prone from working with code that invokes javac programmatically, and breaks annotation processors. We could patch javac8 to work with jdk7 (e.g. using reflection for the enum values added in 8), but that gets hacky and also causes problems for annotation processors.

@cushon
Copy link
Collaborator Author

@cushon cushon commented Jan 30, 2015

@eaftan pushed a new release (2.0) to maven, and I have fixes in progress for the maven and ant integrations:

@tbroyer - the issue with error_prone_core bundling all of its deps has been fixed: 4b323a3. The classloading hackery required to run javac8 on JDK7 is definitely annoying, but I don't have a better alternative yet.

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Jan 30, 2015

@cushon - Thanks, I made similar changes to the gradle-errorprone-plugin: tbroyer/gradle-errorprone-plugin@4cf8096 (I however went as far as trying to make the classloader "correct" wrt resources ;-) )

@cushon
Copy link
Collaborator Author

@cushon cushon commented Jan 30, 2015

@tbroyer - great, thanks. And nice catch re: resources. Was that just to be thorough, or does it fix a problem you saw? I didn't think the javac.jar/rt.jar overlap affected any resources.

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Jan 30, 2015

@cushon - just to be thorough.

Note: I have errors using the plugin in a multi module project. Haven't had time to investigate yet. I get unknown symbol errors when referencing classes from other modules in the project.

@cushon
Copy link
Collaborator Author

@cushon cushon commented Jan 30, 2015

If you have a repro, we can try to investigate. That sounds like it might require more knowledge of gradle / classloading than error-prone, though.

When javac is invoked programatically it can pick up classes from the 'ambient' classpath and add them to the compilation classpath. Maybe multi module builds were previously relying on that, and the new classloader broke them? If that's the problem, setting the compilation classpath explicitly should still work.

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Jan 31, 2015

OK, the problem is that in the first module, one class is not output: errorprone is passed the .java file as input (and other classes depend on it so it's correctly taken into account) but there's no .class file in the output directory. I'll try to create a repro case.

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Jan 31, 2015

And the problem is apparently only triggered by classes named Token (and probably others). Here's the smallest repro-case I could come up with: https://gist.github.com/tbroyer/b087c7151d6da4a774fc
At the end I'm expecting ls to list Token.class and FooToken.class, but there's only FooToken.class. Rename Token to AToken and you get the 2 files.

@cushon
Copy link
Collaborator Author

@cushon cushon commented Feb 2, 2015

Thanks for the nice repro. This was entirely an errorprone bug, should be fixed by 1f912d2. Let me know if that doesn't do it.

There's some duplication right now between the internal and open source integration, and we didn't catch this earlier because it's only present on the open source side. I'm working on removing some of that duplication (and cleaning up the error-prone/javac integration in general), which should make bugs like this less likely.

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Feb 2, 2015

👍 that fixes it! Waiting for that 2.0.1 release now ;-)

@cushon
Copy link
Collaborator Author

@cushon cushon commented Feb 3, 2015

I just pushed 2.0.1.

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented Feb 3, 2015

Great, thanks!

@cushon cushon closed this Feb 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants