Declare inputs and outputs to support up-to-date checking

[oehme]

The spotless check task currently doesn't have any inputs and outputs declared and is thus never considered up-to-date. This is slowing down user's builds unnecessarily.

[nedtwigg]

The input is the source code, and the output is also the exact same source code. Can gradle handle this case?

[oehme]

I'm only talking about the check task. The update task cannot be made incremental, for exactly the reason you said.

[nedtwigg]

My largest project has 6.87MB of java source, spread across 1636 files (in many subprojects, of course :). It takes 5.6 seconds to sum their filesizes with this code in my buildscript:

println 'numFiles = ' + files.size()
long start = System.currentTimeMillis()
long totalSize = 0;
for (File f : files) {
    totalSize += f.length()
}
long elapsed = System.currentTimeMillis() - start
println 'filesize_MB=' + totalSize / 1_000_000
println 'elapsed_ms=' + elapsed

Running spotlessCheck on these files with these rules:

spotless {
    java {
        licenseHeaderFile
        importOrderFile
        eclipseFormatFile
        custom 'AutoValue fix', { it.replace('autovalue.shaded.', '') }
    }
}

Takes 38 seconds with a fresh daemon, 16 seconds on the second run, and levels out around 14s.

Given that it takes 6 seconds just to sum their filesizes, I think adding incremental support can, at best, take ~8-10 seconds off of a very large check task, which is about half. It'll be much much faster than any reasonable set of unit tests on a codebase this size.

@oehme How does gradle determine up-to-date? Does it trust filesize+lastModified, some kind of inotify, or take a hash?

The hardest part will be serializing custom input functions. In the example above, if the user changes the AutoValue fix function, we want to be sure to recalculate the "clean" state. If we don't have a good way to do that, then we introduce a minor staleness bug. If spotlessCheck is very slow, and the up-to-date checking is very fast, then it would be worthwhile. But unless gradle's up-to-date checking is doing something very clever like hooking into filesystem events, it seems to me that it's not worth the complexity.

[oehme]

That code you showed is too simplistic. Gradle can up-to-date check projects with a 100000 source files in about 5s. We have performance tests for that. Also, in the future Gradle will even do this pre-emptively (through file system listeners), so that it already knows the up-to-date state before you even run the next build.

The problem is that the spotless check task will run even if the source code for a project did not change. Imagine you only changed one of your projects, but spotless still insists on re-running on every project. That's a big waste of time. There really is no good reason not to do up-to-date checking ;)

You can hash custom functions by hashing their byte code (which you can get from their ClassLoader).

[oehme]

