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

Project build directory is not excluded when multiple targets are specified #835

Closed
bigdaz opened this issue Apr 7, 2021 · 4 comments · Fixed by #838
Closed

Project build directory is not excluded when multiple targets are specified #835

bigdaz opened this issue Apr 7, 2021 · 4 comments · Fixed by #838
Labels

Comments

@bigdaz
Copy link
Contributor

bigdaz commented Apr 7, 2021

The FormatExtension.parseTargetIsExclude() method provides special handling to exclude .git, .gradle and project build output directory from processing, when the target matches the pattern **/*.... This works well to avoid adding a bunch of spurious files to the Spotless inputs.

Unfortunately, this handling is limited to the case where a single target is specified. So if a formatter is configured with target('**/*.kt', '**/*.kts') then the project build output is not excluded from the inputs.

In Gradle versions < 7.0, this is inefficient but does not result in any user errors/warnings.
In Gradle 7.0, this is more problematic: the build output directory is effectively an input to the Spotless task, resulting in a warning like this:

> Task :app:spotlessKotlinGradle
Execution optimizations have been disabled for task ':app:spotlessKotlinGradle' to ensure correctness due to the following reasons:
  - Gradle detected a problem with the following location: '/Users/foo/XXX-Android/app'.
     Reason: Task ':app:spotlessKotlinGradle' uses this output of task ':app:spotlessJava' without declaring an explicit or implicit dependency. 
     This can lead to incorrect results being produced, depending on what order the tasks are executed. 
     Please refer to https://docs.gradle.org/7.0-rc-2/userguide/validation_problems.html#implicit_dependency for more details about this problem.

To reproduce this issue:

Here is a build scan that results from this execution: https://scans.gradle.com/s/zw2f7glrsko7s/deprecations. Note that this does not include the "Execution optimizations have been disabled" message, but this shows on the console.

@bigdaz
Copy link
Contributor Author

bigdaz commented Apr 7, 2021

@nedtwigg I am willing to contribute a fix for this issue, but I'm not quite sure what behaviour is desired. I think excluding the .git and .gradle directories almost always makes sense, and excluding the build output directory makes sense in the majority of cases. I'm thinking something like:

  1. If no target starts with .git/ then we ignore the .git directory
  2. If no target starts with .gradle/ then we ignore the .gradle directory
  3. If no target explicitly includes the build output directory, then we exclude this directory.

There's still the case like target('**/*.java', 'build/generated-src/*.scala'), where the project output directory would not be excluded. Hopefully we can find a way to exclude all of build except for the explicit include. Let's see.

Thoughts?

@nedtwigg
Copy link
Member

nedtwigg commented Apr 7, 2021

I like your fix, but I think there is a simpler fix which works iff FileCollection.plus/minus is hierarchical:

  • handle target(List<Sting> by creating a FileCollection per string, with the existing logic we have right now
  • union them together at the end

Basically do a more strict "map reduce" with map String -> FileCollection, reduce FileCollection::plus.

Does ('**/*.java' minus 'build/**') plus 'build/generated-src/*.scala' respect the parentheses? Or does it end up as '**/*.java' plus 'build/generates-src/*.scala' minus 'build/**' === '**/*.java' minus 'build/**'?

@nedtwigg nedtwigg added the bug label Apr 7, 2021
@bigdaz
Copy link
Contributor Author

bigdaz commented Apr 7, 2021

I like that simpler plan. I'm pretty sure that each file collection in a union has it's own set of includes and excludes, and I'll confirm how this interacts with the new "implicit dependency check" feature in Gradle 7.

bigdaz added a commit to bigdaz/spotless that referenced this issue Apr 8, 2021
This commit broadens the set of patterns that are considered "recursive", and thus
have automatic excludes applied for `.git`, `.gradle` and `build`.

Any target starting with `**/` is now considered recursive.

Fixes diffplug#835
@nedtwigg
Copy link
Member

Released in plugin-maven 2.10.0 and plugin-gradle 5.12.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