Clean up stale output files from Java compilation after Gradle version change by bmuschko · Pull Request #1074 · gradle/gradle

bmuschko · 2016-12-27T17:33:55Z

Implementation for #1018.

…bled

For now buildSrc will delete the output directories registered by the JavaPlugin and not buildDir.

Many empty directories were left behind which caused the build output cleanup to kick in and produce unexpected output.

Build output registry needs to be define for a build session so it can be shared between the main build and buildSrc.

bmuschko · 2016-12-27T17:34:35Z

@wolfs @big-guy Looking for some early feedback. Thanks!

big-guy · 2017-01-09T22:01:05Z

Reviewed 34 of 34 files at r1.
Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

a discussion (no related file):
I think we should add some coverage for composite builds into this since we have a new listener that we need to make sure is triggered at the right times.

subprojects/core/src/main/java/org/gradle/cache/internal/FileLock.java, line 54 at r1 (raw file):

     */
    interface State {
        boolean canDetectChanges();

This seems a little wonky. What changes would a file lock detect?

subprojects/core/src/main/java/org/gradle/cache/internal/filelock/Version1LockStateSerializer.java, line 61 at r1 (raw file):

        public boolean canDetectChanges() {
            return false;

shouldn't this be based on whether or not we found a file lock state?

subprojects/core/src/main/java/org/gradle/initialization/buildsrc/BuildSrcBuildListenerFactory.java, line 35 at r1 (raw file):

    private static final String DEFAULT_BUILD_SOURCE_SCRIPT_RESOURCE = "defaultBuildSourceScript.txt";

    Listener create() {

I'm not sure if we can remove the previous "clean everything" behavior for buildSrc because we're not fixing all possible inconsistencies yet. So someone could have a custom plugin they use in buildSrc that will now be broken without a clean.

subprojects/core/src/main/java/org/gradle/internal/cleanup/BuildOutputCleanupCache.java, line 29 at r1 (raw file):

     *
     * - The task history is deleted and one or more input files are removed.
     * - The build upgrades or downgrades to a different Gradle version, wasn't executed with that version before and one or more input files are removed.

I'm not sure what this comment means. Does it mean that clean() doesn't clean-up stale output files in these conditions or that these are the conditions that stale files are left behind?

subprojects/core/src/main/java/org/gradle/internal/cleanup/BuildOutputCleanupListener.java, line 22 at r1 (raw file):

import org.gradle.initialization.ModelConfigurationListener;

public class BuildOutputCleanupListener implements ModelConfigurationListener {

could this happen any later? e.g., after the DAG has been created? I think we want to avoid doing work that will be counted as "configuration" time and it might also be useful to show something in the progress logger when we do this.

subprojects/core/src/main/java/org/gradle/internal/cleanup/DefaultBuildOutputCleanupCache.java, line 41 at r1 (raw file):

    public DefaultBuildOutputCleanupCache(CacheRepository cacheRepository, File cacheBaseDir, BuildOutputDeleter buildOutputDeleter, BuildOutputCleanupRegistry buildOutputCleanupRegistry) {
        this.cacheRepository = cacheRepository;
        this.cacheDir = new File(cacheBaseDir, ".gradle/noVersion/buildOutputCleanup");

I think this isn't quite right. This is supposed to be the project cache dir? The project cache dir can be something other than .gradle/, I think we're supposed to use a different CacheRepository method to make this cache Gradle or Project scope.

subprojects/core/src/main/java/org/gradle/internal/cleanup/DefaultBuildOutputCleanupRegistry.java, line 27 at r1 (raw file):

public class DefaultBuildOutputCleanupRegistry implements BuildOutputCleanupRegistry {

    private final List<File> outputs = new ArrayList<File>();

would this make sense as a Set?

subprojects/core/src/test/groovy/org/gradle/initialization/buildsrc/BuildSourceBuilderTest.groovy, line 53 at r1 (raw file):

    void "creates classpath when build src exists"() {
        def classpath = [new File('test')]

is it ok this is relative to the test JVM?

subprojects/core/src/test/groovy/org/gradle/internal/cleanup/BuildOutputCleanupCacheTest.groovy, line 31 at r1 (raw file):

import spock.lang.Specification

class BuildOutputCleanupCacheTest extends Specification {

does this need a @UsesNativeServices?

subprojects/core/src/test/groovy/org/gradle/internal/cleanup/DefaultBuildOutputDeleterTest.groovy, line 36 at r1 (raw file):

    def setup() {
        buildOutputDeleter.logger = logger

Maybe another/better way to do this (instead of relying on the logger sensing) is to make deleteOutput protected and override it here like:

def deletedFiles = []
def buildOutputDeleter = new DefaultBuildOutputDeleter() {
   void deleteOutput(File file) {
      deletedFiles << file
   }
}

Then you can assert that the correct things were deleted by inspecting deletedFiles.

subprojects/docs/src/samples/userguideOutput/incrementalBuildInputFilesConfig.out, line 1 at r1 (raw file):

:clean UP-TO-DATE

What change makes this up-to-date now?

subprojects/integ-test/src/integTest/groovy/org/gradle/integtests/StaleOutputHistoryLossIntegrationTest.groovy, line 33 at r1 (raw file):

import static org.gradle.util.GFileUtils.forceDelete

class StaleOutputHistoryLossIntegrationTest extends AbstractIntegrationSpec {

It'll be useful to see what does the failure look like when we can't properly clean-up the stale files.

subprojects/integ-test/src/integTest/groovy/org/gradle/integtests/samples/UserGuideSamplesRunner.groovy, line 140 at r1 (raw file):

                delete.includeEmptyDirs = true
                delete.addFileset(new FileSet(dir: rootProjectDir, includes: "**/.gradle/** **/build/**"))
                AntUtil.execute(delete)

any idea why we don't use our own Delete stuff here?

subprojects/plugins/src/main/java/org/gradle/api/plugins/JavaPlugin.java, line 208 at r1 (raw file):

    }

    private class SourceSetOutputCleanUpRegistrationListener implements ProjectEvaluationListener {

Now that I've seen how this fits with other things.. I'm wondering if we should make all of these ModelConfigurationListeners so that we don't have to worry about a different afterEvaluate adding more configuration later (ignoring the fact we also have configure tasks...).

subprojects/plugins/src/main/java/org/gradle/api/plugins/JavaPlugin.java, line 225 at r1 (raw file):

        @Override
        public void afterEvaluate(Project project, ProjectState state) {
            buildOutputCleanupRegistry.registerOutputs(main.getOutput().getClassesDir(), main.getOutput().getResourcesDir(), test.getOutput().getClassesDir(), test.getOutput().getResourcesDir());

Could we add a registerOutputs that accepts FileCollection, so this becomes:

buildOutputCleanupRegistry.registerOutputs(main.getOutput(), test.getOutput())

That's probably the other common output type we have.

Comments from Reviewable

big-guy · 2017-01-09T22:33:48Z

Sorry @bmuschko. I thought the comments would appear as normal. I've transferred them to the files on GH.

big-guy · 2017-01-09T22:28:30Z

     * An immutable snapshot of the state of a lock.
     */
    interface State {
+        boolean canDetectChanges();


This seems a little wonky. What changes would a file lock detect?

big-guy · 2017-01-09T22:28:53Z

        }

+        public boolean canDetectChanges() {
+            return false;


shouldn't this be based on whether or not we found a file lock state?

big-guy · 2017-01-09T22:29:29Z

-    Listener create(boolean rebuild) {
-        return new Listener(rebuild);
+    Listener create() {
+        return new Listener();


I'm not sure if we can remove the previous "clean everything" behavior for buildSrc because we're not fixing all possible inconsistencies yet. So someone could have a custom plugin they use in buildSrc that will now be broken without a clean.

I am not sure I quite understand this point. We are deleting the buildDir directory. That's what the clean task does. Are you saying that there could be some logic that hooks into the clean task which now wouldn't happen anymore?

big-guy · 2017-01-09T22:29:45Z

+     * Cleans stale output files from previous builds. Stale files are left behind under the following conditions:
+     *
+     * - The task history is deleted and one or more input files are removed.
+     * - The build upgrades or downgrades to a different Gradle version, wasn't executed with that version before and one or more input files are removed.


I'm not sure what this comment means. Does it mean that clean() doesn't clean-up stale output files in these conditions or that these are the conditions that stale files are left behind?

These are the conditions under which stale files/directories are left behind as indicated by the previous sentence. I'll move it to a new line to make it more clear.

big-guy · 2017-01-09T22:29:58Z

+import org.gradle.api.internal.GradleInternal;
+import org.gradle.initialization.ModelConfigurationListener;
+
+public class BuildOutputCleanupListener implements ModelConfigurationListener {


could this happen any later? e.g., after the DAG has been created? I think we want to avoid doing work that will be counted as "configuration" time and it might also be useful to show something in the progress logger when we do this.

big-guy · 2017-01-09T22:31:46Z

@@ -1,4 +1,4 @@
-:clean
+:clean UP-TO-DATE


What change makes this up-to-date now?

This is related to delete.includeEmptyDirs = true. We didn't delete the directory before, only the contents of the directory. The sample is executed multiple times via UserGuideSamplesRunner.

big-guy · 2017-01-09T22:32:26Z

+import static org.gradle.integtests.fixtures.StaleOutputJavaProject.*
 import static org.gradle.util.GFileUtils.forceDelete

 class StaleOutputHistoryLossIntegrationTest extends AbstractIntegrationSpec {


It'll be useful to see what does the failure look like when we can't properly clean-up the stale files.

I tested this use case in DefaultBuildOutputDeleterTest.logs warning if file cannot be deleted - not necessarily on the functional test level but one level lower. Do you think that's sufficient?

big-guy · 2017-01-09T22:32:40Z

-                delete.dir = rootProjectDir
-                delete.includes = "**/.gradle/** **/build/**"
+                delete.includeEmptyDirs = true
+                delete.addFileset(new FileSet(dir: rootProjectDir, includes: "**/.gradle/** **/build/**"))


any idea why we don't use our own Delete stuff here?

We are directly invoking the Ant task here: AntUtil.execute(delete). It's not a practice we promote in Gradle as it would mean calling the execute() method explicitly. Other than that we don't we have a method in GFileUtils or FileUtils that accepts a pattern.

big-guy · 2017-01-09T22:33:06Z

        }
    }
+
+    private class SourceSetOutputCleanUpRegistrationListener implements ProjectEvaluationListener {


Now that I've seen how this fits with other things.. I'm wondering if we should make all of these ModelConfigurationListeners so that we don't have to worry about a different afterEvaluate adding more configuration later (ignoring the fact we also have configure tasks...).

If registration would support somewhat the same things as org.gradle.api.Project#files(java.lang.Object...) - i.e. some lazy evaluation - we could register the outputs right when we apply the plugin and evaluate them whenever we want to delete them. If we don't do that then we have the problem to decide when we want to evaluate the outputs. We would only evaluate all those paths when we really need to clean. WDYT?

big-guy · 2017-01-09T22:33:17Z

+
+        @Override
+        public void afterEvaluate(Project project, ProjectState state) {
+            buildOutputCleanupRegistry.registerOutputs(main.getOutput().getClassesDir(), main.getOutput().getResourcesDir(), test.getOutput().getClassesDir(), test.getOutput().getResourcesDir());


Could we add a registerOutputs that accepts FileCollection, so this becomes:

buildOutputCleanupRegistry.registerOutputs(main.getOutput(), test.getOutput())

That's probably the other common output type we have.

I removed the unused method that takes List<File> in favor of FileCollection. Good idea.

wolfs

As I understand it we would now cleanup the main and test classes directory after a version upgrade and cleanup the buildSrc/build directory, too. This would fix at least the most pressing problems.
With the current data captured for outputs we currently cannot address the empty sources problem since this could happen anytime even without a Gradle version change. That's OK for now.
I am not sure if we should explore deleting the directories any later than the end of configuration phase or if we also would leave that for a later PR. WDYT?

wolfs · 2017-01-11T22:54:17Z

+    }
+
+    private String createMessage(String type, File output) {
+        return String.format("Cleaned up %s '%s'", type, output);


Instead of using String.format can't we just use the message "Cleaned up {} '{}'" and then pass the two parameters to the logger.quiet method?

wolfs · 2017-01-11T22:54:43Z

+                logger.quiet(createMessage("file", output));
+            }
+        } catch (UncheckedIOException e) {
+            logger.warn(String.format("Unable to clean up '%s'", output));


Instead of using String.format can't we just use the message "Cleaned up {} '{}'" and then pass the two parameters to the logger.warn method?

wolfs · 2017-01-11T22:59:11Z

-    @NotYetImplemented
    @Issue("GRADLE-1501")
-    def "production sources files are removed"() {
+    @Unroll


You can put @Unroll on the class level.

wolfs · 2017-01-11T23:03:48Z

+            }
+        """
+
+        file('settings.gradle') << "include ${(1..projectCount).collect { "'${createProjectName(it)}'" }.join(',')}"


It is possible to create a multiproject build with a correct settings.gradle by using org.gradle.integtests.fixtures.AbstractIntegrationSpec#multiProjectBuild(java.lang.String, java.util.List<java.lang.String>, groovy.lang.Closure).

wolfs · 2017-01-11T23:07:59Z

+        arguments                                              | description
+        [JAR_TASK_NAME]                                        | 'without additional argument'
+        [JAR_TASK_NAME, '--parallel']                          | 'in parallel'
+        [JAR_TASK_NAME, '--parallel', '--configure-on-demand'] | 'in parallel and configure on demand enabled'


For me it would be interesting to see what happens when we apply the java plugin in a separate build file (not the top level build file but a separate one in a subproject) and then only run the jar task in one of the subprojects. I would suppose that then only that classes directory gets cleaned while when running the jar task in one of the other projects after that the stale output files are still present.

wolfs · 2017-01-12T09:00:42Z

        }
    }
+
+    private class SourceSetOutputCleanUpRegistrationListener implements ProjectEvaluationListener {


If registration would support somewhat the same things as org.gradle.api.Project#files(java.lang.Object...) - i.e. some lazy evaluation - we could register the outputs right when we apply the plugin and evaluate them whenever we want to delete them. If we don't do that then we have the problem to decide when we want to evaluate the outputs. We would only evaluate all those paths when we really need to clean. WDYT?

wolfs · 2017-01-12T09:19:45Z

+        !result.output.contains(javaProject.classesDirCleanupMessage)
+        javaProject.mainClassFile.assertIsFile()
+        javaProject.redundantClassFile.assertIsFile()
+        hasDescendants(javaProject.jarFile, javaProject.mainClassFile.name, javaProject.redundantClassFile.name)


We are asserting the same things over and over again. Can't we extract those to assertions on StaleOutputJavaProject?
We could have something linke javaProjectFixture.assertBuiltWithRedundantFile, javaProjectFixture.assertBuiltWithoutRedundantFile and javaProjectFixture.assertCleanupExecuted/assertCleanupNotExecuted. WDYT?

wolfs · 2017-01-12T09:27:57Z

+package org.gradle.internal.cleanup;
+
+import java.io.File;
+import java.util.List;


Unused import.

big-guy · 2017-01-19T07:23:24Z

OBE, a version of this was pushed to release (for 3.4)

Benjamin Muschko added 30 commits December 13, 2016 18:54

Register clean up for Java plugin

d2d4113

Needs to create marker file if it doesn't exist

65da1a4

Reuse fixture for buildSrc

c141e76

Assert that no clean up is performed during first execution

b216c58

Do not explicitly apply Java plugin to buildSrc

3c91fc4

Add test case on reconfiguring output dirs during execution phase

65f9a2a

Verify multi-project builds with parallel and configure-on-demand ena…

4739aac

…bled

Don't explicitly close cache

f48929e

Use correct output message

188f9c6

Only consider the use case of supporting "configuration" tasks

505177c

Reuse deletion logic for buildSrc

5553ab8

For now buildSrc will delete the output directories registered by the JavaPlugin and not buildDir.

Delete buildDir for buildSrc

f6f12c9

Break out deletion logic

b91ef17

Optimze imports

5159ffa

Fix test

ef7abb8

Only check closed state if underlying implementation tracks changes

1a7e11b

Add tests for delete operation

c248294

Add test for locked file

f5328f0

Check that file still exists

a3530de

Sample runner should delete empty directories

39b9a3d

Many empty directories were left behind which caused the build output cleanup to kick in and produce unexpected output.

Register buildSrc's buildDir after project has been evaluated

27aaa10

Move creation of build output registry to different service

95a08ed

Close cache right after use

59260a5

Split up creation of build out clean up classes

c098be9

Build output registry needs to be define for a build session so it can be shared between the main build and buildSrc.

Custom buildSrc build directory can be cleaned up

da165ad

Do not provide thrown exception in message

ea7a091

Fix placeholder logic

e5843fd

Only delete root outputs

7b8e37e

Add more test coverage

fe78169

Make logic more robust

cd9cc51

bmuschko added the in:jvm-ecosystem label Dec 27, 2016

bmuschko requested review from big-guy and wolfs January 3, 2017 12:35

bmuschko self-assigned this Jan 3, 2017

bmuschko added this to the 4.0 RC1 milestone Jan 3, 2017

big-guy reviewed Jan 9, 2017

View reviewed changes

Benjamin Muschko added 8 commits January 10, 2017 17:50

Only register unique files and directories

e971398

Refer to file independent of execution directory

9014981

Better format Javadocs

84f6ff7

Simply consumer logic by taking FileCollection as parameter

763c45b

Remove unused method

bbcb57b

Merge branch 'master' into bm-stale-output-cleanup

29745da

Cache access does not take description anymore

2db33c6

Fix test

ff71f72

wolfs requested changes Jan 12, 2017

View reviewed changes

wolfs reviewed Jan 12, 2017

View reviewed changes

wolfs mentioned this pull request Jan 13, 2017

Gradle considers tasks up-to-date when it should not #1168

Closed

bmuschko assigned wolfs and big-guy and unassigned bmuschko and wolfs Jan 13, 2017

wolfs modified the milestones: 3.4 RC1, 4.0 RC1 Jan 18, 2017

big-guy closed this Jan 19, 2017

bmuschko deleted the bm-stale-output-cleanup branch January 20, 2017 22:16

ov7a removed this from the 3.4 RC1 milestone Mar 28, 2024

Uh oh!

Conversation

bmuschko commented Dec 27, 2016

Uh oh!

bmuschko commented Dec 27, 2016

Uh oh!

big-guy commented Jan 9, 2017

Uh oh!

big-guy commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmuschko Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmuschko Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

big-guy commented Jan 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bmuschko Jan 10, 2017 •

edited

Loading

bmuschko Jan 10, 2017 •

edited

Loading