[JENKINS-16089] Revising 88feabb4

Based on comments from Jesse, revising the fix.

I'm now putting permlinks inside the builds/ directory to avoid the computing hassle involved in the split $JENKINS_HOME.

What we historically had in $JENKINS_HOME/jobs/JOB/lastSuccessfulBuild is also now subsumed by this feature. I initially attempted to create these permalinks in the buidl root directory, but turns out those symlinks aren't the same name as the ID of permalinks, so it doesn't mesh well.

And finally, a test!

core/src/main/java/hudson/model/AbstractBuild.java

@@ -37,6 +37,7 @@ import hudson.console.ModelHyperlinkNote;
 import hudson.matrix.MatrixConfiguration;
 import hudson.model.Fingerprint.BuildPtr;
 import hudson.model.Fingerprint.RangeSet;
+import hudson.model.PermalinkProjectAction.Permalink;
 import hudson.model.listeners.RunListener;
 import hudson.model.listeners.SCMListener;
 import hudson.scm.ChangeLogParser;
@@ -58,6 +59,7 @@ import hudson.tasks.test.AbstractTestResultAction;
 import hudson.tasks.test.AggregatedTestResultAction;
 import hudson.util.*;
 import jenkins.model.Jenkins;
+import jenkins.model.PeepholePermalink;
 import jenkins.model.lazy.AbstractLazyLoadRunMap.Direction;
 import jenkins.model.lazy.BuildReference;
 import org.kohsuke.stapler.HttpResponse;
@@ -368,7 +370,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
     public final FilePath getModuleRoot() {
         FilePath ws = getWorkspace();
         if (ws==null)    return null;
-        return getParent().getScm().getModuleRoot(ws,this);
+        return getParent().getScm().getModuleRoot(ws, this);
     }
 
     /**
@@ -469,42 +471,21 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
         return hudsonVersion;
     }
 
-    @Override
-    public synchronized void delete() throws IOException {
-        // Need to check if deleting this build affects lastSuccessful/lastStable symlinks
-        R lastSuccessful = getProject().getLastSuccessfulBuild(),
-          lastStable = getProject().getLastStableBuild();
-
-        super.delete();
-
-        try {
-            if (lastSuccessful == this)
-                updateSymlink("lastSuccessful", getProject().getLastSuccessfulBuild());
-            if (lastStable == this)
-                updateSymlink("lastStable", getProject().getLastStableBuild());
-        } catch (InterruptedException ex) {
-            LOGGER.warning("Interrupted update of lastSuccessful/lastStable symlinks for "
-                           + getProject().getDisplayName());
-            // handle it later
-            Thread.currentThread().interrupt();
-        }
-    }
-
-    private void updateSymlink(String name, AbstractBuild<?,?> newTarget) throws InterruptedException {
-        if (newTarget != null)
-            newTarget.createSymlink(new LogTaskListener(LOGGER, Level.WARNING), name);
-        else
-            new File(getProject().getRootDir(), name).delete();
-    }
-
-    private void createSymlink(TaskListener listener, String name) throws InterruptedException {
-        String target;
+    /**
+     * Backward compatibility.
+     *
+     * We used to have $JENKINS_HOME/jobs/JOBNAME/lastStable and lastSuccessful symlinked to the appropriate
+     * builds, but now those are done in {@link PeepholePermalink}. So here, we simply create symlinks that
+     * resolves to the symlink created by {@link PeepholePermalink}.
+     */
+    private void createSymlink(TaskListener listener, String name, Permalink target) throws InterruptedException {
+        String targetDir;
         if (getProject().getBuildDir().equals(new File(getProject().getRootDir(), "builds"))) {
-            target = "builds/" + getId();
+            targetDir = "builds/" + target.getId();
         } else {
-            target = getRootDir().getAbsolutePath();
+            targetDir = getProject().getBuildDir()+target.getId();
         }
-        Util.createSymlink(getProject().getRootDir(), target, name, listener);
+        Util.createSymlink(getProject().getRootDir(), targetDir, name, listener);
     }
 
     /**
@@ -738,11 +719,8 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
             try {
                 post2(listener);
 
-                if (result.isBetterOrEqualTo(Result.UNSTABLE))
-                    createSymlink(listener, "lastSuccessful");
-
-                if (result.isBetterOrEqualTo(Result.SUCCESS))
-                    createSymlink(listener, "lastStable");
+                createSymlink(listener, "lastSuccessful", Permalink.LAST_SUCCESSFUL_BUILD);
+                createSymlink(listener, "lastStable", Permalink.LAST_STABLE_BUILD);
             } finally {
                 // update the culprit list
                 HashSet<String> r = new HashSet<String>();

core/src/main/java/hudson/model/Job.java

@@ -744,7 +744,7 @@ public abstract class Job<JobT extends Job<JobT, RunT>, RunT extends Run<JobT, R
      * 
      * @see RunMap
      */
