A custom version of com.sun.tools.javac.api.JavacTool.

Review Request #4122 — Created July 25, 2016 and submitted

benjyw
pants
3722
pants-reviews
stuhood, zundel

Zinc expects to use a class of that name as its java compiler.

If it fails to classload it directly, it'll use the one embedded
in the tools.jar of the JDK it's running on.

Therefore if we want to use some custom version of javac other
than the one in the JDK (say we want to run javac 9 on JDK 8),
then we only need to put that other javac on the classpath.

A future change will implement this, but to test that change
we need some alternative JavacTool that makes it easy to know
that it was invoked. This change introduces such a dummy JavacTool.

Those future tests can verify that the custom JavacTool was indeed
invoked by checking that the compilation attempt fails with
the error message "Pants caused Zinc to load a custom JavacTool".

The reason this is in a separate change than the tests that will
use it is so we can publish it from master, allowing the tests to
depend on it as a published tool.

Verified that this compiles, and can be used in the manner described.

CI passes: https://travis-ci.org/pantsbuild/pants/builds/147276262

BE
ZU
  1. 
      
  2. I'm not an expert, but what did you copy? did you just implement an existing interface?

    1. The only thing I copied is the fully qualified name of the class. Other than that, yes, it just implements an existing interface.

    2. I just don't see how that can be copyrightable.

  3. 
      
ST
  1. Please link the relevant github issue to this one.

    In the medium term, I'd prefer to fix our incorrect assumption about how zinc loads javac to actually use the javax.tools.JavaCompiler interface directly (if possible?). Ity and I are planning to contribute some patches upstream soon to address https://github.com/sbt/zinc/issues/144 , which will touch the code that loads JavaCompiler. If you have a suggestion about how to make the loading more generic, they'd be welcome on that issue.

    1. Changing how Zinc loads javac is necessary in order to, say, use error prone, or other compiler wrappers. It would be pretty simple to add a zinc flag that specifies a fully-qualified class name of some JavaCompiler implementation to use instead of the standard one.

      But to use some other version of plain vanilla javac you have to load it by that standard name, and so to test it you have to a class of that name. That won't change no matter what changes we make to zinc.

      Are we no longer using our in-repo zinc fork? What's the status on that?

    2. Are we no longer using our in-repo zinc fork? What's the status on that?

      zinc the daemon (as forked to pantsbuild) is no longer maintained. Confusingly, sbt/zinc is the new name for what use to be sbt's incremental compiler library. It's now slightly easier to contribute to, as it is intentionally meant for consumption by multiple parties.

      https://rbcommons.com/s/twitter/r/3658/ is still open to update our zinc wrapper to use the new incremental compiler library, but it is currently blocked by https://github.com/sbt/zinc/issues/144

  2. src/java/com/sun/tools/javac/api/BUILD (Diff revision 2)
     
     
     

    Maybe place it in a org.pantsbuild.testprojects org, and shorten the artifact name?

    1. I wasn't sure where this should go. But as far as I can tell testprojects are code that we test JVM compilation on. This, on the other hand, is an actual tool that we need to publish to a public maven repo.

      In other words, if the tests are surgeons then testprojects are patients and this is a scalpel. So I'm not sure testprojects is right.

    2. It is a project used only for tests. If we're providing a feature to load code like this, then this is standing in as a proxy for user code, similar to how a javac_plugin "is a tool" but is also something we allow users to replace. Not a strong objection though.

    3. juat to clarify, this isn't a proxy for user code, it's a proxy for another version of javac (e.g., javac 9).

  3. 
      
ZU
  1. Ship It!
  2. 
      
BE
BE
BE
Review request changed

Status: Closed (submitted)

Change Summary:

edf0fe5fac8c6204266e66edcf12456edcb0cabb

BE
  1. Thanks for the reviews!

  2. 
      
Loading...