I just saw that spotless adds a task to the root project which checks the complete source tree. This is so much unnecessary work on incremental builds. That calls for a redesign towards much more granular tasks. The plugin should add a task for every sourceSet. If the user wants to check stuff outside of the sourceSets, he can add another check task that only checks some specific files (let's say, the build.gradle files in his gradle folder).

Basically the extension defines how to check (including the file extension, but not the full path). The task decides what to check.

[nedtwigg]

spotless adds a task to the root project

It doesn't add a task to the root project, just the project. You can add it to the root project (I often do), or you can add it to each subproject independently if you prefer.

and then checks the complete source tree

It is true that, by default, it creates one spotlessJavaCheck task for all of a single project's sourceSets, rather than creating a task per sourceSet. That's because most people want to have the same format for all of their sourceSets. If the user wants separate formatting rules for different sourceSets, that's easy to do too, but I don't see a reason to make that the default behavior.

I don't understand why one task with 500 files would be faster than two tasks with 250 files each, with or without incremental build support, so I don't understand why this causes unnecessary work. But if it does, and we need to redesign, I'm open to that :)

The problem is that the spotless check task will run even if the source code for a project did not change. Imagine you only changed one of your projects, but spotless still insists on re-running on every project.

Imagine you want to calcualte the sum of 1000 numbers, and rather than caching each incremental sum, you just recalculate the whole sum when any number changes. Supporting incremental processing is only a win if the processing is more expensive than tracking the changes. Spotless is very simple, and so it is very fast - the bottleneck is disk IO.

Until we can measure that something is a bottleneck, and we can measure that we have sped it up, I don't think we can justify adding complexity. I'm open to the idea that my benchmark is naive, but I don't see how yet. My machine is an old laptop with a magnetic disk, and it takes 6 seconds to look at the metadata of 1600 files, and 14 seconds to read them from disk, format them, and check that they are clean. If your build server has an SSD that can read the metadata for 100,000 files in 5 seconds, maybe it's also powerful enough to format them in 10 seconds?

You can hash custom functions by hashing their byte code

That's a clever idea, I like it! When gradle implements filesystem listeners, I could maaaaybe see that it might be worth the complexity of implementing this feature. But for now, I don't understand why users would experience a faster build, because I don't understand how gradle can check for up-to-date in less time than my naive benchmark.

[oehme]

That's because most people want to have the same format for all of their sourceSets.

Sure, the format would be specified in the extension, which all tasks reuse. But each sourceSet should have its own task. When I only change my test code, I don't want to re-check the main code. It's a waste of time.

I don't understand why one task with 500 files would be faster than two tasks with 250 files each, with or without incremental build support

You are thinking way too much in terms of full builds. Gradle is all about incremental builds. When I change a single source file in a single project, especially with continuous build, I expect a response in a few hundred milliseconds, not several seconds or more.

Imagine you want to calcualte the sum of 1000 numbers, and rather than caching each incremental sum, you just recalculate the whole sum when any number changes. Supporting incremental processing is only a win if the processing is more expensive than tracking the changes. Spotless is very simple, and so it is very fast - the bottleneck is disk IO.

I won't get into an argument here, just test it yourself and you'll be suprised how much faster up-to-date checking is than you think

task foo() {
 inputs.files fileTree('someLargeDirectory')
 outputs.file file('dummy')
 doLast {
   //do nothing, just seeing how fast up-to-date checks are
 }
}

I did that on a 256MB tree containing 100000 source files. ./gradlew foo takes 1.3s.

Until we can measure that something is a bottleneck, and we can measure that we have sped it up, I don't think we can justify adding complexity.

As I showed you this is trivial to measure. Also, you are talking as if adding incremental build support was some monumental task. For a simple plugin like this, it's a weekend project at best.

[oehme]

Running spotless on that same filetree takes 13s when I change a single file. That's 10 times longer! Imagine how much faster it would be if it only checked that single file. Gradle makes that trivial.

The config was very simplistic, so this gets worse as you add more rules:

spotless {
    format 'native', {
        target '**/*.cpp', '**/*.c', '**/*.h'

        customReplace      'Not enough space after if', 'if(', 'if ('
        customReplaceRegex 'Too much space after if', 'if +\\(', 'if ('
    }
}

[nedtwigg]

Imagine how much faster it would be if it only checked that single file

We don't have to imagine, we can calculate it ;-) It would be ~12s seconds faster on a 256MB source tree. On a 26MB source tree it would presumably be ~1.2 seconds faster. The biggest source tree I use in my humble world is just 7MB, though my laptop seems to be a lot crummier than yours ;-)

If spotless is consuming 10% of a build's duration, then a 10x speedup in spotless is a 9% build speedup.

I'd be happy to merge a PR. To me, this data confirms that spotless isn't slow enough for this optimization to make it to the top of my weekend project priority queue, at least not on a "utility" basis. But it would be a fun project to learn about or demonstrate Gradle's incremental APIs! If anyone is interested in doing this project for that purpose, go for it!

After thinking about the custom function issue, it doesn't bother me that much anymore - it's fine if changing a custom function doesn't cause spotlessCheck to recalculate. Changes to these functions will almost certainly be tested using spotlessApply, so it's okay if spotlessCheck is broken in this narrow way.

[oehme]

I corrected the number above. It is 10s slower or in other words 10 times as slow.

And remember this is for a trivial set of replacements. I'm sure it's way worse if you add more. I'll give you some more numbers later.

[nedtwigg]

Currently, this is the only task in the project, FormatTask. If anybody has time and need to make the change, I'm happy to merge and release the PR. Clone this repo, run gradlew ide, and you've got a dev environment. Here's the gradle docs on incremental builds.

I'd guess that making this change might require creating a separate FormatApplyTask and FormatCheckTask task, but if there's a way to avoid that it would be better to be able to release as 2.1.0 rather than 3.0.0, but whatever the code needs :)

[oehme]

I will be happy to assist the contributor with this. It will definitely require a 3.0 to do this right.

We should fix some other non-idiomatic Gradle usages while doing this. For instance the formats DSL and how the plugin creates tasks from it is not very user friendly. There are lots of special characters needed like quotes, commas and parentheses. It just looks very different from other Gradle APIs. Also, the task is not available except using afterEvaluate which makes customization look awkward. This can easily be fixed by following the same patterns that all Gradle core plugins use for their DSL. And if we're breaking API to fix the tasks, we should take this into account too.

We should track that in its own issue, but assign it to the same 3.0 milestone. @nedtwigg can you set that up? :)

Here's my design for a Gradle-idiomatic DSL for spotless. The plugin would create a pair of check/apply tasks for every 'target'. This will make the plugin faster, more flexible and easier to extend.

spotless {
  //formats specify how to format certain kinds of code
  formats {
    // by using the named container API you get stuff like 'all' for free :)
    all {
     lineEndings 'GIT_ATTRIBUTES'
    }
    //java is a special method that returns a more specific type
    java {
      lineEndings 'UNIX'
    }
    //so is freshmark :)
    freshmark {

    }
   //custom formats look just like pre-defined ones, no Strings or commas
    misc {
      //the format only specifies the file extension it cares about
      //which filetrees to process is decided on the target level
      extensions '.gradle', '.md', '.gitignore'
      //use boolean assignment instead of no-arg methods to
      //avoid as many parentheses as possible and also making it
      //easy to turn off again
      trimTrailingWhitespace true
      indentWith '\t'
      endWithNewline true
      step('mySpecialStep') {
        //imperative code here
      }
    }
  }
  //targets select file trees to scan and optionally which formats to apply
  targets {
    buildFiles {
      dir(projectDir) //careful here, we don't want to recurse through the whole project ;)
      fileTree("${projectDir}/gradle")
      //optional, default is all formats
      formats formats.misc
    }
    //when the java plugin is applied, it automatically adds a target
    //for every sourceSet of the project
  }
}

[nedtwigg]

(Unrelated aside: setting the lineEndings property is almost always a bad idea now that spotless supports gitattributes, see here for explanation)

I've created issue #32 to discuss your proposed DSL change.

We can definitely implement up-to-date checking and incremental support without changing the DSL, they're almost unrelated. Even if we decide to break the existing DSL, we should first release a version which adds incremental support for people who can't immediately make the jump to 3.0.0.

Note to potential contributors: I will quickly merge and release any PR's which add up-to-date support or incremental build, but please don't break the DSL.

[oehme]

We will need to break up the tasks into two different types, so that will be a breaking change. But yes, we don't need to change the spotless extension DSL to get incremental builds.

Breaking up the DSL to create more fine grained tasks will make incremental builds even better though, that's why I mentioned it here.

[jbduncan]

I'd be interested in looking into this.

As I'm new to writing Gradle plugins, I'd have to study the incremental tasks docs and delve into the Spotless source code to wrap my head around things, so I'll let you know both know if I manage to produce something useful that could act as a good starting point for this.

[oehme]

@jbduncan Cool! Note that the plugin currently scans the whole project dir, which includes stuff like the Gradle cache directory and the build directory. So you'll need to to apply at least some basic filters to its inputs before up-to-date checking will kick in.

[jbduncan]

Hi @oehme, many thanks for your response! It's not clear to me how I should go about implementing these filters you suggested, so I wonder if you could point me towards some resources which I could use to learn more about them?

[oehme]

Have a look at the user guide section on file trees :)

[jbduncan]

Thanks @oehme!

I've made an initial attempt at this in #45, but there's a few things which I'm confused about, and I wonder if you can help me.

Firstly, I don't really know if I've understood the docs on incremental tasks, and so I don't know if I've used the annotations and IncrementalTaskInputs class correctly, and I wondered if you could give me some constructive feedback.

Secondly, the tests no longer compile, but it's not clear to me what I should do to make them compile again (something about needing IncrementalTaskInputs parameters, but I don't know if I should mock them or if there are proper test impls that I can use), so I wondered if you can point me in the right direction.

