Completely revamp how we support JVM compiler plugins. #4287

Merged
merged 5 commits into from Mar 3, 2017

Conversation

Projects
None yet
3 participants
@benjyw
Contributor

benjyw commented Feb 25, 2017

Problem

Previously we had two separate, ad-hoc solutions for javac and scalac plugins, each with different quirks and features.

Solution

This change re-implements plugin support, so that as much as possible of the implementation is shared for javac and scalac, and so that the features and usages are identical for both.

  • The biggest new feature is the ability to specify plugins and their args on specific targets, as well as globally, for all targets.
  • This change streamlines the way in which the ScalaJSZincCompile task injects the scala.js plugin. Previously there was a rather complicated dance that, among other problems, prevented you from using any other plugins alongside the scala.js plugin. Now the scala.js plugin is merely added to the list of required plugins, and processed uniformly with them.
  • We also deprecate the --compiler-plugin-deps option on the Java subsystem. It was only there so that plugin deps could be added to the traversable_dependency_specs of JvmTargets, thus hackily supporting globally-required in-repo plugins. But that ended up causing all sorts of problems, and wasn't worth the trouble. Now if you want to use an in-repo plugin when compiling some target you have to depend on it explicitly (you can still globally depend on external, published plugins).

This change also adds two comprehensive, basically-identical tests, one for javac and one for scalac, that poke at all the features.

@benjyw benjyw requested review from stuhood, baroquebobcat and kwlzn Feb 26, 2017

@stuhood

stuhood approved these changes Feb 27, 2017 edited

Amazing work. Thanks Benjy! Doing a bit of memoization for plugin loading is probably the only blocker.

@@ -90,6 +100,10 @@ def __init__(self,
'strict_deps': PrimitiveField(strict_deps),
'fatal_warnings': PrimitiveField(fatal_warnings),
'zinc_file_manager': PrimitiveField(zinc_file_manager),
+ 'javac_plugins': SetOfPrimitivesField(javac_plugins),

This comment has been minimized.

@stuhood

stuhood Feb 27, 2017

Member

I could imagine plugin ordering mattering in some cases, so rather than a set, this should probably be a list. And if it's a list, you can use PrimitiveField directly I think.

@stuhood

stuhood Feb 27, 2017

Member

I could imagine plugin ordering mattering in some cases, so rather than a set, this should probably be a list. And if it's a list, you can use PrimitiveField directly I think.

This comment has been minimized.

@benjyw

benjyw Feb 28, 2017

Contributor

Well, order cannot matter in javac (the plugin options go into an unordered map, so the order is lost almost immediately). They could hypothetically matter in scalac (the plugins are loaded in cmd-line order, as far as I can tell), but they don't in practice - the loading order doesn't affect when each plugin runs - that happens at whatever phase the plugin asked to be run in.

So now it's down to whether we want to err on the side of hypothetical caution, at the expense of triggering spurious invalidation. I just try to avoid spurious invalidation whenever possible.

We can always change this in the future, should we discover that order matters. That would be a backwards-compatible change, while the reverse would not.

Thoughts?

@benjyw

benjyw Feb 28, 2017

Contributor

Well, order cannot matter in javac (the plugin options go into an unordered map, so the order is lost almost immediately). They could hypothetically matter in scalac (the plugins are loaded in cmd-line order, as far as I can tell), but they don't in practice - the loading order doesn't affect when each plugin runs - that happens at whatever phase the plugin asked to be run in.

So now it's down to whether we want to err on the side of hypothetical caution, at the expense of triggering spurious invalidation. I just try to avoid spurious invalidation whenever possible.

We can always change this in the future, should we discover that order matters. That would be a backwards-compatible change, while the reverse would not.

Thoughts?

This comment has been minimized.

@stuhood

stuhood Feb 28, 2017

Member

Sounds reasonable to leave it as is. Thanks!

@stuhood

stuhood Feb 28, 2017

Member

Sounds reasonable to leave it as is. Thanks!

+ def _get_plugin_map(self, compiler, target):
+ """Returns a map of plugin to args, for the given compiler.
+
+ Only plugins that must actually be activated will be present as keys in the map.

This comment has been minimized.

@stuhood

stuhood Feb 27, 2017

Member

These two paragraphs are pretty rich in information. Would be great as a blurb on the docsite (and could then reference the document here).

@stuhood

stuhood Feb 27, 2017

Member

These two paragraphs are pretty rich in information. Would be great as a blurb on the docsite (and could then reference the document here).

This comment has been minimized.

@benjyw

benjyw Feb 28, 2017

Contributor

Good idea, will do shortly.

@benjyw

benjyw Feb 28, 2017

Contributor

Good idea, will do shortly.

+ :param compiler: one of 'javac', 'scalac'.
+ :param target: The target whose plugins we compute.
+ """
+ # Note that we get() options and getattr() target fields and task methods,

This comment has been minimized.

@stuhood

stuhood Feb 27, 2017

Member

Would this be cleaner as a trait/interface? I know target inheritance is potentially in the sights, but going pure-reflection for this broad a case is a bit scary.

@stuhood

stuhood Feb 27, 2017

Member

Would this be cleaner as a trait/interface? I know target inheritance is potentially in the sights, but going pure-reflection for this broad a case is a bit scary.

This comment has been minimized.

@benjyw

benjyw Feb 28, 2017

Contributor

The trouble is, ScalaJSTarget and its subclasses don't extend JvmTarget (I don't know enough about scala.js to say if it should), so they don't have these attributes.

