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

Fix Gradle 6+ #503

Merged
merged 18 commits into from
Jan 1, 2020
Merged

Fix Gradle 6+ #503

merged 18 commits into from
Jan 1, 2020

Conversation

nedtwigg
Copy link
Member

@nedtwigg nedtwigg commented Jan 1, 2020

Gradle 6 has a couple things that I'm excited to use in other projects, but our dependency resolution technical debt throws a lot of deprecation warnings in Gradle 6. I don't want to break back-compat unless we have to, but it's possible that our hand will be forced soon. We just got a great comment on an old bug (still present in our latest versions). That user is stuck on Gradle 3.5 (!!!), but their insight broke the logjam and got it solved. So, let's see if we can fix this without breaking old users...

@nedtwigg
Copy link
Member Author

nedtwigg commented Jan 1, 2020

Defining terms

An idealized view of dependency resolution is f(mavenCoords) = files. A more accurate way to model dep resolution in gradle is f(mavenCoords, repoConfig) = files. Importantly, repoConfig isn't just repository URLs, it might also be content exclusions, which can even depend on the arbitrary "name" you give to the list of maven coordinates (named configuration).

A gradle build needs to resolve dependencies for two purposes - for the buildscript (build plugins and tools), and for the project (transitive dependencies of the actual thing being built). For the rest of this issue, it is important to understand that dependency resolution for the buildscript and the project are completely independent, and it will be confusing if we accidentally use "project" to mean "buildscript and project".

Gradle defaults and behavior

  • by default, the buildscript of the root project is resolved from gradlePluginPortal, which is a superset of jcenter, which is a superset of mavenCentral
  • by default, the buildscript of subprojects cannot resolve any new dependencies
    • but subprojects inherit the classloader from their parent
    • this gives rise to the standard practice of defining all plugins used anywhere throughout the project in the root project, even if they are only used in subprojects
    • it is possible to manually declare repositories for subproject buildscripts, which makes it possible to resolve dependencies there, but Deprecate buildscript {} in subprojects gradle/gradle#7456 might deprecate this
  • projects have no repositories by default, so users always have to declare one
    • the declared repository will usually be mavenCentral or some superset, possibly augmented by other project-specific repos (e.g. android) or restricted by a company-specific security team

Spotless' unique use-case

When a user does googleJavaFormat('1.2') or scalafmt('0.5.1'), they are effortlessly creating an isolated classloader which gets cached (and thus JIT-accelerated) across all subprojects. To facilitate lazy configuration, the mavenCoordinates needed for the step are not calculated until a task evaluates its up-to-date status. In order to support exact up-to-date caching, the step needs to resolve the jar files implied by mavenCoordinates, to make sure that the formatter itself is exactly the same.

Spotless does not resolve, or even declare, any dependencies during buildscript evaluation. However, after the buildscripts have been executed and gradle is calculating the up-to-date status, we have to declare and resolve these dependencies. I'm not sure of the precise definition of "configuration time" in the gradle docs, but I'm pretty sure that gradle/gradle#2298 (forbid dependency resolution at configuration time by default) doesn't apply to us.

Spotless is a build tool, so it should resolve its dependencies from buildscript repositories, not project repositories. And by default, a subproject doesn't have any buildscript repositories. Our solution to this for a long time has been to resolve all of our dependencies from the root project's buildscript. This is kosher when we apply spotless to the root project, but when subprojects are evaluating their up-to-date status in parallel, then we're really abusing the root project. See #372 for the carnage.

@nedtwigg
Copy link
Member Author

nedtwigg commented Jan 1, 2020

Simple solutions

The simplest solution is to stop abusing the root buildscript, and instead resolve dependencies from each project's buildscript individually. We would still get classloader caching across subprojects (caching is based only on the value of the resolved files), so there's no performance hit. The problem is that users will have to add buildscript { repositories { ... }} to every subproject build.gradle, and even that workaround is at risk of being deprecated.

The next solution is to instead abuse each project's project repositories. So long as the user has defined some mavenCentral superset, this should work fine. But it's definitely not ideal. See #99 for an example of someone who has the requirement that their build and their project come from separate repositories.

We can even let the user pick which of these two workarounds they prefer. e3f9f67 above shows how easy this is to implement, but neither is a good user experience, so I reverted it to look for something better.

@nedtwigg
Copy link
Member Author

nedtwigg commented Jan 1, 2020

Resolving the conflicts

