Partially support isolated projects by inorichi · Pull Request #2854 · diffplug/spotless

Partially support isolated projects by inorichi · Pull Request #2854 · diffplug/spotless

This PR adds partial support for isolated projects.

They're supported as long as predeclareDeps() is not called in the root extension.

The PR does the following:

Removes .getRootProject.file(...) since it's not supported by isolated projects.
Removes the GradleCompat file because project.findProperty and project.hasProperty can't be used and the compat file is not meaningful anymore.
Moves the SpotlessTaskService reference from RegisterDependenciesTask to SpotlessExtension as the task is not really owning the task service. This simplifies some dependencies needing to get the task reference first.
Makes the SpotlessExtension not reference the RegisterDependenciesTask which is created in the root project and moves all of that to SpotlessExtensionPredeclare. Since this task is only needed (AFAIK) for predeclared, it makes sense that the task is only referenced there.
Some tests needed adjustments because RegisterDependenciesTask is not executed when predeclared is not used.
Adds a test that verifies isolated projects compatibility.

Wonderful! Looks like it needs a ./gradlew spotlessApply before CI can check it. Will also need changelog entries and doc updates where appropriate.

Fixed. Sorry I rebased and force pushed the changes, it's an habit of mine...

I added the changelog entry, but I'm not sure where I should update the documentation.

I added a note where you suggested, let me know if it's good enough.

Regarding CI, it seems like there are different isolated project error outputs, while I get the following locally:

- Build file '0/build.gradle': Cannot access project ':' from project ':0'

CI sees this:

- Build file '0/build.gradle': Project ':0' cannot access 'Project.tasks' functionality on another project ':'

And that breaks the test I created. I don't understand the difference, since I'm running the gradle wrapper too and should run the same gradle version. I'll investigate...

Okay, it seems like CI was executing that test on Gradle 9, and the message is different on that version, I've changed the test to .containsAnyOf("Cannot access project", "cannot access 'Project.tasks'") so that Gradle 8 and 9 are supported.

Edit: I just saw that GradleIntegrationHarness runs different gradle versions based on the JDK used, that's what happened.

VegetalDev added a commit to Echoes-from-Beyond/echoes-from-beyond that referenced this pull request

		List<String> customRuleSets) throws IOException {
		Objects.requireNonNull(version);
		File defaultEditorConfig = getProject().getRootProject().file(".editorconfig");
		File defaultEditorConfig = new File(getProject().getRootDir(), ".editorconfig");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use project.isolated.rootProject.projectDirectory.file(".editorconfig") instead.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work with the min Gradle requirement, need to add glue.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new commit. .getLayout() seems to be IP safe so no need to use unstable APIs

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me check something, I think the IP test is not calling that code and is giving a false hope.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll create a separate test file for testing IP. I added ktlint to the tests in MultiProjectTest and they still work, even with the old getProject().getRootProject().file(".editorconfig"), and this is really weird as this was giving me issues when I was testing on a real project.

I don't have time right now to check all of this, I'll try to take another look later.

Edit: I needed to apply ktlint on the subprojects, now the tests are failing even with .getLayout() so my previous assertion was totally wrong.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new File(getProject().getRootProject().getProjectDir(), ".editorconfig"); is compatible with IP, which should be the same as getProject().getRootProject().file(".editorconfig");. I updated my PR to use that and separated the test to another file.

I'm only testing ktlint on the test, as that's the only formatter that was not compatible with IP, but maybe all the formatters should be tested? Or maybe they should only be checked when/if they break.

Edit: I've also reverted the test to use gradle.properties because then it's enabled for all tests in this file, and the same approach is used by the configuration cache tests.

		if (!isUsingPredeclared)
		return;

		project.getRootProject().getTasks().withType(RegisterDependenciesTask.class, (registerTask) -> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling getRootProject().getTasks() looks unsafe for IP.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the only incompatibility remaining (hence the partial support in the title), I added a comment about it in line 132.

This affects only the predeclared dependencies, which I think should not be used alongside isolated projects.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right. Let's leave it here. I couldn't find a replacement for it either.