Partially support isolated projects#2854
Conversation
|
Wonderful! Looks like it needs a |
3db843d to
611af68
Compare
611af68 to
f7829b5
Compare
41e3f19 to
bebb746
Compare
|
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. |
https://github.com/diffplug/spotless/tree/main/plugin-gradle#dependency-resolution-modes This should have a note of some kind saying "breaks isolated projects" or something like that. Assuming CI passes and no other contributors have any objections, I'll merge and release next week. |
|
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: CI sees this: 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 Edit: I just saw that |
In anticipation of diffplug/spotless#2854.
| List<String> customRuleSets) throws IOException { | ||
| Objects.requireNonNull(version); | ||
| File defaultEditorConfig = getProject().getRootProject().file(".editorconfig"); | ||
| File defaultEditorConfig = new File(getProject().getRootDir(), ".editorconfig"); |
There was a problem hiding this comment.
Use project.isolated.rootProject.projectDirectory.file(".editorconfig") instead.
There was a problem hiding this comment.
It doesn't work with the min Gradle requirement, need to add glue.
There was a problem hiding this comment.
Added a new commit. .getLayout() seems to be IP safe so no need to use unstable APIs
There was a problem hiding this comment.
Let me check something, I think the IP test is not calling that code and is giving a false hope.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleCompat.java
Show resolved
Hide resolved
| if (!isUsingPredeclared) | ||
| return; | ||
|
|
||
| project.getRootProject().getTasks().withType(RegisterDependenciesTask.class, (registerTask) -> { |
There was a problem hiding this comment.
Calling getRootProject().getTasks() looks unsafe for IP.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
All right. Let's leave it here. I couldn't find a replacement for it either.
plugin-gradle/src/test/java/com/diffplug/gradle/spotless/MultiProjectTest.java
Outdated
Show resolved
Hide resolved
27c3007 to
c336483
Compare
e2a5b92 to
2519190
Compare
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:
.getRootProject.file(...)since it's not supported by isolated projects.GradleCompatfile becauseproject.findPropertyandproject.hasPropertycan't be used and the compat file is not meaningful anymore.SpotlessTaskServicereference fromRegisterDependenciesTasktoSpotlessExtensionas the task is not really owning the task service. This simplifies some dependencies needing to get the task reference first.SpotlessExtensionnot reference theRegisterDependenciesTaskwhich is created in the root project and moves all of that toSpotlessExtensionPredeclare. Since this task is only needed (AFAIK) for predeclared, it makes sense that the task is only referenced there.RegisterDependenciesTaskis not executed when predeclared is not used.