And finally, it's not clear to me how and where I should use getProject().fileTree(...) in the code to trigger up-to-date checking, and I wondered if you could point me in the right direction with this too.

[oehme]

Firstly, I don't really know if I've understood the docs on incremental tasks, and so I don't know if I've used the annotations and IncrementalTaskInputs class correctly, and I wondered if you could give me some constructive feedback.

I'll leave a few comments in the PR. Generally, the annotations are the first step, so the task can be up-to-date checked. Using IncrementalTaskInputs is the icing on the cake :) Then the task only processes the inputs that actually changed.

Secondly, the tests no longer compile, but it's not clear to me what I should do to make them compile again (something about needing IncrementalTaskInputs parameters, but I don't know if I should mock them or if there are proper test impls that I can use), so I wondered if you can point me in the right direction.

Generally I don't unit-test task classes, because they rely on quite a bit of Gradle infrastructure. Instead, I extract the processing logic into a separate class and unit-test that one. If you do want to unit-test them, then mocking the IncrementalTaskInputs is probably the way to go.

And finally, it's not clear to me how and where I should use getProject().fileTree(...) in the code to trigger up-to-date checking, and I wondered if you could point me in the right direction with this too.

I might have been a little unclear here. It does not trigger up-to-date checking. It's just that checking the whole project dir without a filter would include constantly changing things like the Gradle cache and the build directory.

The problematic code is where the spotless plugin sets up the targets. This should have some default excludes (cache dir, build dir).

[jbduncan]

Generally I don't unit-test task classes, because they rely on quite a bit of Gradle infrastructure. Instead, I extract the processing logic into a separate class and unit-test that one. If you do want to unit-test them, then mocking the IncrementalTaskInputs is probably the way to go.

It's not clear to me yet if I do want to unit-test the task classes, but it seems like the path of least resistance ATM, so I'll go down that route for now, and if I see a way of improving things later, I'll improve them then. :)

[jbduncan]

Thanks again for your feedback @oehme. I've realised I'm now stuck again, as one of the tests is failing and I'm struggling to see how to make things go green again, and I wonder if you or @nedtwigg can help me move forward.

I pushed my most recent code snapshot to 6061363.

[nedtwigg]

I grabbed your code and responded in the PR.

[jbduncan]

Hmm, I'm feeling rather lost now. I understand there's been further discussion at #45 and #47, but I've lost track of how it all fits together, and there's even some parts which my mind is just refusing to parse. Consequently I've not understood what it is I'm supposed to be doing now to move forward with #45.

@nedtwigg @oehme Would one of you kindly give me a summary of what's been discussed and explain what I should do next? :)

[nedtwigg]

Just pulled your latest, tests pass, great work!

The core wrinkle is that all FormatterSteps need to support Serializable/equals/hashCode, and we don't have a great way to do that for custom steps. We talked this through here and @oehme convinced me that his way was best. #47 is the one open wrinkle, we'll have to figure out what we think about it to completely close it out.

Also, I just realized another hard part - LineEnding.Policy. By default, we use GitAttributes, which depends on every .gitattributes file in the repo, as well as system-wide config files. So we've gotta make LineEnding.Policy be Serializable, and figure out how to implement that for GitAttributes.

I made some unrelated changes causing a conflict - I'm merging master into your branch and I'll upload the change and come back again with a to-do list we can work through together.

[nedtwigg]

Just uploaded the merge and some minor cleanup. We need to make all of the following things properly serializable/equalsTo/hashCode. Right now a lot of these are lazily evaluated, and a naive serializable implementation will make it more eager than is necessary. Maybe that's unavoidable, depending on how gradle handles the @Input annotations - we'll pick @oehme's brain as we go ;-)

• GitAttributesLinePolicy
• GoogleJavaFormat
• EclipseFormatterStep
• IndentStep
• LicenseHeaderStep
• FreshMarkExtension constructor
• FormatExtension::customReplace
• FormatExtension::customReplaceRegex

Depending on what we decide on #47, we can either ignore these completely, or at least handle them piece-by-piece. We'll need some infrastructure for making the serialization easy, and for easily testing that the serialization is working. I've got some ideas for this, but I gotta run. I'll upload these ideas later today, feel free to upload other ideas too :)

