Fixed the merge conflict and enhanced NoHistoryArtifactState.getOutputFiles() a bit.

I changed EmptyFileSet and EmptyFileCollection into proper singletons.

Both are currently private static inner classes of the package-private NoHistoryArtifactState which is definitely OK for now. The question remains if they would be useful in other contexts in which case they would have to be moved somewhere else.

I guess that NoHistoryArtifactState should also be a singleton but I don't know future plans regarding that class. It would certainly reduce GC pressure to always return the same instance instead of new instances over at ShortCircuitTaskArtifactStateRepository.

49d7688 broke this PR again.

TaskExecutionHistory.getOutputFiles() has been replaced by TaskExecutionHistory.getOutputFiles(String propertyName), preventing me from doing a general FileCollection outputFiles = repository.getStateFor(task).getExecutionHistory().getOutputFiles();

This changed method is used by SimpleStaleClassCleaner and the propertyName argument is set manually, per task (e.g. return new SimpleStaleClassCleaner(taskOutputs, "destinationDir"); in CleaningGroovyCompiler).

I have no idea how to find correct propertyName value(s) for given tasks in a generic way and it appears like no such way exists at the moment.

So, unfortunately, I see no way to fix my PR anymore.

@lptr (author of 49d7688) & @eljobe (current GRADLE-2579 assignee): Please let me know if this PR is relevant at all to you.

If it is then please let me know how to retrieve the above propertyName in a generic, non-hardcoded way. Alternatively, you could restore the previously available, general TaskExecutionHistory.getOutputFiles() method.

I just need a way to retrieve any files previously generated by a given task so I can delete them if sourceFiles of said task have become empty.

@huxi: sorry for the long silence on this one. We definitely want to get the underlying problem fixed, and your PR provides some good basis to build on. I will have time to look into it in more detail in two weeks when I come back from vacation. Meanwhile, if you want to get it to work, you can use TaskOutputsInternal.getFileProperties() to see what properties there are.

@lptr No problem, I know you were all pretty busy with getting 3.0 out of the door.

TaskOutputsInternal.getFileProperties() won't help (I guess) because I need the previously generated outputs from the history, not the outputs of the current task.

But we can sort this all out when you are back. Have a nice vacation!

@huxi: you are right. I just reverted that change, see b69d831.

OK, thanks. Now enjoy your vacation! 😃

One problem we still need to look into about this PR is that it calls repository.getStateFor(task), which is an expensive operation. If you look at SkipUpToDateTaskExecuter.execute(), you can see that the information you need is looked up and stored there already, but it happens later in the chain of TaskExecuters. One thing that could be done is to move the lookup into a separate (otherwise no-op) TaskStateLookupTaskExecuter or something that does the lookup and storing of data, so it is available in all the executers further down the chain.

Also, vacation won't start until the weekend. :)

@lptr I fixed that problem. Nice side effect is better separation of concerns in SkipUpToDateTaskExecuter.

Can't do the ./gradlew quickCheck or ./gradlew quickCheck core:check at the moment since current HEAD explodes in buildSrc for me. I'll try to execute them a bit later. The related tests work in IDEA, though.

I also extended SkipEmptySourceFilesTaskExecuterTest for my use case and tightened the mock call checking.

@lptr TL;DR ./gradlew quickCheck :core:check works.

The strange error in buildSrc was caused by a corrupted ~/.gradle/cache. Deleting the cache fixed that problem.

After that, I got failing tests in core, e.g. OutputFilesCollectionSnapshotterTest. It took me a moment to realize that this test doesn't exist anymore. Performing a ./gradlew clean and restarting the build fixed it.

So it looks like I was bitten by GRADLE-2579 while fixing GRADLE-2579. I love the irony! 😃

Any news about this?

I will have time to work on this soon (this week or next week).

@huxi: sorry about the long delay again. I took your changes here, squashed them, and pushed them straight to master with some fixes. I hope it's alright. I'm closing this PR now.

Oh, and thank you for the contribution, and especially thanks for not letting this drop below the radar!

@lptr You are very welcome! This will be included in 3.2, right?

Yes, it is on track for being included in 3.2.

@lptr Awesome! You should set GRADLE-2579, GRADLE-2440 and GRADLE-3520 to fixed/resolved, though.

I seriously consider engraving a small tombstone for this issue. :)

:)

I didn't see GRADLE-3520 yet, thanks. Will close all those once the fix is through internal reviews.

Well, turns out GRADLE-3520 is not related after all. Even so, we've managed to resolve the other two, so still a great result. :)

Ah, I see the difference. It looked like a duplicate on first glance.