I think it is wise for Gradle to push people to centralize their buildscript dependencies into the root project. I also think it is good that we let people grab a wide variety of formatters and versions without thinking about named configurations and the behind-the-scenes work. There is a conflict, but I don't think either side of the conflict should move very far.

There is a simple (and definitely correct) solution, which combines these two pieces of the story above:

  • the way that Spotless resolves dependencies now is already fine for the root project
  • the reason we can't do that same thing for subprojects is that Gradle pushes people (wisely, I think) to resolve all dependencies in the root project

The solution is to require that if a step is used in a subproject, it must also be used (or at least declared) in the root project, even if it's not actually applied to a target.

A more thorough look at the resolution

The whole trick is inside one commit, 892203d, which is tested working on Gradle 2.14 and 6+. We create a new task in the root project called spotlessRegisterDependencies, which depends on every SpotlessTask in the root project. This means that in order for spotlessRegisterDependencies to determine its up-to-date status, every single SpotlessTask in the root project will need to register and resolve its dependencies. Because this is all happening within the root project, there is no parallelism here.

In subprojects, every SpotlessTask will have a dependency on :spotlessRegisterDependencies. And rather than resolving its dependencies on its own, these subproject SpotlessTasks will just copy the answer from the root project. If a subproject asks for a set of maven coordinates which the root project never asked for, then we give the user a warning, and we'll do the shady rootProject workaround that we've been doing, which will cause Gradle 6+ to give its own warning.

For some projects, the spotless block is the same in the root project as in the child projects. These projects will just work without any changes or warnings. A more common archetype has a root project with no code, and a bunch of subprojects with code that will need e.g. a java formatter that isn't needed in the root. Projects such as this will see a warning like this:

This subproject is using a formatter that was not used in the root project.  To enable
performance optimzations (and avoid Gradle 7 deprecation warnings), you must declare
all of your formatters within the root project.  For example, if your subproject has
a `java {}` block but your root project does not, just add a matching `java {}` block to
your root project.  If you want to make it clear that it is intentional that the target
is empty, you can do this in your root build.gradle:

  spotless {
    java {
      targetEmptyForDeclaration()
      [...same steps as subproject...]
    }
  }

To help you figure out which block is missing, the step you are missing is
  step name: google-java-format
  requested: com.google.googlejavaformat:google-java-format:1.2 with transitives

TODO

docs, changelog, fix the maven build (JitCI does everything except maven, and it's passing).

This is a pretty complicated PR, and I'm in a bit of a hurry because there is other stuff I'd like to release which requires Gradle 6+ and thus is blocked on this. The only new user-facing API is FormatExtension.targetEmptyForDeclaration(), so we could revert this whole PR and the only thing we'd have to deprecate is that one method. All the rest is just under-the-hood improvements, along with helpful error messages to enable the improvements to run. So if you'd like to debate this but I've already merged and released, please don't hesitate to chime in, I'm very willing to revisit this.

@diffplug diffplug deleted a comment from jbduncan Jan 1, 2020
@diffplug diffplug deleted a comment from jbduncan Jan 1, 2020
…arations in root. We can infer them without a performance penalty by registering every subproject SpotlessTask with the RegisterDependenciesTask. When we eventually bump our minimum-required gradle to take advantage of task configuration avoidance, this workaround will still work - the only task we'll have to create eagerly is the registerDependencies task.
…re out which step is being configured."

This reverts commit 28d8962.
@nedtwigg
Copy link
Member Author

nedtwigg commented Jan 1, 2020

The best way to understand the final change is the collapsed changes across the whole PR (files tab). The high level overview is:

* - When a user asks for a formatter, we need to download the jars for that formatter
* - Gradle wants us to resolve all our dependencies in the root project - no new dependencies in subprojects
* - So, whenever a SpotlessTask in a subproject gets configured, we call {@link #hookSubprojectTask(SpotlessTask)},
* which makes this task a dependency of the SpotlessTask
* - When this "registerDependencies" task does its up-to-date check, it queries the task execution graph to see which
* SpotlessTasks are at risk of being executed, and causes them all to be evaluated safely in the root buildscript.

@nedtwigg nedtwigg merged commit d776768 into master Jan 1, 2020
@nedtwigg nedtwigg deleted the feat/gradle-6 branch January 1, 2020 22:56
@nedtwigg
Copy link
Member Author

nedtwigg commented Jan 2, 2020

Published in 3.27.0 (twitter announcement)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants