Build output directory is not excluded in Windows in Spotless 4.0.x

[davidburstromspotify]

Summary:
Spotless runs on build output files in Windows. The same project passes in Linux and MacOS.

Gradle version: 6.5.1-rc1

Spotless version: 4.0.0 and 4.0.1
Regression when moving from version: 3.30.0

Operating System: Windows Server 2012

Build has standard googleJavaFormat applied.

subprojects {
    apply(plugin = "com.diffplug.gradle.spotless")
    plugins.whenPluginAdded {
        if (this is JavaPlugin) {
            configure<SpotlessExtension> {
                java {
                    googleJavaFormat()
                }   
            }
        }
    }   
}

Build output:

Execution failed for task ':version:spotlessJavaCheck'. org.gradle.api.GradleException: The following files had format violations:
version\build\generated\src\constants\java\project\Version.java
Run 'gradlew.bat :version:spotlessApply' to fix these violations.
Caused by: org.gradle.api.GradleException: The following files had format violations:
     version\build\generated\src\constants\java\project\Version.java
  Run 'gradlew.bat :version:spotlessApply' to fix these violations.
    at com.diffplug.gradle.spotless.SpotlessCheck.formatViolationsFor(SpotlessCheck.java:87)
    at com.diffplug.gradle.spotless.SpotlessCheck.performAction(SpotlessCheck.java:76)
    at org.gradle.internal.reflect.JavaMethod.invoke(JavaMethod.java:104)
    ....

[nedtwigg]

Interesting! The culprit PR must be #576, but I don't see how it caused the regression.

How does the code-generation work? Based on the configuration you copy-pasted, I would expect autogenerated files in build/ to be within the Spotless target for all platforms, pre and post 4.x. So I would think that you would need targetExclude like so:

configure<SpotlessExtension> {
  java {
    targetExclude 'build/**'
    googleJavaFormat()
  }   
}

Without knowing more about your code generation, it looks like this may have been a bug in 3.x, which is now fixed in 4.x on Windows, but still broken on 4.x unix. A repo that reproduces the issue would be helpful to resolve the confusion.

[davidburstromspotify]

As per your suggestion, I tried adding

configure<SpotlessExtension> {
  java {
    targetExclude(files("$buildDir/**"))
    googleJavaFormat()
  }   
}

but it made no difference in Windows. The same build error happened.

[davidburstromspotify]

The code generation is done with

val VERSION: String by project

val constantsDir = File(project.buildDir, "generated/src/constants/java")

tasks.register("constantsGenerator") {
    val outputFile = File(constantsDir, "project/Version.java")
    inputs.property("version", VERSION as String)
    outputs.file(outputFile)
    doLast {
        outputFile.parentFile.mkdirs()
        outputFile.writeText("package project;\n" +
                "/** Holds the current build version. */\n" +
                "public final class Version {\n" +
                "  private Version() {}\n" +
                "\n" +
                "  public static final String VERSION = \"$VERSION\";\n" +
                "}\n")
    }
}

tasks.named("compileJava").configure {
    dependsOn(tasks.named("constantsGenerator"))
}

sourceSets.main {
    java.srcDir(constantsDir)
}

So, the directory is considered regular source input.

[valfirst]

It seems I'm facing the same issue: build passes on Linux and MacOS, but fails on Windows.
Build: https://github.com/vividus-framework/vividus/runs/703692427?check_suite_focus=true#step:8:105

* What went wrong:
Execution failed for task ':vividus:spotlessMisc'.
> Illegal char <:> at index 59: D:\a\vividus\vividus\vividus\output\spotless\spotlessMisc\D:\a\vividus\vividus\build.gradle

Change: vividus-framework/vividus-build-system@a2967fe

[nedtwigg]

@valfirst turns out your bug is unrelated to @davidburstromspotify's issue. Thanks for reporting though, it was useful, and we have a fix in #588. You will see vastly improved performance adopting the next version once it is published.

@davidburstromspotify I tried to reproduce, and here was what I observed:

• without targetExclude 'build/**', I get org.gradle.api.GradleException: The following files had format violations, which is correct and expected
• with targetExclude 'build/**', I get a different error, java.nio.file.DirectoryNotEmptyException, which exposes a bug in our incremental update. But if I do a clean and then spotlessCheck, the check passes, as expected. When you said "The same build error happened.", is there any chance it was actually a different error? Regardless, we need to fix the DirectoryNotEmptyException bug.

[nedtwigg]

The DirectoryNotEmptyException is fixed by #589.

[davidburstromspotify]

Just to confirm, I had the exact same problem as before:

Execution failed for task ':version:spotlessJavaCheck'. org.gradle.api.GradleException: The following files had format violations:
    version\build\generated\src\constants\java\project\Version.java

I did also see the DirectoryNotEmptyException at some point while trying out different combos, so I'm happy to see that is fixed.

[nedtwigg]

Oh! I just realized. Something strange is going with your target. Looks like you need targetExclude 'version/build/**', note the version prefix in your error message.

[nedtwigg]

If you have this in some shared/root script:

configure<SpotlessExtension> {
  java {
    targetExclude(files("$buildDir/**"))
    googleJavaFormat()
  }   
}

it might be that you're getting the absolute path of the root build directory put in there, and it's not applying correctly to subprojects. Same thing might be happening with your code generation, it looks your root project has version/build/... in its sourceset, whereas I would expect that folder to only be in the sourceset for the :version subproject.

[davidburstromspotify]

Nope, the targetExclude gets the build directory for the subproject. I'm going to set up a public project that reproduces the issue now.

[davidburstromspotify]

Here's a repo that I have tested on Mac and Windows. https://github.com/davidburstromspotify/spotless-issue-585

[nedtwigg]