Based on how hard it is to actually do this, maybe we'll consider bailing on this approach and using the full shortcut presented by #47.

[nedtwigg]

I've uploaded a few commits. They have long commit messages that tell the story, but here's the outline:

1. 7b6aea7, 6113458, 452fbd9, c1357f5 build up to FormatterStep.Strict which puts a hard wall between the serializable state of a rule, and the pure logic which applies that to the files.
2. 51f0531 uses this framework to rewrite LicenseHeaderStep.
3. eefdc50 is an implementation of the idea in Quick up-to-date checking for custom rules (sub to #31) #47

Based on this, I think that making every built-in rule properly support up-to-date checking is fairly straightforward (though it's definitely a lot of work!).

If it looks like a generally good idea to you guys, then I think we should merge this to a new branch 3.x and start implementing all these things as their own PRs. It also desperately needs some integration testing to ensure that the incremental builds are running and not running as we expect.

[oehme]

You can use TestKit to verify that your tasks are executed/skipped as expected.

Gradle evaluates inputs just before a task is executed, so no worries about eagerness.

Also don't sweat about the Serializability. We don't need cross machine/cross version compatibility. This is just about local caching and if serialization breaks with a new version then Gradle will just reexecute the task.

Literally just slapping implements Serializable on those classes should work in almost any case.

[nedtwigg]

FormatterSteps are independent of the files they are transforming. They are dependent on anything which determines their configuration. So EclipseFormatter.State needs to have the file that holds the formatter properties file, but it does not need the target files that it is transforming.

For FreshMark, the key should be the Map<String, String> of properties that it is using.

The FormatTask tracks which files are being transformed, not the FormatterSteps.

[jbduncan]

FormatterSteps are independent of the files they are transforming. They are dependent on anything which determines their configuration. So EclipseFormatter.State needs to have the file that holds the formatter properties file, but it does not need the target files that it is transforming.

Ahh, okay, I see now. Thanks for clearing up my confusion on this.

For FreshMark, the key should be the Map<String, String> of properties that it is using.

Hmm, it seems the default properties are not serializable, so the closest I can get to a version of properties which can be used as a key, is with the following code snippet.

addStep(FormatterStep.createLazy(NAME,
		() -> {
			// defaults to all project properties
			if (properties == null) {
				properties = getProject().getProperties();
			}
			// properties isn't serializable, so make a serializable copy
			return new LinkedHashMap<>(Maps.transformValues(properties, value -> (value == null) ? null : value.toString()));
		},
		key -> new FreshMark(key, getProject().getLogger()::warn)::compile));

The consequence of this solution is :spotlessFreshmarkCheck never reports as being UP-TO-DATE...

[nedtwigg]

Looks like we'll need a Freshmark.State class. Mapping them all to Map<String, String> is a great fix for up-to-date checking, but it's possible for some props to be actual java objects that need to be passed as the objects themselves, not string. So Map<String, ?> can be transient, and Map<String, String> can be the actual prop.

Possible cause 1: LinkedHashMap<> cares about order, and whatever gradle is using doesn't, so they're getting reordered randomly.
Possible cause 2: There's a property like "currentTime: Date". Try some debug code that dumps all the properties in getProject().getProperties() and see if any are changing. Depending on what it is, it might be appropriate for the default behavior of this method to be that it is never up-to-date unless properties are manually specified. Or, maybe instead of passig getProject().getProperties(), we should manually parse gradle.properties ourselves.

[oehme]

Gradle definitely doesn't reorder things randomly ;)

My bet is on: There is either a timestamp property or a property that doesn't implement toString(), so you just get the identity hash code which is different on every invocation.

The solution is pretty simple: The user should be explicit about what properties should be passed to FreshMark. The default should be none.

[nedtwigg]

It's very handy that everything in gradle.properties shows up in freshmark without much work. One solution is to add a method addProps(File propertiesFile). But I still lean towards filtering out the individual property which is causing the issue.

[oehme]

There is no such individual property. Every build out there probably has dozens of properties that either are some kind of timestamp or simply don't have a toString() method. The properties are used by build script authors as a bucket to put all kinds of data.

[nedtwigg]

We could check for unimplemented toString() using System.identityHashCode() and filter them out. Regardless, you're convincing me that adding all props is the wrong way to go :) I'm still curious which is the troublemaker.

[jbduncan]

When I print out the properties during a run of gradle check, they look like this:

properties = {
    parent: null
    classLoaderScope: org.gradle.api.internal.initialization.DefaultClassLoaderScope@1fd793ea
    plugins: [org.gradle.api.plugins.HelpTasksPlugin@4493592c, org.gradle.language.base.plugins.LifecycleBasePlugin@6c33e52a, org.gradle.api.plugins.BasePlugin@32382b0c, org.gradle.api.plugins.ReportingBasePlugin@e0c5757, org.gradle.platform.base.plugins.ComponentBasePlugin@34613fbe, org.gradle.language.base.plugins.LanguageBasePlugin@41fcc7ce, org.gradle.platform.base.plugins.BinaryBasePlugin@7dea052b, org.gradle.api.plugins.JavaBasePlugin@1fca0c52, org.gradle.api.plugins.JavaPlugin@7729233b, com.diffplug.gradle.spotless.SpotlessPlugin@182e787a]
    configurations: [configuration ':archives', configuration ':compile', configuration ':compileClasspath', configuration ':compileOnly', configuration ':default', configuration ':runtime', configuration ':testCompile', configuration ':testCompileClasspath', configuration ':testCompileOnly', configuration ':testRuntime']
    logger: org.gradle.internal.logging.slf4j.OutputEventListenerBackedLogger@2378b9ca
    rootDir: C:\Users\Jonathan\dev\Java\IntelliJ Projects\spotless
    projectRegistry: org.gradle.api.internal.project.DefaultProjectRegistry@7ee85ce6
    path: :
    testResultsDirName: test-results
    targetCompatibility: 1.8
    ...
    rootProject: root project 'spotless'
    libsDirName: libs
    artifactIdLib: spotless-lib
    properties: {parent=null, classLoaderScope=org.gradle.api.internal.initialization.DefaultClassLoaderScope@1fd793ea, ...}
}

I dare not paste all the properties, because the list is massive.

But looking through the first few props, it seems obvious to me that a number of properties simply do not implement toString() and print out their identities instead, as @oehme suspected.

I didn't notice any timestamp-related property as I was glancing through the list, so I suspect that the objects which don't implement toString() are to blame.

Therefore I'd be inclined to completely throw out the safeguard currently in place to set the properties to getProject().getProperties() if the user doesn't specify them themselves.

@nedtwigg Do you have any ideas as to what we could do instead? Just throw an exception? Or log it and/or perform some other action?

[oehme]

Therefore I'd be inclined to completely throw out the safeguard currently in place to set the properties to getProject().getProperties() if the user doesn't specify them themselves.

I think that's the wrong default. It'll always be out of date. It should be no properties by default and the user should conciously decide what she needs.

[nedtwigg]

I think I agree with @oehme. I think we should add a propsFile argument which can load a .properties file.

[jbduncan]

@nedtwigg I admit I don't really understand what you mean by adding a propsFile argument (it wasn't clear to me if you wanted me to add it to FreshmarkExtension::new's parameter list or another method's), but my current solution is to turn FreshmarkExtension.java, which currently contains:

public class FreshMarkExtension extends FormatExtension {
	public static final String NAME = "freshmark";

	public Map<String, ?> properties;

	public FreshMarkExtension(SpotlessExtension root) {
		super(NAME, root);
		customLazy(NAME, () -> {
			// defaults to all project properties
			if (properties == null) {
				properties = getProject().getProperties();
			}
			FreshMark freshMark = new FreshMark(properties, getProject().getLogger()::warn);
			return freshMark::compile;
		});
	}

	public void properties(Map<String, ?> properties) {
		this.properties = properties;
	}
	...
}

...into:

public class FreshMarkExtension extends FormatExtension {
	public static final String NAME = "freshmark";

	public FreshMarkExtension(SpotlessExtension root) {
		super(NAME, root);
	}

	public void properties(Map<String, ?> properties) {
		customLazy(NAME, () -> {
			FreshMark freshMark = new FreshMark(properties, getProject().getLogger()::warn);
			return freshMark::compile;
		});
	}
	...
}

My solution seems to pass all the tests on my machine.

What do you think?

[nedtwigg]

customLazy tasks will always be out-of-date. We need to capture the state of the Map<String, String>, and move FreshMark into lib, using same Provisioner construct as EclipseFormatter and GoogleJavaFormat. If I'm being confusing, that's my bad, I might be able to communicate better by just writing it :)

