Issues with conventional plugin implementation classpath wiring in 2.13.

[Luke Daley]

Hi,

I’ve been reviewing https://github.com/gradle/gradle/blob/master/design-docs/testing-toolkit.md#story-plugin-development-plugin-automatically-injects-plugin-classpath. This is all currently on master and does not impact 2.12.

Here’s what I found that’s suboptimal:

1. This fails badly when there is no plugin manifest available, which can happen when executing a test from the IDE after a clean (or clean checkout)

The failure you get here is that the build under test can’t find the plugin (i.e. the one under test) that it is trying to apply. This is the generic plugin not found message:

    FAILURE: Build failed with an exception.

    * Where:

    Build file '/private/var/folders/dy/9ksxfrqj64v5r56zkkhh3wnh0000gn/T/junit3394743112889677484/build.gradle' line: 3

    * What went wrong:

    Plugin [id: 'example.plugin'] was not found in any of the following sources:

    - Gradle Core Plugins (plugin is not in 'org.gradle' namespace)

    - Gradle Central Plugin Repository (plugin dependency must include a version number for this source)

    * Try:

    Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

    BUILD FAILED

    Total time: 1.506 secs

At the moment, this all works by magically using the classpath manifest file if it is there and not using it if it is not. That is, we have no way of knowing that the user expects it to be there so we can’t fail if it’s not there. It is legitimate for it to not be there for a number of reasons.

I’m not sure we can live with this behaviour. The failure diagnostic is far too indirect. The only solution I can propose is that we require the user to indicate that they intend to use the magical classpath wiring via a method on GradleRunner, e.g.: 

withPluginClasspath() 

This would overload the existing withPluginClasspath(Iterable<? extends File> classpath), and as a bonus give us an opportunity to document this behaviour on the API. We _could later_ (i.e. not now) add further overloads to make it possible to back this convention off gradually and use alternative “resources” for obtaining the plugin-under-test classpath (we have the need for this kind of thing in the testing of the build receipt plugin).

2. This fails badly when the plugin manifest is out of date, which can happen when someone changes a source file in the IDE and then re-runs the test in the IDE

There’s no obvious resolution here. There are three possible approaches:

1. Do nothing and rely on the user to detect this by the test not behaving as expected

2. Try and detect that the plugin classpath is out of date and warn or fail

3. Use the test process runtime classpath, or parts of it

This is also not a new problem; the existing usage patterns that we encourage suffer from the same problem. What is different here though is the level of magic happening makes it less obvious that this could happen. That is, users are more likely to think now that the plugin implementation is being taken from the runtime test classpath.

For #2, the only idea I have is to include the locations of the plugin source in the information we pass and to check every time it’s going to be used whether there are any source files with a modification timestamp (and possibly checksum) newer than the newest file on the classpath. This would be doable, but it would be non trivial and not 100% accurate as there are more inputs into the plugin than the source.

For #3, this would be a kind of cross referencing of plugin classpath manifest with the test runtime. That is, find the equivalent class files on disk being used by the test runtime (e.g. IDE test runtime) of the ones advertised by the manifest and use those, assuming they are more up to date. There are all kinds of problems with this though and I think it is a dead end.

It’s worth noting that this problem does go away to some extent for simple setups when using IDEA and direct import, because it writes class files to the same location that Gradle does. This means that it is actually updating the implementation classpath advertised by the manifest.

I propose we don’t do anything about this problem now and wait for complaints. I think there’s a reasonable understanding of this out-of-dateness problem by developers that do any kind of code generation in the wild and this is exactly the same problem. We can recommend that people use IDEA or Buildship and execute the tests from the IDE but have Gradle run them.

3. An import of a stock `java-gradle-plugin` into IDEA fails when trying to run tests because of the (known) incompatibility of gradleTestKit() and gradleApi()

We know this problem and are working on a solution. What this means though is that we can’t release this feature (i.e. automatic classpath wiring) without first fixing that.

4. The data exchange between the build time and test runtime will be complex to evolve

Currently, this exchange is a plain text file called `plugin-classpath.txt` located at the root of the classpath whose contents are the plugin implementation classpath with one entry per line. If we want to exchange more information (e.g. the source location, the plugin ID) we’d need another mechanism or to replace this mechanism.

I think we should use a more evolvable format; specifically a properties file. I think we should name the file `plugin-under-test-metadata.properties` and use a `implementation-classpath` property to communicate the classpath. Later, we can add more entries if we need to.

5. The name of the generation task should not be a verb

We generally, rightly or wrongly, do not use verb-ish tasks names for tasks that produce artifacts.  Right now, the task that creates the classpath manifest is named `generatePluginClasspathManifest`. This should be called `pluginClasspathManifest`. For example, we use `jar` instead of `generateJar`.

6. The plugin implementation is not an explicit input into consuming tasks

It is for simple setups though, by accident. The actual content of the implementation classpath _is_ an input into any task that is going to load up the plugin and use it (e.g. a Test task). However, we strictly speaking are only treating the locations of the classpath components as an input, not the actual content (i.e. the manifest file is an input to the Test task, but not the files that the manifest points to). For the out of the box setup though, they actually are because they just happen to be the main source set output (mostly) and this happens to be wired as an input into the Test task. It’s completely legitimate for a Test task to not require the plugin implementation as part of its runtime classpath though and our by-accident wiring breaks down here.

Three options:

1. Ignore the problem - i.e. accidental wiring working for the out of the box case is good enough

2. Force the plugin implementation classpath to be part of the runtime classpath of any plugin implementation consuming source set

3. Wire the implementation classpath in as inputs to the Test task directly

#2 has the advantage in that it works for all consumers of the consuming source set (e.g. a JavaExec task using sourceSets.test.runtimeClasspath to use GradleRunner). The disadvantage is that we are effectively polluting the classpath unnecessarily.

#3 has the advantage in that it avoids polluting the classpath, but requires knowing/specifying the consuming tasks. We can have the java-gradle-plugin plugin wire up the `test` task, but can’t more than that conventionally.

7. The `javaGradlePlugin.functionalTestClasspath` extension mechanism is clunky WRT naming.

At the moment we have:

javaGradlePlugin {

    functionalTestClasspath {

        testSourceSets sourceSets.functionalTest

    }

}

This should be:

gradlePlugin {

  testSourceSets sourceSets.functionalTest

}

i.e. we should keep this extension agnostic to the implementation language, and remove the `functionalTestClasspath` layer.

8. We might want to work on the “Java Gradle Plugin Development Plugin” user guide chapter

https://docs.gradle.org/current/userguide/javaGradle_plugin.html

We are linking to this from the TestKit chapter, so it will probably get more traffic than it ever has. We can probably release with it like this, but it could certainly be improved to be a little more clear as to why you should use it and how.

9. The classpath is silently not injected if the Gradle version used to run the test build doesn’t support magic injection (< 2.9 IIRC)

This is a trap. However, because we don’t really know that the user wants to use injection it’s not clear that we should just fail hard. There might be a `plugin-classpath.txt` there, but the user intends to do the manual injection for older versions (which is practically invisible to us).

To solve this, I think we need to require an explicit declaration of the intent to use `plugin-classpath.txt` file. That way, we could fail hard if the Gradle version being used doesn’t support it just like we do with an explicit classpath.

[Benjamin Muschko]

On Tue, Mar 1, 2016 at 12:05 AM, Luke Daley <lu...@gradle.com> wrote:

Hi,

I’ve been reviewing https://github.com/gradle/gradle/blob/master/design-docs/testing-toolkit.md#story-plugin-development-plugin-automatically-injects-plugin-classpath. This is all currently on master and does not impact 2.12.

Here’s what I found that’s suboptimal:

1. This fails badly when there is no plugin manifest available, which can happen when executing a test from the IDE after a clean (or clean checkout)

