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.

Sign up for GitHub

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

Skip execution in incremental (m2e) builds by default #2059

Merged
merged 7 commits into from
Apr 7, 2024
Merged

Skip execution in incremental (m2e) builds by default #2059

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d0460d7
Skip execution in incremental (m2e) builds by default
kwin Mar 2, 2024
70ba6ea
Add section to readme how to enable m2e support
kwin Mar 4, 2024
7b1e2f2
more markdown fixes
kwin Mar 4, 2024
2c2cdfa
missing question mark
kwin Mar 4, 2024
144b173
Add reasoning for disabling incremental support by default
kwin Mar 4, 2024
2f45f98
rename parameters to have a m2e prefix
kwin Apr 3, 2024
a2caf6e
add changelog entry
kwin Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions plugin-maven/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1696,6 +1696,28 @@ cmd> mvn spotless:apply -DspotlessFiles=my/file/pattern.java,more/generic/.*-pat

The patterns are matched using `String#matches(String)` against the absolute file path.

## Does Spotless support incremental builds in Eclipse?

Spotless comes with [m2e](https://eclipse.dev/m2e/) support. However, by default its execution is skipped in incremental builds as most developers want to fix all issues in one go via explicit `mvn spotless:apply` prior to raising a PR and don't want to be bothered with Spotless issues during working on the source code in the IDE.
To enable it use the following parameter

```
<configuration>
<m2eEnableForIncrementalBuild>true</m2eEnableForIncrementalBuild><!-- this is false by default -->
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<m2eEnableForIncrementalBuild>true</m2eEnableForIncrementalBuild><!-- this is false by default -->
<m2eIncrementalBuild>WARNING</m2eIncrementalBuild><!-- this is NONE by default, can also be ERROR -->

Copy link
Member

Choose a reason for hiding this comment

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

or maybe m2eIncrementalBuildMessages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

m2eIncrementalBuild is too fuzzy for me, m2eIncrementalBuildMessages sounds better.

Copy link
Member

Choose a reason for hiding this comment

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

That works for me :)

Copy link
Contributor Author

@kwin kwin Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

After having a second thought, I think having only one parameter is over-simplifying things, as we talk about two different goals:

  1. apply
  2. check

The message severity is only ever useful for the latter, while m2eEnableForIncrementalBuild applies to both.

Copy link
Member

Choose a reason for hiding this comment

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

If m2eEnableForIncrementalBuild is false, then m2eIncrementalBuildMessages has no effect, correct? And if m2eEnableForIncrementalBuild is true, would it ever make sense for m2eIncrementalBuildMessageSeverity to be NONE or SILENT?

I might be wrong, but this seems like having a separate switch for the fuel pump on a car.

Copy link
Contributor Author

@kwin kwin Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If m2eEnableForIncrementalBuild is false, then m2eIncrementalBuildMessages has no effect, correct?

That is correct.

And if m2eEnableForIncrementalBuild is true, would it ever make sense for m2eIncrementalBuildMessageSeverity to be NONE or SILENT?

No, but the point is that noone expects that m2eIncrementalBuildMessageSeverity or m2eIncrementalBuildMessages would toggle off the incremental build at all.

but this seems like having a separate switch for the fuel pump on a car.

For me having one multivalue switch feels more like adjusting your AC will implicitly also start your car ;-)

Copy link
Member

Choose a reason for hiding this comment

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

That's not such a bad idea if the car can't run without A/C. But whatever, I usually defer to the people who are gonna use the thing.

Only thing missing now is a changelog entry.

Copy link
Contributor Author

@kwin kwin Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Done

P.S. I would consider m2eIncrementalBuildMessageSeverity advanced setting (rarely adjusted) while m2eEnableForIncrementalBuild is a basic setting (easy to understand for everyone, more often adjusted). Hope this is enough justification for keeping both separate, but this is definitely my last argument to this discussion :-)

</configuration>
```

In addition Eclipse problem markers are being emitted. By default they have the severity `WARNING`.
You can adjust this with

```
<configuration>
<m2eIncrementalBuildMessageSeverity>ERROR</m2eIncrementalBuildMessageSeverity><!-- WARNING or ERROR -->
</configuration>
```

nedtwigg marked this conversation as resolved.
Show resolved Hide resolved
Note that for Incremental build support the goals have to be bound to a phase prior to `test`.

<a name="examples"></a>

## Example configurations (from real-world projects)
Expand Down
11 changes: 11 additions & 0 deletions plugin-maven/src/main/java/com/diffplug/spotless/maven/AbstractSpotlessMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,13 @@ public abstract class AbstractSpotlessMojo extends AbstractMojo {
@Parameter
private UpToDateChecking upToDateChecking = UpToDateChecking.enabled();

/**
* If set to {@code true} will also run on incremental builds (i.e. within Eclipse with m2e).
* Otherwise this goal is skipped in incremental builds and only runs on full builds.
*/
@Parameter(defaultValue = "false")
protected boolean m2eEnableForIncrementalBuild;

protected abstract void process(Iterable<File> files, Formatter formatter, UpToDateChecker upToDateChecker) throws MojoExecutionException;

private static final int MINIMUM_JRE = 11;
Expand Down Expand Up @@ -245,6 +252,10 @@ private boolean shouldSkip() {
if (skip) {
return true;
}
if (buildContext.isIncremental() && !m2eEnableForIncrementalBuild) {
getLog().debug("Skipping for incremental builds as parameter 'enableForIncrementalBuilds' is set to 'false'");
return true;
}

switch (goal) {
case GOAL_CHECK:
Expand Down
31 changes: 28 additions & 3 deletions plugin-maven/src/main/java/com/diffplug/spotless/maven/SpotlessCheckMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2023 DiffPlug
* Copyright 2016-2024 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -24,6 +24,7 @@
import org.apache.maven.plugin.MojoExecutionException;
import org.apache.maven.plugins.annotations.LifecyclePhase;
import org.apache.maven.plugins.annotations.Mojo;
import org.apache.maven.plugins.annotations.Parameter;
import org.sonatype.plexus.build.incremental.BuildContext;

import com.diffplug.spotless.Formatter;
Expand All @@ -38,6 +39,30 @@
@Mojo(name = AbstractSpotlessMojo.GOAL_CHECK, defaultPhase = LifecyclePhase.VERIFY, threadSafe = true)
public class SpotlessCheckMojo extends AbstractSpotlessMojo {

private static final String INCREMENTAL_MESSAGE_PREFIX = "Spotless Violation: ";

public enum MessageSeverity {
WARNING(BuildContext.SEVERITY_WARNING), ERROR(BuildContext.SEVERITY_ERROR);
nedtwigg marked this conversation as resolved.
Show resolved Hide resolved

private final int severity;

MessageSeverity(int severity) {
this.severity = severity;
}

public int getSeverity() {
return severity;
}
}

/**
* The severity used to emit messages during incremental builds.
* Either {@code WARNING} or {@code ERROR}.
* @see AbstractSpotlessMojo#m2eEnableForIncrementalBuild
*/
@Parameter(defaultValue = "WARNING")
private MessageSeverity m2eIncrementalBuildMessageSeverity;

@Override
protected void process(Iterable<File> files, Formatter formatter, UpToDateChecker upToDateChecker) throws MojoExecutionException {
ImpactedFilesTracker counter = new ImpactedFilesTracker();
Expand All @@ -51,14 +76,14 @@ protected void process(Iterable<File> files, Formatter formatter, UpToDateChecke
}
continue;
}

buildContext.removeMessages(file);
try {
PaddedCell.DirtyState dirtyState = PaddedCell.calculateDirtyState(formatter, file);
if (!dirtyState.isClean() && !dirtyState.didNotConverge()) {
problemFiles.add(file);
if (buildContext.isIncremental()) {
Map.Entry<Integer, String> diffEntry = DiffMessageFormatter.diff(formatter, file);
buildContext.addMessage(file, diffEntry.getKey() + 1, 0, diffEntry.getValue(), BuildContext.SEVERITY_ERROR, null);
buildContext.addMessage(file, diffEntry.getKey() + 1, 0, INCREMENTAL_MESSAGE_PREFIX + diffEntry.getValue(), m2eIncrementalBuildMessageSeverity.getSeverity(), null);
}
counter.cleaned();
} else {
Expand Down
Loading