[FIXED HUDSON-2417] Taking stability into account when deciding which builds & artifacts to keep.

Previously, Hudson only tried to keep successful builds, meaning it would discard the last stable build
if there was a long run of unstable builds. This made .../lastStableBuild/... permalinks useless.
Artifact archiver logic with "latest only" also improved more generally:
1. Discards old artifacts at the start of the build. It might as well save the disk space sooner rather than later.
2. Rather than deleting all old artifacts prior to some point, now keeps the latest representatives
   of different stability classes of build: always keeps last stable if available, and also last successful if newer,
   and (just in case) also the last failed if that is newer. Formerly it behaved oddly: during a run of failed builds,
   all of their artifacts would be kept, though these are unlikely to be useful.
Also introducing tests for all this functionality, which does not seem to have been tested before.


git-svn-id: https://hudson.dev.java.net/svn/hudson/trunk/hudson/main@15587 71c3de6d-444a-0410-be80-ed276b4c234a

core/src/main/java/hudson/tasks/ArtifactArchiver.java

@@ -117,13 +117,18 @@ public class ArtifactArchiver extends Publisher {
             return true;
         }
 
-        if(latestOnly) {
-            AbstractBuild<?,?> b = p.getLastSuccessfulBuild();
-            if(b!=null) {
-                while(true) {
-                    b = b.getPreviousBuild();
-                    if(b==null)     break;
+        return true;
+    }
 
+
+    public @Override boolean prebuild(AbstractBuild<?, ?> build, BuildListener listener) {
+        if(latestOnly) {
+            AbstractBuild<?,?> b = build.getProject().getLastCompletedBuild();
+            Result bestResultSoFar = Result.NOT_BUILT;
+            while(b!=null) {
+                if (b.getResult().isBetterThan(bestResultSoFar)) {
+                    bestResultSoFar = b.getResult();
+                } else {
                     // remove old artifacts
                     File ad = b.getArtifactsDir();
                     if(ad.exists()) {
@@ -135,9 +140,9 @@ public class ArtifactArchiver extends Publisher {
                         }
                     }
                 }
+                b = b.getPreviousBuild();
             }
         }
-
         return true;
     }
 

core/src/main/java/hudson/tasks/LogRotator.java

@@ -78,11 +78,12 @@ public class LogRotator implements Describable<LogRotator> {
     public void perform(Job<?,?> job) throws IOException, InterruptedException {
         // keep the last successful build regardless of the status
         Run lsb = job.getLastSuccessfulBuild();
+        Run lstb = job.getLastStableBuild();
 
         if(numToKeep!=-1) {
             Run[] builds = job.getBuilds().toArray(new Run[0]);
             for( int i=numToKeep; i<builds.length; i++ ) {
-                if(!builds[i].isKeepLog() && builds[i]!=lsb)
+                if(!builds[i].isKeepLog() && builds[i]!=lsb && builds[i]!=lstb)
                     builds[i].delete();
             }
         }
@@ -92,7 +93,7 @@ public class LogRotator implements Describable<LogRotator> {
             cal.add(Calendar.DAY_OF_YEAR,-daysToKeep);
             // copy it to the array because we'll be deleting builds as we go.
             for( Run r : job.getBuilds().toArray(new Run[0]) ) {
-                if(r.getTimestamp().before(cal) && !r.isKeepLog() && r!=lsb)
+                if(r.getTimestamp().before(cal) && !r.isKeepLog() && r!=lsb && r!=lstb)
                     r.delete();
             }
         }

core/src/main/resources/hudson/tasks/ArtifactArchiver/config.jelly

@@ -31,7 +31,7 @@ THE SOFTWARE.
     <f:entry title="${%Excludes}" help="/help/tasks/artifactArchiver/excludes.html">
       <f:textbox name="artifacts.excludes" value="${instance.excludes}"/>
     </f:entry>
-    <f:entry title="">
+    <f:entry title="" help="/help/tasks/artifactArchiver/latestOnly.html">
       <f:checkbox name="artifacts.latestOnly" checked="${instance.latestOnly}" />
       <label class="attach-previous">${%lastBuildOnly}</label>
     </f:entry>

core/src/main/resources/hudson/tasks/ArtifactArchiver/config.properties

@@ -20,4 +20,4 @@
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 # THE SOFTWARE.
 
-lastBuildOnly=Discard all but the last successful artifact to save disk space
\ No newline at end of file
+lastBuildOnly=Discard all but the last successful/stable artifact to save disk space

test/src/test/java/hudson/tasks/ArtifactArchiverTest.java

+/*
+ * The MIT License
+ *
+ * Copyright (c) 2004-2009, Sun Microsystems, Inc., Kohsuke Kawaguchi
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package hudson.tasks;
+
+import hudson.Launcher;
+import hudson.model.AbstractBuild;
+import hudson.model.BuildListener;
+import hudson.model.FreeStyleProject;
+import hudson.model.Result;
+import hudson.tasks.LogRotatorTest.TestsFail;
+import static hudson.tasks.LogRotatorTest.build;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import org.jvnet.hudson.test.Bug;
+import org.jvnet.hudson.test.FailureBuilder;
+import org.jvnet.hudson.test.HudsonTestCase;
+import org.jvnet.hudson.test.TestBuilder;
+
+/**
+ * Verifies that artifacts from the last successful and stable builds of a job will be kept if requested.
+ */
+public class ArtifactArchiverTest extends HudsonTestCase {
+
+    public void testSuccessVsFailure() throws Exception {
+        FreeStyleProject project = createFreeStyleProject();
+        project.getPublishersList().replaceBy(Collections.singleton(new ArtifactArchiver("f", "", true)));
+        assertEquals("(no artifacts)", Result.FAILURE, build(project)); // #1
+        assertFalse(project.getBuildByNumber(1).getHasArtifacts());
+        project.getBuildersList().replaceBy(Collections.singleton(new CreateArtifact()));
+        assertEquals(Result.SUCCESS, build(project)); // #2
+        assertTrue(project.getBuildByNumber(2).getHasArtifacts());
+        project.getBuildersList().replaceBy(Arrays.asList(new CreateArtifact(), new FailureBuilder()));
+        assertEquals(Result.FAILURE, build(project)); // #3
+        assertTrue(project.getBuildByNumber(2).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(3).getHasArtifacts());
+        assertEquals(Result.FAILURE, build(project)); // #4
+        assertTrue(project.getBuildByNumber(2).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(3).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(4).getHasArtifacts());
+        assertEquals(Result.FAILURE, build(project)); // #5
+        assertTrue(project.getBuildByNumber(2).getHasArtifacts());
+        assertFalse("no better than #4", project.getBuildByNumber(3).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(4).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(5).getHasArtifacts());
+        project.getBuildersList().replaceBy(Collections.singleton(new CreateArtifact()));
+        assertEquals(Result.SUCCESS, build(project)); // #6
+        assertTrue("#2 is still lastSuccessful until #6 is complete", project.getBuildByNumber(2).getHasArtifacts());
+        assertFalse(project.getBuildByNumber(3).getHasArtifacts());
+        assertFalse(project.getBuildByNumber(4).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(5).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(6).getHasArtifacts());
+        assertEquals(Result.SUCCESS, build(project)); // #7
+        assertFalse("lastSuccessful was #6 for ArtifactArchiver", project.getBuildByNumber(2).getHasArtifacts());
+        assertFalse(project.getBuildByNumber(3).getHasArtifacts());
+        assertFalse(project.getBuildByNumber(4).getHasArtifacts());
+        assertFalse(project.getBuildByNumber(5).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(6).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(7).getHasArtifacts());
+    }
+
+    @Bug(2417)
+    public void testStableVsUnstable() throws Exception {
+        FreeStyleProject project = createFreeStyleProject();
+        Publisher artifactArchiver = new ArtifactArchiver("f", "", true);
+        project.getPublishersList().replaceBy(Collections.singleton(artifactArchiver));
+        project.getBuildersList().replaceBy(Collections.singleton(new CreateArtifact()));
+        assertEquals(Result.SUCCESS, build(project)); // #1
+        assertTrue(project.getBuildByNumber(1).getHasArtifacts());
+        project.getPublishersList().replaceBy(Arrays.asList(artifactArchiver, new TestsFail()));
+        assertEquals(Result.UNSTABLE, build(project)); // #2
+        assertTrue(project.getBuildByNumber(1).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(2).getHasArtifacts());
+        assertEquals(Result.UNSTABLE, build(project)); // #3
+        assertTrue(project.getBuildByNumber(1).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(2).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(3).getHasArtifacts());
+        assertEquals(Result.UNSTABLE, build(project)); // #4
+        assertTrue(project.getBuildByNumber(1).getHasArtifacts());
+        assertFalse(project.getBuildByNumber(2).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(3).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(4).getHasArtifacts());
+        project.getPublishersList().replaceBy(Collections.singleton(artifactArchiver));
+        assertEquals(Result.SUCCESS, build(project)); // #5
+        assertTrue(project.getBuildByNumber(1).getHasArtifacts());
+        assertFalse(project.getBuildByNumber(2).getHasArtifacts());
+        assertFalse(project.getBuildByNumber(3).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(4).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(5).getHasArtifacts());
+        assertEquals(Result.SUCCESS, build(project)); // #6
+        assertFalse(project.getBuildByNumber(1).getHasArtifacts());
+        assertFalse(project.getBuildByNumber(2).getHasArtifacts());
+        assertFalse(project.getBuildByNumber(3).getHasArtifacts());
+        assertFalse(project.getBuildByNumber(4).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(5).getHasArtifacts());
+        assertTrue(project.getBuildByNumber(6).getHasArtifacts());
+    }
+
+    private static class CreateArtifact extends TestBuilder {
+        public boolean perform(AbstractBuild<?,?> build, Launcher launcher, BuildListener listener) throws IOException, InterruptedException {
+            build.getProject().getWorkspace().child("f").write("content", "UTF-8");
+            return true;
+        }
+    }
+
+}

test/src/test/java/hudson/tasks/LogRotatorTest.java

+/*
+ * The MIT License
+ *
+ * Copyright (c) 2004-2009, Sun Microsystems, Inc., Kohsuke Kawaguchi
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package hudson.tasks;
+
+import hudson.Launcher;
+import hudson.model.AbstractBuild;
+import hudson.model.BuildListener;
+import hudson.model.Cause.LegacyCodeCause;
+import hudson.model.Descriptor;
+import hudson.model.FreeStyleProject;
+import hudson.model.Result;
+import hudson.model.Run;
+import java.util.Collections;
+import java.util.concurrent.TimeUnit;
+import org.jvnet.hudson.test.Bug;
+import org.jvnet.hudson.test.FailureBuilder;
+import org.jvnet.hudson.test.HudsonTestCase;
+
+/**
+ * Verifies that the last successful and stable builds of a job will be kept if requested.
+ */
+public class LogRotatorTest extends HudsonTestCase {
+
+    public void testSuccessVsFailure() throws Exception {
+        FreeStyleProject project = createFreeStyleProject();
+        project.setLogRotator(new LogRotator(-1, 2));
+        assertEquals(Result.SUCCESS, build(project)); // #1
+        project.getBuildersList().replaceBy(Collections.singleton(new FailureBuilder()));
+        assertEquals(Result.FAILURE, build(project)); // #2
+        assertEquals(Result.FAILURE, build(project)); // #3
+        assertEquals(1, numberOf(project.getLastSuccessfulBuild()));
+        project.getBuildersList().replaceBy(Collections.<Builder>emptySet());
+        assertEquals(Result.SUCCESS, build(project)); // #4
+        assertEquals(4, numberOf(project.getLastSuccessfulBuild()));
+        assertEquals(null, project.getBuildByNumber(1));
+        assertEquals(null, project.getBuildByNumber(2));
+        assertEquals(3, numberOf(project.getLastFailedBuild()));
+    }
+
+    @Bug(2417)
+    public void testStableVsUnstable() throws Exception {
+        FreeStyleProject project = createFreeStyleProject();
+        project.setLogRotator(new LogRotator(-1, 2));
+        assertEquals(Result.SUCCESS, build(project)); // #1
+        project.getPublishersList().replaceBy(Collections.singleton(new TestsFail()));
+        assertEquals(Result.UNSTABLE, build(project)); // #2
+        assertEquals(Result.UNSTABLE, build(project)); // #3
+        assertEquals(1, numberOf(project.getLastStableBuild()));
+        project.getPublishersList().replaceBy(Collections.<Publisher>emptySet());
+        assertEquals(Result.SUCCESS, build(project)); // #4
+        assertEquals(null, project.getBuildByNumber(1));
+        assertEquals(null, project.getBuildByNumber(2));
+    }
+
+    static Result build(FreeStyleProject project) throws Exception {
+        return project.scheduleBuild2(0, new LegacyCodeCause()).get(10, TimeUnit.SECONDS).getResult();
+    }
+
+    private static int numberOf(Run<?,?> run) {
+        return run != null ? run.getNumber() : -1;
+    }
+
+    static class TestsFail extends Publisher {
+        public @Override boolean perform(AbstractBuild<?,?> build, Launcher launcher, BuildListener listener) {
+            build.setResult(Result.UNSTABLE);
+            return true;
+        }
+        public Descriptor<Publisher> getDescriptor() {
+            return new Descriptor<Publisher>(TestsFail.class) {
+                public String getDisplayName() {
+                    return "TestsFail";
+                }
+            };
+        }
+    }
+
+}

war/resources/help/project-config/log-rotation.html

@@ -15,4 +15,5 @@
 
   Hudson also allows you to mark an individual build as 'Keep this log forever', to
   exclude certain important builds from being discarded automatically.
+  The last stable and last successful build are always kept as well.
 </div>
\ No newline at end of file

war/resources/help/tasks/artifactArchiver/latestOnly.html

+<div>
+    <p>
+        If checked, Hudson will discard most artifacts from older builds.
+        Artifacts from the last stable former build (if any) are kept, and also those
+        from the last unstable build if that is newer, and from the last failed
+        build if that is newer.
+    </p>
+    <p>
+        This option saves disk space, but still lets you safely create permalinks
+        such as <code>.../lastStableBuild/artifact/...</code> or
+        <code>.../lastSuccessfulBuild/artifact/...</code> or
+        <code>.../lastCompletedBuild/artifact/...</code>.
+    </p>
+    <p>
+        (Since the current build is still running, Hudson conservatively assumes
+        that it might yet fail in a publisher - so it keeps at least one older build's
+        artifacts around.)
+    </p>
+</div>