The failure you get here is that the build under test can’t find the plugin (i.e. the one under test) that it is trying to apply. This is the generic plugin not found message:

    FAILURE: Build failed with an exception.

    * Where:

    Build file '/private/var/folders/dy/9ksxfrqj64v5r56zkkhh3wnh0000gn/T/junit3394743112889677484/build.gradle' line: 3

    * What went wrong:

    Plugin [id: 'example.plugin'] was not found in any of the following sources:

    - Gradle Core Plugins (plugin is not in 'org.gradle' namespace)

    - Gradle Central Plugin Repository (plugin dependency must include a version number for this source)

    * Try:

    Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

    BUILD FAILED

    Total time: 1.506 secs

At the moment, this all works by magically using the classpath manifest file if it is there and not using it if it is not. That is, we have no way of knowing that the user expects it to be there so we can’t fail if it’s not there. It is legitimate for it to not be there for a number of reasons.

I’m not sure we can live with this behaviour. The failure diagnostic is far too indirect. The only solution I can propose is that we require the user to indicate that they intend to use the magical classpath wiring via a method on GradleRunner, e.g.: 

withPluginClasspath() 

This would overload the existing withPluginClasspath(Iterable<? extends File> classpath), and as a bonus give us an opportunity to document this behaviour on the API. We _could later_ (i.e. not now) add further overloads to make it possible to back this convention off gradually and use alternative “resources” for obtaining the plugin-under-test classpath (we have the need for this kind of thing in the testing of the build receipt plugin).

Done, introduced withPluginClasspath(). Users will have to actively declare that they want to automatically inject the classpath.

2. This fails badly when the plugin manifest is out of date, which can happen when someone changes a source file in the IDE and then re-runs the test in the IDE

There’s no obvious resolution here. There are three possible approaches:

1. Do nothing and rely on the user to detect this by the test not behaving as expected

2. Try and detect that the plugin classpath is out of date and warn or fail

3. Use the test process runtime classpath, or parts of it

This is also not a new problem; the existing usage patterns that we encourage suffer from the same problem. What is different here though is the level of magic happening makes it less obvious that this could happen. That is, users are more likely to think now that the plugin implementation is being taken from the runtime test classpath.

For #2, the only idea I have is to include the locations of the plugin source in the information we pass and to check every time it’s going to be used whether there are any source files with a modification timestamp (and possibly checksum) newer than the newest file on the classpath. This would be doable, but it would be non trivial and not 100% accurate as there are more inputs into the plugin than the source.

For #3, this would be a kind of cross referencing of plugin classpath manifest with the test runtime. That is, find the equivalent class files on disk being used by the test runtime (e.g. IDE test runtime) of the ones advertised by the manifest and use those, assuming they are more up to date. There are all kinds of problems with this though and I think it is a dead end.

It’s worth noting that this problem does go away to some extent for simple setups when using IDEA and direct import, because it writes class files to the same location that Gradle does. This means that it is actually updating the implementation classpath advertised by the manifest.

I propose we don’t do anything about this problem now and wait for complaints. I think there’s a reasonable understanding of this out-of-dateness problem by developers that do any kind of code generation in the wild and this is exactly the same problem. We can recommend that people use IDEA or Buildship and execute the tests from the IDE but have Gradle run them.

I'd agree here. This is already the case. So far I haven't seen any reports on the forum from people complaining about it. I'd say let's keep it as is for now and tackle it if we get complaints.

3. An import of a stock `java-gradle-plugin` into IDEA fails when trying to run tests because of the (known) incompatibility of gradleTestKit() and gradleApi()

We know this problem and are working on a solution. What this means though is that we can’t release this feature (i.e. automatic classpath wiring) without first fixing that.

The issue already exists even without the integration of the `java-gradle-plugin`.

4. The data exchange between the build time and test runtime will be complex to evolve

Currently, this exchange is a plain text file called `plugin-classpath.txt` located at the root of the classpath whose contents are the plugin implementation classpath with one entry per line. If we want to exchange more information (e.g. the source location, the plugin ID) we’d need another mechanism or to replace this mechanism.

