Skip to content

spotlessApply and spotlessCheck in the same build fails in 4.x #581

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

Closed
andrej-urvantsev opened this issue May 24, 2020 · 4 comments · Fixed by #584
Closed

spotlessApply and spotlessCheck in the same build fails in 4.x #581

andrej-urvantsev opened this issue May 24, 2020 · 4 comments · Fixed by #584
Labels

Comments

@andrej-urvantsev
Copy link

I have this in my build.gradle.kts:

defaultTasks("spotlessApply", "build")

so I always run formatter before build. It worked up until version 4.0.0. Now plugin formats everything but fails at the end:

> Task :spotlessJavaCheck FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':spotlessJavaCheck'.
> The following files had format violations:
      src/test/java/se/inoviagroup/v2t/gateway/AbstractSpringBootTest.java
          
  Run './gradlew :spotlessApply' to fix these violations.

If I run build again - it passes, since everything was formatted previously.

It worked on gradle 6.4.1 and spotless gradle plugin 3.30.0.

@nedtwigg
Copy link
Member

Interesting! Here is the problem, and a quick workaround. Before Spotless 4.0, there was one task that did all the work, and check and apply were just control tasks that effectively set a flag on the worker task. That inadvertently meant that apply always happened before check.

In Spotless 4.0+, check and apply are both able to do their work independently. That means there is no ordering constraint, and if you do spotlessApply spotlessCheck, there's no guarantee which order they will happen in, and if check goes first then it will fail. A fix for this would be for every check to have mustRunAfter apply.

The thing is, running apply and check in the same build is a bad idea, and I'm tempted to make it an error. If you run apply, then check is always going to pass, which doesn't tell you anything. So if you commit badly formatted code, it will still pass CI, because CI is going to format it before checking.

Making it an error is probably too opinionated. It seems that other people also want to run apply on every build, so we should probably add the mustRunAfter constraint to make that work...

@andrej-urvantsev
Copy link
Author

Making it an error is probably too opinionated

I think it is. Especially in a case where I explicitly invoke formatting task during the build. So my goals in this case - format sources and build them at the same time.

I have spotlessApply task in "defaultTasks", so I invoke it in local builds, but I don't have it in CI/CD pipeline, so it just verifies formatting and fails if it's not formatted. This worked in version 3.30.0 at least.

I wouldn't like spotless to format my sources auto-magically on every build(so CI fails if sources aren't formatted), but I'd like to get back possibility to run formatting if it's invoked explicitly without failing the build.

@nedtwigg nedtwigg changed the title Plugin formats sources but fails anyway spotlessApply and spotlessCheck in the same build fails in 4.x May 26, 2020
@nedtwigg
Copy link
Member

@bigdaz interesting corner case we hadn't thought of in #576. It's not that uncommon for people to do spotlessApply build, which means that you get check and apply in the same build. At first, I thought this would be easy to fix with a simple checkTask.mustRunAfter(applyTask), but it turns out to be a bit harder, since check is using the same output of SpotlessTask as apply, which doesn't rerun after apply modified the source tree.

I'm inclined to just use the taskgraph and make check a no-op. If you're too busy to take a peek, I'll fix with the solution below in a week or so.

// if the user runs both, make sure that apply happens first,
// and then we don't need to run check since we know that apply just happened 
checkTask.mustRunAfter(applyTask);
project.getGradle().getTaskGraph().whenReady(graph -> {
  if (graph.hasTask(applyTask) && graph.hasTask(checkTask)) {
    checkTask.setEnabled(false);
  }
});

@nedtwigg
Copy link
Member

nedtwigg commented Jun 1, 2020

Fixed in plugin-gradle 4.1.0.

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

Successfully merging a pull request may close this issue.

2 participants