In the case of task fields I could add those fields to the base class, but I figured it might be better to be uniform here in our use of reflection. In for a penny, in for a pound...

Thoughts? If ScalaJSTarget were a JvmTarget (or had the plugin fields some other way, e.g., mixin or copypasta) then the need for reflection would go away. And maybe that's a good idea anyway? Scala.js explicitly allows other plugins to run alongside it.

@benjyw

benjyw Feb 28, 2017

Contributor

The trouble is, ScalaJSTarget and its subclasses don't extend JvmTarget (I don't know enough about scala.js to say if it should), so they don't have these attributes.

In the case of task fields I could add those fields to the base class, but I figured it might be better to be uniform here in our use of reflection. In for a penny, in for a pound...

Thoughts? If ScalaJSTarget were a JvmTarget (or had the plugin fields some other way, e.g., mixin or copypasta) then the need for reflection would go away. And maybe that's a good idea anyway? Scala.js explicitly allows other plugins to run alongside it.

This comment has been minimized.

@benjyw

benjyw Feb 28, 2017

Contributor

Oh, but that would only be the case for scalac plugins, it would still make no sense for the ScalaJS tasks or targets to reference javac plugins. But I suppose it could, and then throw an exception, like we do for java_library if it has a scalac plugin.

@benjyw

benjyw Feb 28, 2017

Contributor

Oh, but that would only be the case for scalac plugins, it would still make no sense for the ScalaJS tasks or targets to reference javac plugins. But I suppose it could, and then throw an exception, like we do for java_library if it has a scalac plugin.

This comment has been minimized.

@benjyw

benjyw Mar 1, 2017

Contributor

Anyway, I'll hold off on pushing this until someone in the know chimes in about this. I don't know enough about scala.js to have a strong opinion.

@benjyw

benjyw Mar 1, 2017

Contributor

Anyway, I'll hold off on pushing this until someone in the know chimes in about this. I don't know enough about scala.js to have a strong opinion.

+ classpath_element))
+ active_plugins[name] = rel_classpath_element
+
+ for c in classpath:

This comment has been minimized.

@stuhood

stuhood Feb 27, 2017

Member

Since _find_scalac_plugins will run for every compilation, it would be good to memoize some of this. Maybe extract the body of this loop (each classpath entry) into a memoized_method?

@stuhood

stuhood Feb 27, 2017

Member

Since _find_scalac_plugins will run for every compilation, it would be good to memoize some of this. Maybe extract the body of this loop (each classpath entry) into a memoized_method?

This comment has been minimized.

@benjyw

benjyw Feb 28, 2017

Contributor

Great idea. Done.

@benjyw

benjyw Feb 28, 2017

Contributor

Great idea. Done.

- cls.register_jvm_tool(register, 'scalac-plugin-jars', classpath=[])
+ cls.register_jvm_tool(register, 'scalac-plugin-jars', classpath=[],
+ removal_version='1.5.0.dev0',
+ removal_hint='Use --compile-zinc-scalac-plugin-dep instead.')