I think we should use a more evolvable format; specifically a properties file. I think we should name the file `plugin-under-test-metadata.properties` and use a `implementation-classpath` property to communicate the classpath. Later, we can add more entries if we need to.

Changed it to the suggested proposal.

5. The name of the generation task should not be a verb

We generally, rightly or wrongly, do not use verb-ish tasks names for tasks that produce artifacts.  Right now, the task that creates the classpath manifest is named `generatePluginClasspathManifest`. This should be called `pluginClasspathManifest`. For example, we use `jar` instead of `generateJar`.

Changed it to the suggested proposal.
 

6. The plugin implementation is not an explicit input into consuming tasks

It is for simple setups though, by accident. The actual content of the implementation classpath _is_ an input into any task that is going to load up the plugin and use it (e.g. a Test task). However, we strictly speaking are only treating the locations of the classpath components as an input, not the actual content (i.e. the manifest file is an input to the Test task, but not the files that the manifest points to). For the out of the box setup though, they actually are because they just happen to be the main source set output (mostly) and this happens to be wired as an input into the Test task. It’s completely legitimate for a Test task to not require the plugin implementation as part of its runtime classpath though and our by-accident wiring breaks down here.

Three options:

1. Ignore the problem - i.e. accidental wiring working for the out of the box case is good enough

2. Force the plugin implementation classpath to be part of the runtime classpath of any plugin implementation consuming source set

3. Wire the implementation classpath in as inputs to the Test task directly

#2 has the advantage in that it works for all consumers of the consuming source set (e.g. a JavaExec task using sourceSets.test.runtimeClasspath to use GradleRunner). The disadvantage is that we are effectively polluting the classpath unnecessarily.

#3 has the advantage in that it avoids polluting the classpath, but requires knowing/specifying the consuming tasks. We can have the java-gradle-plugin plugin wire up the `test` task, but can’t more than that conventionally.

We discussed #3 early in our discussion for this spec. If we'd go for #3 we should make it a configurable convention though.

7. The `javaGradlePlugin.functionalTestClasspath` extension mechanism is clunky WRT naming.

At the moment we have:

javaGradlePlugin {

    functionalTestClasspath {

        testSourceSets sourceSets.functionalTest

    }

}

This should be:

gradlePlugin {

  testSourceSets sourceSets.functionalTest

}

i.e. we should keep this extension agnostic to the implementation language, and remove the `functionalTestClasspath` layer.

Changed it to the suggested proposal.

8. We might want to work on the “Java Gradle Plugin Development Plugin” user guide chapter

https://docs.gradle.org/current/userguide/javaGradle_plugin.html

We are linking to this from the TestKit chapter, so it will probably get more traffic than it ever has. We can probably release with it like this, but it could certainly be improved to be a little more clear as to why you should use it and how.

The functionality as we have it right now is really just the starting point for future enhancement. If we'd want to clarify and enhance the documentation it would be good to know the vision for this plugin which is not fully clear to me.

9. The classpath is silently not injected if the Gradle version used to run the test build doesn’t support magic injection (< 2.9 IIRC)

This is a trap. However, because we don’t really know that the user wants to use injection it’s not clear that we should just fail hard. There might be a `plugin-classpath.txt` there, but the user intends to do the manual injection for older versions (which is practically invisible to us).

To solve this, I think we need to require an explicit declaration of the intent to use `plugin-classpath.txt` file. That way, we could fail hard if the Gradle version being used doesn’t support it just like we do with an explicit classpath.

Goes together with #1 from my perspective. Changed it to the suggested proposal.

--
You received this message because you are subscribed to the Google Groups "gradle-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to gradle-dev+...@googlegroups.com.
To post to this group, send email to gradl...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/gradle-dev/etPan.56d5232f.59fd5b95.2b2b%40Trajan.local.
For more options, visit https://groups.google.com/d/optout.