I cloned your example repo, on win and mac. I believe it is all operating as expected. You are configuring your spotless in a subprojects block from the root project. In that block, if you do "$buildDir", you will get rootdir/build, not rootdir/subproject/build. Likewise, if you do files('build'), you are calling Project.files() on the root project, not on the subproject.

So I would expect that all of them would fail except for targetExclude('build/**'), because that is the only one where subproject/build will be excluded. You can add printlns to your buildscript for another way to see this.

The only thing I don't understand is why this ever worked in 3.30.0. One reason might be task ordering - if spotless runs before constantsGenerator, which it normally should, then it will pass. I don't know why Spotless 4.0 would change this order.

[davidburstromspotify]

Are you entirely sure about this? The subprojects block should set the actual subproject as the receiver of the methods in the block, and it was one of the first things I double checked. When I try this with the same repo you cloned, adding

java {
  println(buildDir)
  println("$buildDir")
  println(files("build/**")) // which isn't correct globbing for a file collection, but that's beside the point
  ...
  googleJavaFormat()
}

the output is the expected

/Users/xyz/repos/spotless-issue-585/version/build
/Users/xyz/repos/spotless-issue-585/version/build
[/Users/xyz/repos/spotless-issue-585/version/build/**]

i.e. a file in the version subproject, not the root project.

Could it be that buildDir gets relativized into version/version/build/** which doesn't match?

I'd say that using buildDir should be a supported scenario because it's not a given that the build directory always stays in the project or is called build. The string literal would be a DRY violation.

[nedtwigg]

Aha, you are correct! Still no idea how this worked in 3.30.0, but this is the code to parse target and targetExclude:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java

Lines 194 to 232 in 204c547

	} else if (target instanceof String ||
	(target instanceof List && ((List<?>) target).stream().allMatch(o -> o instanceof String))) {
	File dir = getProject().getProjectDir();
	PatternFilterable userExact; // exactly the collection that the user specified
	if (target instanceof String) {
	userExact = getProject().fileTree(dir).include((String) target);
	} else {
	// target can only be a List<String> at this point
	@SuppressWarnings("unchecked")
	List<String> targetList = (List<String>) target;
	userExact = getProject().fileTree(dir).include(targetList);
	}
	boolean filterOutGitAndGradle;
	// since people are likely to do '**/*.md', we want to make sure to exclude folders
	// they don't want to format which will slow down the operation greatly
	// but we only want to do that if they are *including* - if they are specifying
	// what they want to exclude, we shouldn't filter at all
	if (target instanceof String && !isExclude) {
	String str = (String) target;
	filterOutGitAndGradle = str.startsWith("**/*") || str.startsWith("**\\*");
	} else {
	filterOutGitAndGradle = false;
	}
	if (filterOutGitAndGradle) {
	List<String> excludes = new ArrayList<>();
	// no git
	excludes.add(".git");
	// no .gradle
	if (getProject() == getProject().getRootProject()) {
	excludes.add(".gradle");
	}
	// no build folders (flatInclude means that subproject might not be subfolders, see https://github.com/diffplug/spotless/issues/121)
	relativizeIfSubdir(excludes, dir, getProject().getBuildDir());
	for (Project subproject : getProject().getSubprojects()) {
	relativizeIfSubdir(excludes, dir, subproject.getBuildDir());
	}
	userExact = userExact.exclude(excludes);
	}
	return (FileCollection) userExact;

Note that it is all fileTree, versus files. Also, if you pass a string, it turns into fileTree(projectDir).include(string). That's why the absolute path of "$buildDir" fails, while build passes.

This was confusing and hard to debug, which means the design isn't very good. But in the design's defense, it is documented pretty clearly. Also, it's been this way forever, so it's very difficult to change. Thanks for sticking with this, it's a helpful example of how our API is confusing, but I'm not sure what we can do to improve it. Open to backward-compatible suggestions!

[nedtwigg]

The java.nio.file.DirectoryNotEmptyException bug, and also the Illegal char <:> at index bug that @valfirst reported are both fixed in 4.1.0.

Happy to discuss further ways to make target/targetExclude easier to use, but I think the heart (surface) of this issue has been solved.

[davidburstromspotify]

Thanks for the investigation! Well, at least there is a workaround.

Maybe it is not the right forum, but I also wanted to check with you what's up with GJF flagging the generated file as wrongly formatted when it is in fact identical (except for a suffix) to the https://github.com/davidburstromspotify/spotless-issue-585/blob/master/version/src/main/java/project/Version2.java file.

[nedtwigg]

A suffix is enough to make a file dirty.

[davidburstromspotify]

No, I meant that Version.java and Version2.java are identical, except that there's a "2" in Version2.java. Try diffing them for yourself to see what I mean. The generated Version.java has no formatting issues but is still failing.

[nedtwigg]

Ah, I see what you're talking about. google-java-format has had win vs unix bugs in the past. For example, this is a thing we did to fix a windows bug in google-java-format 1.1 on Windows:

spotless/lib/src/main/java/com/diffplug/spotless/java/GoogleJavaFormatStep.java

Lines 181 to 182 in e04fae5

	static String fixWindowsBug(String input, String version) {
	if (IS_WINDOWS && version.equals("1.1")) {

We have several features in place to paper-over bugs in upstream formatters, but there's only so much we can do. Always happy to take a PR to correct any platform-dependent behavior. One possible fix is to try a different version, e.g. googleJavaFormat('1.7') or 1.6.

[davidburstromspotify]

Hm, interesting. The weird thing here is that in my repro-repo, this applies on Mac as well. In any of the scenarios commented with build fails on Mac and Windows, claims wrong format of generated file, it doesn't matter which OS you're using. The file will be reported as wrongly formatted regardless.

[davidburstromspotify]

Version 5.8.2 works correctly, as far as I can tell.