[jbduncan]

Ooh, yes please! If you could write it down, then I think there's a much higher chance that I'd understand. :)

[nedtwigg]

• Created FreshMark inside lib: a060ee4
• Refactored gradle's FreshMark to use better DSL: 8bb31d1

[jbduncan]

Hi @nedtwigg, I've been working on incrementalising ImportSorterStep over the last few days, but I've observed that when I run ./gradlew check twice on the root project (before and after I've stashed my local changes away with git stash), it throws an exception at :spotlessFreshmarkCheck, and I'm struggling to tell what's causing it, so I wondered if you'd be happy to have a look into it?

Here is an abbreviated example of the stack trace I get.

Exception in thread "main" org.gradle.testkit.runner.UnexpectedBuildFailure: Unexpected build execution failure in C:\Users\Jonathan\dev\Java\IntelliJ Projects\spotless with arguments [-b, spotlessSelf.gradle, spotlessCheck, --stacktrace]

Output:
:spotlessFreshmarkCheck FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':spotlessFreshmarkCheck'.
> java.nio.file.AccessDeniedException: C:\Users\Jonathan\dev\Java\IntelliJ Projects\spotless\plugin-gradle\build\reports

* Try:
Run with --info or --debug option to get more log output.

* Exception is:
org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':spotlessFreshmarkCheck'.
	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeActions(ExecuteActionsTaskExecuter.java:84)
	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.execute(ExecuteActionsTaskExecuter.java:55)
	at org.gradle.api.internal.tasks.execution.SkipUpToDateTaskExecuter.execute(SkipUpToDateTaskExecuter.java:61)
	...
	at org.gradle.launcher.daemon.server.DaemonStateCoordinator$1.run(DaemonStateCoordinator.java:293)
	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:54)
	at org.gradle.internal.concurrent.StoppableExecutorImpl$1.run(StoppableExecutorImpl.java:40)
Caused by: java.io.UncheckedIOException: java.nio.file.AccessDeniedException: C:\Users\Jonathan\dev\Java\IntelliJ Projects\spotless\plugin-gradle\build\reports
	at com.diffplug.gradle.spotless.CheckFormatTask.addFileIfNotClean(CheckFormatTask.java:61)
	at com.diffplug.gradle.spotless.CheckFormatTask.lambda$check$0(CheckFormatTask.java:38)
	at org.gradle.api.internal.changedetection.changes.ChangesOnlyIncrementalTaskInputs.doOutOfDate(ChangesOnlyIncrementalTaskInputs.java:46)
	at org.gradle.api.internal.changedetection.changes.StatefulIncrementalTaskInputs.outOfDate(StatefulIncrementalTaskInputs.java:39)
	at org.gradle.api.internal.changedetection.changes.ChangesOnlyIncrementalTaskInputs.outOfDate(ChangesOnlyIncrementalTaskInputs.java:27)
	at com.diffplug.gradle.spotless.CheckFormatTask.check(CheckFormatTask.java:38)
	at org.gradle.internal.reflect.JavaMethod.invoke(JavaMethod.java:73)
	at org.gradle.api.internal.project.taskfactory.DefaultTaskClassInfoStore$IncrementalTaskAction.doExecute(DefaultTaskClassInfoStore.java:163)
	at org.gradle.api.internal.project.taskfactory.DefaultTaskClassInfoStore$StandardTaskAction.execute(DefaultTaskClassInfoStore.java:134)
	at org.gradle.api.internal.project.taskfactory.DefaultTaskClassInfoStore$StandardTaskAction.execute(DefaultTaskClassInfoStore.java:123)
	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeAction(ExecuteActionsTaskExecuter.java:95)
	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeActions(ExecuteActionsTaskExecuter.java:76)
	... 71 more