This comment has been minimized.

@benjyw

benjyw Feb 28, 2017

Contributor

Hmmm, I wondered about this, and I'm still not sure.

Technically, plugins aren't part of the Scala platform - they're not part of any spec. Rather they are a detail of scalac. If we one day switched to using a different compiler, it might not have plugins at all. So to me that makes it look more like a feature of our compilation task than of ScalaPlatform, which is mostly for specifying versions of the scala runtime.

It's true that we mention the scala compiler version in ScalaPlatform, and that scalac is closely tied to the runtime, but I'd rather not add to that confusion.

Similar considerations might apply to Java, although the (poorly-named, my bad) Java subsystem doesn't specify anything about runtimes, just javac. So really it should be called Javac, and then I suppose it might be a sensible place for plugin settings.

Nonetheless, it really seems like more of a configuration of our compilation task, but I could be convinced otherwise. I'm fuzzy on this one.

Thoughts? Other reviewers want to chime in?

@benjyw

benjyw Feb 28, 2017

Contributor

Hmmm, I wondered about this, and I'm still not sure.

Technically, plugins aren't part of the Scala platform - they're not part of any spec. Rather they are a detail of scalac. If we one day switched to using a different compiler, it might not have plugins at all. So to me that makes it look more like a feature of our compilation task than of ScalaPlatform, which is mostly for specifying versions of the scala runtime.

It's true that we mention the scala compiler version in ScalaPlatform, and that scalac is closely tied to the runtime, but I'd rather not add to that confusion.

Similar considerations might apply to Java, although the (poorly-named, my bad) Java subsystem doesn't specify anything about runtimes, just javac. So really it should be called Javac, and then I suppose it might be a sensible place for plugin settings.

Nonetheless, it really seems like more of a configuration of our compilation task, but I could be convinced otherwise. I'm fuzzy on this one.

Thoughts? Other reviewers want to chime in?

This comment has been minimized.

@benjyw

benjyw Feb 28, 2017

Contributor

After pondering this further - plugin specs seem more like compiler args than a part of the platform, so I'm now leaning more towards keeping these options on the compile.zinc task.

@benjyw

benjyw Feb 28, 2017

Contributor

After pondering this further - plugin specs seem more like compiler args than a part of the platform, so I'm now leaning more towards keeping these options on the compile.zinc task.

This comment has been minimized.

@benjyw

benjyw Mar 1, 2017

Contributor

More generally, we should sort out those subsystems, with an eye to "would these still make sense if we switched compilers"...

@benjyw

benjyw Mar 1, 2017

Contributor

More generally, we should sort out those subsystems, with an eye to "would these still make sense if we switched compilers"...

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

More generally, we should sort out those subsystems, with an eye to "would these still make sense if we switched compilers"...

Yep, makes sense. But regarding "part of the platform": I don't really know what a platform is. It's a fuzzy thing. So I'd really just like to rename the scala-platform subsystem to scala, for symmetry with java.

@stuhood

stuhood Mar 2, 2017

Member

More generally, we should sort out those subsystems, with an eye to "would these still make sense if we switched compilers"...

Yep, makes sense. But regarding "part of the platform": I don't really know what a platform is. It's a fuzzy thing. So I'd really just like to rename the scala-platform subsystem to scala, for symmetry with java.

This comment has been minimized.

@benjyw

benjyw Mar 3, 2017

Contributor

Yeah, this all needs sorting out.

@benjyw

benjyw Mar 3, 2017

Contributor

Yeah, this all needs sorting out.

@stuhood

stuhood approved these changes Mar 2, 2017

Really, really great. Thanks again Benjy.

+)
+```
+
+A javac plugin target has the same fields as `java_library` target,

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

as a

@benjyw benjyw merged commit f60a46d into pantsbuild:master Mar 3, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@benjyw benjyw deleted the benjyw:jvm_compiler_plugins branch Mar 3, 2017

@baroquebobcat

This comment has been minimized.

Show comment
Hide comment
@baroquebobcat

baroquebobcat Mar 3, 2017

Contributor

This looks pretty darn slick.

Contributor

baroquebobcat commented Mar 3, 2017

This looks pretty darn slick.

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment