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 conflicts between Spotless and Android re: clean task #1014

Merged
merged 10 commits into from
Dec 6, 2021
Merged

Conversation

nedtwigg
Copy link
Member

@nedtwigg nedtwigg commented Dec 5, 2021

Spotless eagerly applies BasePlugin for the purpose of creating the clean task. This causes a problem for users who define their own clean task, which the Android template recommends.

Now we don't have to anymore, because we find it lazily (and without triggering configuration) like so:

static void configureCleanTask(Project project, Consumer<Delete> onClean) {
project.getTasks().withType(Delete.class).configureEach(clean -> {
if (clean.getName().equals(BasePlugin.CLEAN_TASK_NAME)) {
onClean.accept(clean);
}
});
}

@nedtwigg nedtwigg requested a review from jbduncan December 5, 2021 07:00
@jbduncan
Copy link
Member

jbduncan commented Dec 6, 2021

Wow, I've just been down the rabbit hole that is issue #429!

To summarise, it looks like our ktlint test was flaking for a long time because it was using JCenter to download ktlint, and switching to Maven Central on main fixed that?

I also understand that this flakiness was aggravated at one point by our use of detached configurations in GradleProvisioner, which was making projects using Spotless and Gradle's parallelism support throw mysterious exceptions half the time? Although I understand that this was fixed back in Spotless 5, when we replaced our detached configurations with named ones.

So I suppose my only query is: will the @Disabled annotation in this PR be removed after this PR is merged in?

If so, then I have no other concerns. The rest of this PR LGTM! 👍

Copy link
Member

@jbduncan jbduncan left a comment

Choose a reason for hiding this comment

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

Other than the @Disabled annotation in the ktlint test file, this PR looks good to me.

@nedtwigg
Copy link
Member Author

nedtwigg commented Dec 6, 2021

will the @disabled annotation in this PR be removed after this PR is merged in?

No. We are using the gradle retry plugin so that flaky dependency resolutions get a chance to retry. But even with that, we still got a failure, but only on shyiko

https://app.circleci.com/pipelines/github/diffplug/spotless/1518/workflows/2a6d9e93-1482-487e-ad96-98ab9d9a0872/jobs/8664

The shyiko stuff is quite old, ktlint has lived at com.pinterest for a few years now, so I'm hesitant to put much time into tracking it down.

@jbduncan
Copy link
Member

jbduncan commented Dec 6, 2021

The shyiko stuff is quite old, ktlint has lived at com.pinterest for a few years now, so I'm hesitant to put much time into tracking it down.

Ah okay, I understand. This seems acceptable to me, then. LGTM. :)

@nedtwigg nedtwigg merged commit de5339f into main Dec 6, 2021
@nedtwigg nedtwigg deleted the fix/clean branch December 6, 2021 19:01
@nedtwigg
Copy link
Member Author

nedtwigg commented Dec 6, 2021

Shipped in plugin-gradle/6.0.3.

@nedtwigg
Copy link
Member Author

This PR was ultimately reverted in 6.5.0 by #1179 in response to user story #1164.

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.

3 participants