Caused by: java.nio.file.AccessDeniedException: C:\Users\Jonathan\dev\Java\IntelliJ Projects\spotless\plugin-gradle\build\reports
	at com.diffplug.spotless.Formatter.isClean(Formatter.java:103)
	at com.diffplug.gradle.spotless.CheckFormatTask.addFileIfNotClean(CheckFormatTask.java:57)
	... 82 more


BUILD FAILED

Total time: 2.188 secs

	at org.gradle.testkit.runner.internal.DefaultGradleRunner$1.execute(DefaultGradleRunner.java:222)
	at org.gradle.testkit.runner.internal.DefaultGradleRunner$1.execute(DefaultGradleRunner.java:219)
	at org.gradle.testkit.runner.internal.DefaultGradleRunner.run(DefaultGradleRunner.java:282)
	at org.gradle.testkit.runner.internal.DefaultGradleRunner.build(DefaultGradleRunner.java:219)
	at com.diffplug.gradle.spotless.SelfTest.runWithTestKit(SelfTest.java:132)
	at com.diffplug.gradle.spotless.SelfTestCheck.main(SelfTestCheck.java:20)
:plugin-gradle:spotlessCheck FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':plugin-gradle:spotlessCheck'.
> Process 'command 'C:\Program Files\Java\jdk1.8.0_101\bin\java.exe'' finished with non-zero exit value 1

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 17.819 secs
Process 'command 'C:\Program Files\Java\jdk1.8.0_101\bin\java.exe'' finished with non-zero exit value 1
14:31:03: External task execution finished 'build'.

[nedtwigg]

I've been having this too. The fix is to delete the .gradle directory. Dunno the cause, but self-applying the gradle plugin to its own directory is pretty difficult, and quite janky. Might be possible to fix by making spotlessSelf.gradle more specific.

[jbduncan]

@nedtwigg Thanks for the advice, deleting .gradle seems to work nicely as a short-term fix.

I've now made the import sorter related classes ready for incremental builds: 5d365b3.

Hmm, having thought about it, I wonder if the reason the build fails at :spotlessFreshmarkCheck on a second run is because it fails to deserialize the Freshmark-related classes that are serialized on the first run...

(By my understanding, the serialized bits are saved in files within .gradle, so it would make sense to me that deleting .gradle would 'fix' the problem, as there would be nothing for Gradle to deserialize.)

[nedtwigg]

If that is the case, it should be fixed since d3994cf on 12/7, because freshMark no longer adds all the project properties.

[jbduncan]

Hi @nedtwigg, thanks for pointing that out. I've investigated things further, and I now think the exception occurs because it tries to read a file from the plugin-gradle build directory.

I don't know why it throws an exception there, and probably more importantly, it's not clear to me why it's even trying to read from the build directory in the first place, as I implemented a filter at some point which prevents (or, at least should prevent) Spotless from reading a Gradle project's build/ and .gradle/ dirs.

@oehme Do you have any thoughts regarding this?

[oehme]

As far as I can recall your code only excluded the current projects build dir. That doesn't stop it from recursing into subproject folders and reading their build directory.

[jbduncan]

Thanks @oehme! It looks like running ./gradlew check twice on Spotless no longer causes to an exception to be thrown, so I'd say it's fixed now! (See commit e3ec14f.)

[jbduncan]

Oh oops, spoke too soon. It only works if ./gradlew spotlessCheck is run twice, not ./gradlew check. I'll investigate this some other time...

[nedtwigg]

HUGE THANKS to @jbduncan for getting this built. Might be some loose ends still in here, but this issue is too big to be helpful any longer.

[jbduncan]

I'm reasonably certain are indeed some loose ends, I just don't know where they are. My reason for thinking this is when I last ran Spotless 3.0-SNAPSHOT against itself a few times 2 days ago, not all the Spotless related Gradle tasks were reporting as being "UP-TO-DATE".

I'd be happy to raise a new issue for tackling these loose ends when I'm not feeling so tired, if you so wish @nedtwigg. :)

[nedtwigg]

Solved part of the problem in c821e85. If you find any 3.0 blockers, by all means raise issues for them :)