-    protected File getBuildDir() {
+    public File getBuildDir() {
         return Jenkins.getInstance().getBuildDirFor(this);
     }
 

core/src/main/java/jenkins/model/PeepholePermalink.java

 package jenkins.model;
 
 import com.google.common.base.Predicate;
-import hudson.Functions;
+import hudson.Extension;
 import hudson.Util;
 import hudson.model.Job;
 import hudson.model.PermalinkProjectAction.Permalink;
 import hudson.model.Run;
+import hudson.model.TaskListener;
+import hudson.model.listeners.RunListener;
 import hudson.util.AtomicFileWriter;
 import hudson.util.StreamTaskListener;
 import org.apache.commons.io.FileUtils;
 
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import java.io.File;
 import java.io.IOException;
 import java.io.StringWriter;
@@ -48,14 +52,14 @@ import java.util.logging.Logger;
  * (it simply scans the history till find the new matching build.) To tolerate G(B)
  * that goes from false to true, you need to be able to intercept whenever G(B) changes
  * from false to true, then call {@link #resolve(Job)} to check the current permalink target
- * is up to date, then call {@link #updateCache(Job, int)} if it needs updating.
+ * is up to date, then call {@link #updateCache(Job, Run)} if it needs updating.
  *
  * @author Kohsuke Kawaguchi
  * @since 1.507
  */
 public abstract class PeepholePermalink extends Permalink implements Predicate<Run<?,?>> {
     /**
-     * Checks if the given build satifies the peep-hole criteria.
+     * Checks if the given build satisfies the peep-hole criteria.
      *
      * This is the "G(B)" as described in the class javadoc.
      */
@@ -65,7 +69,7 @@ public abstract class PeepholePermalink extends Permalink implements Predicate<R
      * The file in which the permalink target gets recorded.
      */
     protected File getPermalinkFile(Job<?,?> job) {
-        return new File(job.getRootDir(),"permalinks/"+getId());
+        return new File(job.getBuildDir(),getId());
     }
 
     /**
@@ -77,18 +81,10 @@ public abstract class PeepholePermalink extends Permalink implements Predicate<R
         Run<?,?> b=null;
 
         try {
-            String target = null;
-            if (USE_SYMLINK) { // f.exists() return false if symlink exists but point to a non-existent directory
-                target = Util.resolveSymlink(f);
-                if (target==null && f.exists()) {
-                    // if this file isn't a symlink, it must be a regular file
-                    target = FileUtils.readFileToString(f,"UTF-8").trim();
-                }
-            } else {
-                if (f.exists()) {
-                    // if this file isn't a symlink, it must be a regular file
-                    target = FileUtils.readFileToString(f,"UTF-8").trim();
-                }
+            String target = Util.resolveSymlink(f);
+            if (target==null && f.exists()) {
+                // if this file isn't a symlink, it must be a regular file
+                target = FileUtils.readFileToString(f,"UTF-8").trim();
             }
 
             if (target!=null) {
@@ -107,8 +103,8 @@ public abstract class PeepholePermalink extends Permalink implements Predicate<R
             LOGGER.log(Level.WARNING, "Failed to read permalink cache:" + f, e);
             // if we fail to read the cache, fall back to the re-computation
         } catch (IOException e) {
-            LOGGER.log(Level.WARNING, "Failed to read permalink cache:" + f, e);
-            // if we fail to read the cache, fall back to the re-computation
+            // this happens when the symlink doesn't exist
+            // (and it cannot be distinguished from the case when the actual I/O error happened
         }
 
         if (b==null) {
@@ -116,40 +112,39 @@ public abstract class PeepholePermalink extends Permalink implements Predicate<R
             b = job.getLastBuild();
         }
 
-        int n;
         // start from the build 'b' and locate the build that matches the criteria going back in time
-        while (true) {
-            if (b==null) {
-                n = RESOLVES_TO_NONE;
-                break;
-            }
-            if (apply(b)) {
-                n = b.getNumber();
-                break;
-            }
+        b = find(b);
 
-            b=b.getPreviousBuild();
-        }
+        updateCache(job,b);
+        return b;
+    }
 
-        updateCache(job,n);
+    /**
+     * Start from the build 'b' and locate the build that matches the criteria going back in time
+     */
+    private Run<?,?> find(Run<?,?> b) {
+        for ( ; b!=null && !apply(b); b=b.getPreviousBuild())
+            ;
         return b;
     }
 
     /**
      * Remembers the value 'n' in the cache for future {@link #resolve(Job)}.
      */
-    protected void updateCache(Job<?,?> job, int n) {
+    protected void updateCache(@Nonnull Job<?,?> job, @Nullable Run<?,?> b) {
+        final int n = b==null ? RESOLVES_TO_NONE : b.getNumber();
+
         File cache = getPermalinkFile(job);
+        File tmp = new File(cache.getPath()+".tmp");
         cache.getParentFile().mkdirs();
 
         try {
             StringWriter w = new StringWriter();
             StreamTaskListener listener = new StreamTaskListener(w);
 
-            if (USE_SYMLINK) {
-                Util.createSymlink(cache.getParentFile(),"../builds/"+n,cache.getName(),listener);
-            } else {
-                // symlink not supported. use a regular
+            Util.createSymlink(tmp.getParentFile(),String.valueOf(n),tmp.getName(),listener);
+            if (Util.resolveSymlink(tmp)==null) {
+                // symlink not supported. use a regular file
                 AtomicFileWriter cw = new AtomicFileWriter(cache);
                 try {
                     cw.write(String.valueOf(n));
@@ -157,28 +152,55 @@ public abstract class PeepholePermalink extends Permalink implements Predicate<R
                 } finally {
                     cw.abort();
                 }
+            } else {
+                cache.delete();
+                tmp.renameTo(cache);
             }
         } catch (IOException e) {
-            LOGGER.log(Level.WARNING, "Failed to update permalink cache for " + job, e);
+            LOGGER.log(Level.WARNING, "Failed to update "+job+" "+getId()+" permalink for " + b, e);
             cache.delete();
         } catch (InterruptedException e) {
-            LOGGER.log(Level.WARNING, "Failed to update permalink cache for "+job,e);
+            LOGGER.log(Level.WARNING, "Failed to update "+job+" "+getId()+" permalink for " + b, e);
             cache.delete();
+        } finally {
+            tmp.delete();
+        }
+    }
+
+    @Extension
+    public static class RunListenerImpl extends RunListener<Run<?,?>> {
+        /**
+         * If any of the peephole permalink points to the build to be deleted, update it to point to the new location.
+         */
+        @Override
+        public void onDeleted(Run run) {
+            Job<?, ?> j = run.getParent();
+            for (PeepholePermalink pp : Util.filter(j.getPermalinks(), PeepholePermalink.class)) {
+                if (pp.apply(run)) {
+                    if (pp.resolve(j)==run) {
+                        pp.updateCache(j,pp.find(run.getPreviousBuild()));
+                    }
+                }
+            }
+        }
+
+        /**
+         * See if the new build matches any of the peephole permalink.
+         */
+        @Override
+        public void onCompleted(Run<?,?> run, @Nonnull TaskListener listener) {
+            Job<?, ?> j = run.getParent();
+            for (PeepholePermalink pp : Util.filter(j.getPermalinks(), PeepholePermalink.class)) {
+                if (pp.apply(run)) {
+                    Run<?, ?> cur = pp.resolve(j);
+                    if (cur==null || cur.getNumber()<run.getNumber())
+                        pp.updateCache(j,run);
+                }
+            }
         }
     }
 
     private static final int RESOLVES_TO_NONE = -1;
 
     private static final Logger LOGGER = Logger.getLogger(PeepholePermalink.class.getName());
-
-    /**
-     * True if we use the symlink as cache, false if plain text file.
-     *
-     * <p>
-     * On Windows, even with Java7, using symlinks require one to go through quite a few hoops
-     * (you need to change the security policy to specifically have this permission, then
-     * you better not be in the administrator group because this token gets filtered out
-     * on UAC-enabled Windows.)
-     */
-    public static boolean USE_SYMLINK = !Functions.isWindows();
 }

core/src/test/java/hudson/model/StubJob.java

@@ -57,7 +57,7 @@ class StubJob extends Job {
         
     }
 
-    @Override protected File getBuildDir() {
+    @Override public File getBuildDir() {
         return new File(System.getProperty("java.io.tmpdir"));
     }
     

test/src/test/groovy/jenkins/model/PeepholePermalinkTest.groovy

+package jenkins.model
+
+import hudson.Functions
+import hudson.Util
+import hudson.model.Run
+import org.jvnet.hudson.test.FailureBuilder
+import org.jvnet.hudson.test.HudsonTestCase
+
+/**
+ *
+ *
+ * @author Kohsuke Kawaguchi
+ */
+class PeepholePermalinkTest extends HudsonTestCase {
+    /**
+     * Basic operation of the permalink generation.
+     */
+    void testBasics() {
+        if (Functions.isWindows())  return; // can't run on windows because we rely on symlinks
+
+        def p = createFreeStyleProject()
+        def b1 = assertBuildStatusSuccess(p.scheduleBuild2(0))
+
+        def lsb = new File(p.buildDir, "lastSuccessfulBuild")
+        def lfb = new File(p.buildDir, "lastFailedBuild")
+
+        assertLink(lsb,b1)
+
+        // now another build that fails
+        p.buildersList.add(new FailureBuilder())
+        def b2 = p.scheduleBuild2(0).get()
+
+        assertLink(lsb,b1)
+        assertLink(lfb,b2)
+
+        // one more build and this time it succeeds
+        p.buildersList.clear()
+        def b3 = assertBuildStatusSuccess(p.scheduleBuild2(0))
+
+        assertLink(lsb,b3)
+        assertLink(lfb,b2)
+
+        // delete b3 and symlinks should update properly
+        b3.delete()
+        assertLink(lsb,b1)
+        assertLink(lfb,b2)
+
+        b1.delete()
+        assertLink(lsb,null)
+        assertLink(lfb,b2)
+
+        b2.delete()
+        assertLink(lsb,null)
+        assertLink(lfb,null)
+    }
+
+    def assertLink(File symlink, Run build) {
+        assert Util.resolveSymlink(symlink)==(build==null ? "-1" : build.number as String);
+    }
+
+    /**
+     * job/JOBNAME/lastStable and job/JOBNAME/lastSuccessful symlinks that we used to generate should still work
+     */
+    void testLegacyCompatibility() {
+        if (Functions.isWindows())  return; // can't run on windows because we rely on symlinks
+
+        def p = createFreeStyleProject()
+        def b1 = assertBuildStatusSuccess(p.scheduleBuild2(0))
+
+        ["lastStable","lastSuccessful"].each { n ->
+            // test if they both point to b1
+            assert new File(p.rootDir,"$n/build.xml").length() == new File(b1.rootDir,"build.xml").length()
+        }
+    }
+}
\ No newline at end of file