While debugging the shelve plugin I noticed that the tar function available in the FilePath class is not conserving the symlinks anymore. The shelve plugin is calling the archive function in FilePath:

      archive(final ArchiverFactory factory, OutputStream os, final DirScanner scanner) // factory use is TarArchiverFactory with no compression

       

      Both tar archiving function do not properly handle the symlinks to directory (but it works correctly for symlinks to files):

      * public int tar(OutputStream out, final String glob) -> uses a DirScanner.Glob (ant style FileSets) that follows the symlink, therefore creates a brand new directory in place of the symlink, not what we want for a tar archive.

      * public int tar(OutputStream out, FileFilter filter) -> uses a FileFilter wrapper which for some reasons doesn't redirect the symlink calls to the wrapped FileFilter

       

      Why this is important: this issue has been here for long, but a change made on 2.91 (see my comment below for Issue 2), made this appear more clearly.

       

          [JENKINS-52781] tar function is breaking symlinks

          Pierre Beitz added a comment - - edited

          dnusbaum

          I did some additional tests and found out that the issue I found, was already present, but was outlined by another one (or a behavior change?) introduced in 2.91. Here are the details:

          Issue 1: tar breaking the symlinks (not a regression, at least since 2.60)

          This is the issue I initially reported, except it's not a regression. I found out that a symlink to a directory is tarred into a directory. Here is a unit test outlining that. Note that the test is red since 2.60 at least:

           

          import hudson.FilePath;
          import org.junit.Assert;
          import org.junit.Rule;
          import org.junit.rules.TemporaryFolder;
          
          import java.io.IOException;
          import java.io.OutputStream;
          import java.nio.file.Files;
          import java.nio.file.Path;
          import java.nio.file.Paths;
          
          public class Test {
          
              @Rule
              public TemporaryFolder folder= new TemporaryFolder();
          
              @org.junit.Test
              public void aSymlinkToADirectoryFails() throws IOException, InterruptedException {
                  Path folderToTar = folder.newFolder().toPath();
                  Path subdirectory = Files.createDirectory(folderToTar.resolve("subdirectory"));
                  Path targetDirectory = subdirectory.resolve("1");
                  Files.createDirectory(targetDirectory);
                  Files.createFile(targetDirectory.resolve("oneFile"));
                  Path link = Files.createSymbolicLink(subdirectory.resolve("link"), Paths.get("1"));
                  Assert.assertTrue(Files.isSymbolicLink(link));
          
                  Path tarFile = folderToTar.resolve("archive.tar");
                  FilePath tarFilePath = new FilePath(tarFile.toFile());
          
                  try (OutputStream os = tarFilePath.write()) {
                      new FilePath( folderToTar.toFile()).tar(os, "**/*");
                  }
          
                  Path untarredFolder = folder.getRoot().toPath().resolve("untarredFolder");
          
                  tarFilePath.untar(new FilePath(untarredFolder.toFile()), FilePath.TarCompression.NONE);
          
                  Assert.assertTrue(Files.isSymbolicLink(untarredFolder.resolve("subdirectory/link")));
          
              }
          
              @org.junit.Test
              public void aSymlinkToAFileWorks() throws IOException, InterruptedException {
                  Path folderToTar = folder.newFolder().toPath();
                  Path subdirectory = Files.createDirectory(folderToTar.resolve("subdirectory"));
                  Path targetFile = subdirectory.resolve("1");
                  Files.createFile(targetFile);
                  Path link = Files.createSymbolicLink(subdirectory.resolve("link"), Paths.get("1"));
                  Assert.assertTrue(Files.isSymbolicLink(link));
          
                  Path tarFile = folderToTar.resolve("archive.tar");
                  FilePath tarFilePath = new FilePath(tarFile.toFile());
          
                  try (OutputStream os = tarFilePath.write()) {
                      new FilePath( folderToTar.toFile()).tar(os, "**/*");
                  }
          
                  Path untarredFolder = folder.getRoot().toPath().resolve("untarredFolder");
          
                  tarFilePath.untar(new FilePath(untarredFolder.toFile()), FilePath.TarCompression.NONE);
          
                  Assert.assertTrue(Files.isSymbolicLink(untarredFolder.resolve("subdirectory/link")));
          
              }
          }
          

          Issue 2: behavior change in 2.91

          The shelve plugin is using the tar function we just saw to archive job directories. Upon unarchiving, a job directory looks therefore like this:

          drwxr-xr-x  7 pierrebeitz  staff  224 27 jul 23:50 .
          drwxr-xr-x  7 pierrebeitz  staff  224 27 jul 23:50 ..
          drwxr-xr-x  9 pierrebeitz  staff  288 27 jul 23:50 builds
          -rw-r--r--  1 pierrebeitz  staff  497 27 jul 23:42 config.xml
          drwxr-xr-x  5 pierrebeitz  staff  160 27 jul 23:50 lastStable
          drwxr-xr-x  5 pierrebeitz  staff  160 27 jul 23:50 lastSuccessful
          -rw-r--r--  1 pierrebeitz  staff    2 27 jul 23:42 nextBuildNumber
          

          Instead of:

          drwxr-xr-x  7 pierrebeitz  staff  224 27 jul 23:42 .
          drwxr-xr-x  7 pierrebeitz  staff  224 27 jul 23:44 ..
          drwxr-xr-x  9 pierrebeitz  staff  288 27 jul 23:42 builds
          -rw-r--r--  1 pierrebeitz  staff  497 27 jul 23:42 config.xml
          lrwxr-xr-x  1 pierrebeitz  staff   22 27 jul 23:42 lastStable -> builds/lastStableBuild
          lrwxr-xr-x  1 pierrebeitz  staff   26 27 jul 23:42 lastSuccessful -> builds/lastSuccessfulBuild
          -rw-r--r--  1 pierrebeitz  staff    2 27 jul 23:42 nextBuildNumber

           

          The thing is, before 2.91, when launching a new run of the job, Jenkins was somehow capable of deleting those directories and recreating the symlinks.

          Now, it is unable of doing so:

          ln builds/lastSuccessfulBuild ***/jenkins-home/jobs/toto/lastSuccessful failed
          java.nio.file.DirectoryNotEmptyException: ***/jenkins-home/jobs/toto/lastSuccessful
          	at sun.nio.fs.UnixFileSystemProvider.implDelete(UnixFileSystemProvider.java:242)
          	at sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:108)
          	at java.nio.file.Files.deleteIfExists(Files.java:1165)
          	at hudson.Util.createSymlink(Util.java:1333)
          	at hudson.model.Run.createSymlink(Run.java:1866)
          	at hudson.model.Run.updateSymlinks(Run.java:1847)
          	at hudson.model.Run.execute(Run.java:1725)
          	at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
          	at hudson.model.ResourceController.execute(ResourceController.java:97)
          	at hudson.model.Executor.run(Executor.java:421)

           

          As the job directory is not supposed to be in this state, I can understand that this behavior change is not necessarily a bug. However the first issue looks like a bug.

          Pierre Beitz added a comment - - edited dnusbaum I did some additional tests and found out that the issue I found, was already present, but was outlined by another one (or a behavior change?) introduced in 2.91. Here are the details: Issue 1: tar breaking the symlinks (not a regression, at least since 2.60) This is the issue I initially reported, except it's not a regression. I found out that a symlink to a directory is tarred into a directory. Here is a unit test outlining that. Note that the test is red since 2.60 at least:   import hudson.FilePath; import org.junit.Assert; import org.junit.Rule; import org.junit.rules.TemporaryFolder; import java.io.IOException; import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; public class Test { @Rule public TemporaryFolder folder= new TemporaryFolder(); @org.junit.Test public void aSymlinkToADirectoryFails() throws IOException, InterruptedException { Path folderToTar = folder.newFolder().toPath(); Path subdirectory = Files.createDirectory(folderToTar.resolve( "subdirectory" )); Path targetDirectory = subdirectory.resolve( "1" ); Files.createDirectory(targetDirectory); Files.createFile(targetDirectory.resolve( "oneFile" )); Path link = Files.createSymbolicLink(subdirectory.resolve( "link" ), Paths.get( "1" )); Assert.assertTrue(Files.isSymbolicLink(link)); Path tarFile = folderToTar.resolve( "archive.tar" ); FilePath tarFilePath = new FilePath(tarFile.toFile()); try (OutputStream os = tarFilePath.write()) { new FilePath( folderToTar.toFile()).tar(os, "**/*" ); } Path untarredFolder = folder.getRoot().toPath().resolve( "untarredFolder" ); tarFilePath.untar( new FilePath(untarredFolder.toFile()), FilePath.TarCompression.NONE); Assert.assertTrue(Files.isSymbolicLink(untarredFolder.resolve( "subdirectory/link" ))); } @org.junit.Test public void aSymlinkToAFileWorks() throws IOException, InterruptedException { Path folderToTar = folder.newFolder().toPath(); Path subdirectory = Files.createDirectory(folderToTar.resolve( "subdirectory" )); Path targetFile = subdirectory.resolve( "1" ); Files.createFile(targetFile); Path link = Files.createSymbolicLink(subdirectory.resolve( "link" ), Paths.get( "1" )); Assert.assertTrue(Files.isSymbolicLink(link)); Path tarFile = folderToTar.resolve( "archive.tar" ); FilePath tarFilePath = new FilePath(tarFile.toFile()); try (OutputStream os = tarFilePath.write()) { new FilePath( folderToTar.toFile()).tar(os, "**/*" ); } Path untarredFolder = folder.getRoot().toPath().resolve( "untarredFolder" ); tarFilePath.untar( new FilePath(untarredFolder.toFile()), FilePath.TarCompression.NONE); Assert.assertTrue(Files.isSymbolicLink(untarredFolder.resolve( "subdirectory/link" ))); } } Issue 2: behavior change in 2.91 The shelve plugin is using the tar function we just saw to archive job directories. Upon unarchiving, a job directory looks therefore like this: drwxr-xr-x 7 pierrebeitz staff 224 27 jul 23:50 . drwxr-xr-x 7 pierrebeitz staff 224 27 jul 23:50 .. drwxr-xr-x 9 pierrebeitz staff 288 27 jul 23:50 builds -rw-r--r-- 1 pierrebeitz staff 497 27 jul 23:42 config.xml drwxr-xr-x 5 pierrebeitz staff 160 27 jul 23:50 lastStable drwxr-xr-x 5 pierrebeitz staff 160 27 jul 23:50 lastSuccessful -rw-r--r-- 1 pierrebeitz staff 2 27 jul 23:42 nextBuildNumber Instead of: drwxr-xr-x 7 pierrebeitz staff 224 27 jul 23:42 . drwxr-xr-x 7 pierrebeitz staff 224 27 jul 23:44 .. drwxr-xr-x 9 pierrebeitz staff 288 27 jul 23:42 builds -rw-r--r-- 1 pierrebeitz staff 497 27 jul 23:42 config.xml lrwxr-xr-x 1 pierrebeitz staff 22 27 jul 23:42 lastStable -> builds/lastStableBuild lrwxr-xr-x 1 pierrebeitz staff 26 27 jul 23:42 lastSuccessful -> builds/lastSuccessfulBuild -rw-r--r-- 1 pierrebeitz staff 2 27 jul 23:42 nextBuildNumber   The thing is, before 2.91, when launching a new run of the job, Jenkins was somehow capable of deleting those directories and recreating the symlinks. Now, it is unable of doing so: ln builds/lastSuccessfulBuild ***/jenkins-home/jobs/toto/lastSuccessful failed java.nio.file.DirectoryNotEmptyException: ***/jenkins-home/jobs/toto/lastSuccessful at sun.nio.fs.UnixFileSystemProvider.implDelete(UnixFileSystemProvider.java:242) at sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:108) at java.nio.file.Files.deleteIfExists(Files.java:1165) at hudson.Util.createSymlink(Util.java:1333) at hudson.model.Run.createSymlink(Run.java:1866) at hudson.model.Run.updateSymlinks(Run.java:1847) at hudson.model.Run.execute(Run.java:1725) at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43) at hudson.model.ResourceController.execute(ResourceController.java:97) at hudson.model.Executor.run(Executor.java:421)   As the job directory is not supposed to be in this state, I can understand that this behavior change is not necessarily a bug. However the first issue looks like a bug.

          Devin Nusbaum added a comment -

          My guess is that before 2.91, this code was executed if the path for the new symlink was currently a directory, but now we just call Files#deleteIfExists, which fails for directories. We could reintroduce similar behavior so that things would work the same as before 2.91, but as you noted that wouldn't fix the main issue.

          Devin Nusbaum added a comment - My guess is that before 2.91, this code was executed if the path for the new symlink was currently a directory, but now we just call Files#deleteIfExists , which fails for directories. We could reintroduce similar behavior so that things would work the same as before 2.91, but as you noted that wouldn't fix the main issue.

          Pierre Beitz added a comment - - edited

          dnusbaum

           

          I dig a bit deeper on the symlink issue. It turns out that if ant is set to follow the symlinks, it will consider a symlink to a directory as a 'normal directory' when creating the FileSet, therefore, there is no way for the callee to know afterward if a directory is a real one or not (ant code here

           

          I found out that asking ant not to follow the symlink, I could then retrieve them on a separate collection in the FileSet and treat them myself. Here is a small patch demonstrating what I'm saying:

           

          Index: core/src/main/java/hudson/util/DirScanner.java
          IDEA additional info:
          Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
          <+>UTF-8
          ===================================================================
          --- core/src/main/java/hudson/util/DirScanner.java	(revision 94abd9119082211e24b1bffc6d33d6905446ef08)
          +++ core/src/main/java/hudson/util/DirScanner.java	(date 1532794865000)
          @@ -8,6 +8,7 @@
           import java.io.FileFilter;
           import java.io.IOException;
           import java.io.Serializable;
          +import java.nio.file.Paths; import static hudson.Util.fixEmpty;@@ -29,6 +30,11 @@
                * @since 1.532
                */
               protected final void scanSingle(File f, String relative, FileVisitor visitor) throws IOException {
          +        if (scanSymlink(f, relative, visitor)) return;
          +        visitor.visit(f, relative);
          +    }
          +
          +    protected boolean scanSymlink(File f, String relative, FileVisitor visitor) throws IOException {
                   if (visitor.understandsSymlink()) {
                       String target;
                       try {
          @@ -38,10 +44,10 @@
                       }
                       if (target != null) {
                           visitor.visitSymlink(f, target, relative);
          -                return;
          +                return true;
                       }
                   }
          -        visitor.visit(f, relative);
          +        return false;
               }     /**
          @@ -120,7 +126,12 @@
                       fs.setDefaultexcludes(useDefaultExcludes);             if(dir.exists()) {
          +                fs.setFollowSymlinks(false);
                           DirectoryScanner ds = fs.getDirectoryScanner(new org.apache.tools.ant.Project());
          +                for(String f: ds.getNotFollowedSymlinks()) {
          +                    File file = dir.toPath().relativize(Paths.get(f)).toFile();
          +                    scanSymlink(new File(f), file.getPath(), visitor);
          +                }
                           for( String f : ds.getIncludedFiles()) {
                               File file = new File(dir, f);
                               scanSingle(file, f, visitor);

           

           

          Pierre Beitz added a comment - - edited dnusbaum   I dig a bit deeper on the symlink issue. It turns out that if ant is set to follow the symlinks, it will consider a symlink to a directory as a 'normal directory' when creating the FileSet, therefore, there is no way for the callee to know afterward if a directory is a real one or not ( ant code here   I found out that asking ant not to follow the symlink, I could then retrieve them on a separate collection in the FileSet and treat them myself. Here is a small patch demonstrating what I'm saying:   Index: core/src/main/java/hudson/util/DirScanner.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- core/src/main/java/hudson/util/DirScanner.java (revision 94abd9119082211e24b1bffc6d33d6905446ef08) +++ core/src/main/java/hudson/util/DirScanner.java (date 1532794865000) @@ -8,6 +8,7 @@ import java.io.FileFilter; import java.io.IOException; import java.io.Serializable; + import java.nio.file.Paths; import static hudson.Util.fixEmpty;@@ -29,6 +30,11 @@ * @since 1.532 */ protected final void scanSingle(File f, String relative, FileVisitor visitor) throws IOException { + if (scanSymlink(f, relative, visitor)) return ; + visitor.visit(f, relative); + } + + protected boolean scanSymlink(File f, String relative, FileVisitor visitor) throws IOException { if (visitor.understandsSymlink()) { String target; try { @@ -38,10 +44,10 @@ } if (target != null ) { visitor.visitSymlink(f, target, relative); - return ; + return true ; } } - visitor.visit(f, relative); + return false ; } /** @@ -120,7 +126,12 @@ fs.setDefaultexcludes(useDefaultExcludes); if (dir.exists()) { + fs.setFollowSymlinks( false ); DirectoryScanner ds = fs.getDirectoryScanner( new org.apache.tools.ant.Project()); + for ( String f: ds.getNotFollowedSymlinks()) { + File file = dir.toPath().relativize(Paths.get(f)).toFile(); + scanSymlink( new File(f), file.getPath(), visitor); + } for ( String f : ds.getIncludedFiles()) { File file = new File(dir, f); scanSingle(file, f, visitor);    

          Devin Nusbaum added a comment -

          pierrebtz So if I understand correctly, the problem is that Archiver#visitSymlink does not even get called for symlinks when their target is a directory? If so, then something along the lines of what you proposed seems like the best approach. Do you know when the problem was first introduced?

          Devin Nusbaum added a comment - pierrebtz So if I understand correctly, the problem is that Archiver#visitSymlink does not even get called for symlinks when their target is a directory? If so, then something along the lines of what you proposed seems like the best approach. Do you know when the problem was first introduced?

          Pierre Beitz added a comment -

          dnusbaum: I just issued a PR, attaching it to this issue (I though there was a bot doing that for me, but it seems I was wrong).

          Pierre Beitz added a comment - dnusbaum : I just issued a PR, attaching it to this issue (I though there was a bot doing that for me, but it seems I was wrong).

          This seems to have been In Review for for 5 months now.  Is it really in review or is it stalled for some reason?  Where is the PR?

          Brian J Murrell added a comment - This seems to have been In Review for for 5 months now.  Is it really in review or is it stalled for some reason?  Where is the PR?

          Pierre Beitz added a comment -

          brianjmurrell Review is stalled, the changes are a bit touchy and would need to be reviewed/discussed with more people. PR is linked to the ticket: https://github.com/jenkinsci/jenkins/pull/3569

          Pierre Beitz added a comment - brianjmurrell Review is stalled, the changes are a bit touchy and would need to be reviewed/discussed with more people. PR is linked to the ticket:  https://github.com/jenkinsci/jenkins/pull/3569

          Pierre Beitz added a comment -

          As discussed in the associated PR this work will be superseded by https://issues.jenkins-ci.org/browse/JENKINS-37862.

          Pierre Beitz added a comment - As discussed in the associated PR  this work will be superseded by  https://issues.jenkins-ci.org/browse/JENKINS-37862 .

          Jesse Glick added a comment -

          pierrebtz the Shelve plugin could probably work around this issue by excluding known symlink filenames from its patternset.

          Jesse Glick added a comment - pierrebtz the Shelve plugin could probably work around this issue by excluding known symlink filenames from its patternset.

          Pierre Beitz added a comment -

          jglick you're right. Added this in the plugin: https://github.com/jenkinsci/shelve-project-plugin/pull/19

          Pierre Beitz added a comment - jglick you're right. Added this in the plugin:  https://github.com/jenkinsci/shelve-project-plugin/pull/19

            pierrebtz Pierre Beitz
            pierrebtz Pierre Beitz
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Created:
              Updated:
              Resolved: