Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: JENKINS-73837 - Adjusted DirScanner to allow empty directories in the TarArchiver #9836

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
82dbd2f
feat: JENKINS-73837 - Adjusted DirScanner to allow empty directories …
DerSimeon Oct 6, 2024
4f3652e
fix: Fixed spotless format violations
DerSimeon Oct 6, 2024
4328df6
fix: Fixed most VirtualFileTests
DerSimeon Oct 7, 2024
4cea26a
feat: JENKINS-73837 - Updated the testEOFbrokenFlush Test
DerSimeon Oct 8, 2024
20bf90d
fix: JENKINS-73837 - Fixed most of the DirectoryBrowserSupportTests
DerSimeon Oct 8, 2024
14c5169
fix: Updated VirtualFileTests and DirectoryBrowserSupportTests to all…
DerSimeon Oct 9, 2024
6a65f48
Merge branch 'jenkinsci:master' into fix/JENKINS-73837-empty-dirs-in-…
DerSimeon Oct 9, 2024
879bbda
fix: JENKINS-73837 - removed unused import to fix spotbugs check
DerSimeon Oct 9, 2024
3f887f4
fix: Updated VirtualFile#collectFiles to fix VirtualFileTest#list
DerSimeon Oct 13, 2024
139ebe8
Merge branch 'master' into fix/JENKINS-73837-empty-dirs-in-tararchiver
DerSimeon Oct 13, 2024
b78d5d6
fix: removed duplicated test, fixed Spotless check
DerSimeon Oct 13, 2024
0dc6d7d
feat: added zip test to also close JENKINS-49296
DerSimeon Oct 14, 2024
67713a6
chore: removed unused imports
DerSimeon Oct 14, 2024
53f875e
fix: Reverted VirtualFile formatting
DerSimeon Oct 15, 2024
63ccc18
chore: dropped unneeded comments
DerSimeon Oct 15, 2024
384cafe
fix: Adjusted ZipArchiverTest#emptyDirectory to properly close inputs…
DerSimeon Oct 15, 2024
5340b07
fix: moved emptyDirectory test to reduce diff size
DerSimeon Oct 15, 2024
f286a85
fix: Adjusted TarArchiverTest#emptyDirectory to properly close inputs…
DerSimeon Oct 15, 2024
542ef5a
chore: fix spotless
DerSimeon Oct 15, 2024
ab6f1e7
Merge branch 'master' into fix/JENKINS-73837-empty-dirs-in-tararchiver
timja Oct 15, 2024
2cf54e6
Merge branch 'master' into fix/JENKINS-73837-empty-dirs-in-tararchiver
timja Oct 15, 2024
45d34c6
Revert unnecessary changes to minimize diff
basil Oct 15, 2024
14cf0bf
Strengthen assertions
basil Oct 15, 2024
52fa89e
No need to call collectFiles on a file
basil Oct 15, 2024
62f840b
chore: strengthen assertions in DirectoryBrowserSupportTest
DerSimeon Oct 15, 2024
dc860dc
Merge branch 'master' into fix/JENKINS-73837-empty-dirs-in-tararchiver
basil Oct 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions core/src/main/java/hudson/util/DirScanner.java
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,12 @@ public void scan(File dir, FileVisitor visitor) throws IOException {
File file = new File(dir, f);
scanSingle(file, f, visitor);
}
for (String d : ds.getIncludedDirectories()) {
if (!d.isEmpty()) {
File file = new File(dir, d);
scanSingle(file, d, visitor);
}
}
}
}

Expand Down
5 changes: 2 additions & 3 deletions core/src/main/java/jenkins/util/VirtualFile.java
DerSimeon marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -327,9 +327,8 @@ public Collection<String> call() throws IOException {

private static void collectFiles(VirtualFile d, Collection<String> names, String prefix) throws IOException {
for (VirtualFile child : d.list()) {
if (child.isFile()) {
names.add(prefix + child.getName());
} else if (child.isDirectory()) {
names.add(prefix + child.getName());
if (child.isDirectory()) {
collectFiles(child, names, prefix + child.getName() + "/");
}
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/hudson/FilePathTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ public void testEOFbrokenFlush() throws IOException, InterruptedException {
// Compress archive
final FilePath tmpDirPath = new FilePath(srcFolder);
int tarred = tmpDirPath.tar(Files.newOutputStream(archive.toPath()), "**");
assertEquals("One file should have been compressed", 3, tarred);
assertEquals("One file should have been compressed", 4, tarred);

// Decompress
final File dstFolder = temp.newFolder("dst");
Expand Down
2 changes: 0 additions & 2 deletions core/src/test/java/hudson/util/io/TarArchiverTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
import java.util.Set;
import org.apache.tools.tar.TarEntry;
import org.apache.tools.tar.TarInputStream;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
Expand Down Expand Up @@ -129,7 +128,6 @@ private static void run(FilePath dir, String... cmds) throws InterruptedExceptio
}
}

@Ignore("TODO fails to add empty directories to archive")
@Issue("JENKINS-73837")
@Test
public void emptyDirectory() throws Exception {
Expand Down
2 changes: 0 additions & 2 deletions core/src/test/java/hudson/util/io/ZipArchiverTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import java.util.zip.ZipFile;
import java.util.zip.ZipInputStream;
import org.junit.Assume;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
Expand Down Expand Up @@ -81,7 +80,6 @@ public void huge64bitFile() throws IOException {
}
}

@Ignore("TODO fails to add empty directories to archive")
@Issue("JENKINS-49296")
@Test
public void emptyDirectory() throws Exception {
Expand Down
42 changes: 27 additions & 15 deletions core/src/test/java/jenkins/util/VirtualFileTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -132,16 +132,16 @@ private static String modeString(int mode) throws IOException {
System.err.println("testing " + vf.getClass().getName());
assertEquals("[.hg/config.txt, sub/mid.txt, sub/subsub/lowest.txt, top.txt]", new TreeSet<>(vf.list("**/*.txt", null, false)).toString());
assertEquals("[sub/mid.txt, sub/subsub/lowest.txt, top.txt]", new TreeSet<>(vf.list("**/*.txt", null, true)).toString());
assertEquals("[.hg/config.txt, sub/mid.txt, sub/subsub/lowest.txt, top.txt, very/deep/path/here]", new TreeSet<>(vf.list("**", null, false)).toString());
assertEquals("[.hg, .hg/config.txt, sub, sub/mid.txt, sub/subsub, sub/subsub/lowest.txt, top.txt, very, very/deep, very/deep/path, very/deep/path/here]", new TreeSet<>(vf.list("**", null, false)).toString());
assertEquals("[]", new TreeSet<>(vf.list("", null, false)).toString());
assertEquals("[sub/mid.txt, sub/subsub/lowest.txt]", new TreeSet<>(vf.list("sub/", null, false)).toString());
assertEquals("[sub/mid.txt]", new TreeSet<>(vf.list("sub/", "sub/subsub/", false)).toString());
assertEquals("[sub/mid.txt]", new TreeSet<>(vf.list("sub/", "sub/subsub/**", false)).toString());
assertEquals("[sub/mid.txt]", new TreeSet<>(vf.list("sub/", "**/subsub/", false)).toString());
assertEquals("[sub, sub/mid.txt, sub/subsub, sub/subsub/lowest.txt]", new TreeSet<>(vf.list("sub/", null, false)).toString());
assertEquals("[sub, sub/mid.txt]", new TreeSet<>(vf.list("sub/", "sub/subsub/", false)).toString());
assertEquals("[sub, sub/mid.txt]", new TreeSet<>(vf.list("sub/", "sub/subsub/**", false)).toString());
assertEquals("[sub, sub/mid.txt]", new TreeSet<>(vf.list("sub/", "**/subsub/", false)).toString());
assertEquals("[.hg/config.txt, sub/mid.txt]", new TreeSet<>(vf.list("**/mid*,**/conf*", null, false)).toString());
assertEquals("[sub/mid.txt, sub/subsub/lowest.txt]", new TreeSet<>(vf.list("sub/", "**/notthere/", false)).toString());
assertEquals("[sub, sub/mid.txt, sub/subsub, sub/subsub/lowest.txt]", new TreeSet<>(vf.list("sub/", "**/notthere/", false)).toString());
assertEquals("[top.txt]", new TreeSet<>(vf.list("*.txt", null, false)).toString());
assertEquals("[sub/subsub/lowest.txt, top.txt, very/deep/path/here]", new TreeSet<>(vf.list("**", "**/mid*,**/conf*", false)).toString());
assertEquals("[.hg, sub, sub/subsub, sub/subsub/lowest.txt, top.txt, very, very/deep, very/deep/path, very/deep/path/here]", new TreeSet<>(vf.list("**", "**/mid*,**/conf*", false)).toString());
}
}
/** Roughly analogous to {@code org.jenkinsci.plugins.compress_artifacts.ZipStorage}. */
Expand Down Expand Up @@ -240,7 +240,13 @@ public void list_Glob_NoFollowLinks_FileVF() throws Exception {
assertThat(children, containsInAnyOrder(
"a/aa/aa.txt",
"a/ab/ab.txt",
"b/ba/ba.txt"
"b/ba/ba.txt",
"a",
"a/aa",
"a/aa/aaa",
"a/ab",
"b",
"b/ba"
));
}

Expand All @@ -256,7 +262,13 @@ public void list_Glob_NoFollowLinks_FilePathVF() throws Exception {
assertThat(children, containsInAnyOrder(
"a/aa/aa.txt",
"a/ab/ab.txt",
"b/ba/ba.txt"
"b/ba/ba.txt",
"a",
"a/aa",
"a/aa/aaa",
"a/ab",
"b",
"b/ba"
));
}

Expand All @@ -281,7 +293,7 @@ public void zip_NoFollowLinks_FilePathVF() throws Exception {
assertTrue(unzipPath.isDirectory());
assertTrue(unzipPath.child("a").child("aa").child("aa.txt").exists());
assertTrue(unzipPath.child("a").child("ab").child("ab.txt").exists());
assertFalse(unzipPath.child("a").child("aa").child("aaa").exists());
assertTrue(unzipPath.child("a").child("aa").child("aaa").exists());
assertFalse(unzipPath.child("a").child("_b").exists());
assertTrue(unzipPath.child("b").child("ba").child("ba.txt").exists());
assertFalse(unzipPath.child("b").child("_a").exists());
Expand Down Expand Up @@ -311,7 +323,7 @@ public void zip_NoFollowLinks_FilePathVF_withPrefix() throws Exception {
assertTrue(unzipPath.child(prefix).isDirectory());
assertTrue(unzipPath.child(prefix).child("a").child("aa").child("aa.txt").exists());
assertTrue(unzipPath.child(prefix).child("a").child("ab").child("ab.txt").exists());
assertFalse(unzipPath.child(prefix).child("a").child("aa").child("aaa").exists());
assertTrue(unzipPath.child(prefix).child("a").child("aa").child("aaa").exists());
assertFalse(unzipPath.child(prefix).child("a").child("_b").exists());
assertTrue(unzipPath.child(prefix).child("b").child("ba").child("ba.txt").exists());
assertFalse(unzipPath.child(prefix).child("b").child("_a").exists());
Expand Down Expand Up @@ -339,7 +351,7 @@ public void zip_NoFollowLinks_FileVF() throws Exception {
assertTrue(unzipPath.isDirectory());
assertTrue(unzipPath.child("a").child("aa").child("aa.txt").exists());
assertTrue(unzipPath.child("a").child("ab").child("ab.txt").exists());
assertFalse(unzipPath.child("a").child("aa").child("aaa").exists());
assertTrue(unzipPath.child("a").child("aa").child("aaa").exists());
assertFalse(unzipPath.child("a").child("_b").exists());
assertTrue(unzipPath.child("b").child("ba").child("ba.txt").exists());
assertFalse(unzipPath.child("b").child("_a").exists());
Expand Down Expand Up @@ -369,7 +381,7 @@ public void zip_NoFollowLinks_FileVF_withPrefix() throws Exception {
assertTrue(unzipPath.child(prefix).isDirectory());
assertTrue(unzipPath.child(prefix).child("a").child("aa").child("aa.txt").exists());
assertTrue(unzipPath.child(prefix).child("a").child("ab").child("ab.txt").exists());
assertFalse(unzipPath.child(prefix).child("a").child("aa").child("aaa").exists());
assertTrue(unzipPath.child(prefix).child("a").child("aa").child("aaa").exists());
assertFalse(unzipPath.child(prefix).child("a").child("_b").exists());
assertTrue(unzipPath.child(prefix).child("b").child("ba").child("ba.txt").exists());
assertFalse(unzipPath.child(prefix).child("b").child("_a").exists());
Expand Down Expand Up @@ -504,7 +516,7 @@ public void list_Glob_NoFollowLinks_ExternalSymlink_FilePathVF() throws Exceptio
VirtualFile symlinkVirtualPath = VirtualFile.forFilePath(symlinkPath);
VirtualFile symlinkChildVirtualPath = symlinkVirtualPath.child("aa");
Collection<String> children = symlinkChildVirtualPath.list("**", null, true, LinkOption.NOFOLLOW_LINKS);
assertThat(children, contains("aa.txt"));
assertThat(children, containsInAnyOrder("aaa", "aa.txt"));
}

@Test
Expand All @@ -519,7 +531,7 @@ public void list_Glob_NoFollowLinks_ExternalSymlink_FileVF() throws Exception {
VirtualFile symlinkVirtualFile = VirtualFile.forFile(symlinkFile);
VirtualFile symlinkChildVirtualFile = symlinkVirtualFile.child("aa");
Collection<String> children = symlinkChildVirtualFile.list("**", null, true, LinkOption.NOFOLLOW_LINKS);
assertThat(children, contains("aa.txt"));
assertThat(children, containsInAnyOrder("aaa", "aa.txt"));
}

@Test
Expand Down
33 changes: 21 additions & 12 deletions test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -687,7 +686,9 @@ public void symlink_outsideWorkspace_areNotAllowed() throws Exception {
List<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, containsInAnyOrder(
p.getName() + "/intermediateFolder/public2.key",
p.getName() + "/public1.key"
p.getName() + "/public1.key",
p.getName() + "/intermediateFolder/",
p.getName() + "/intermediateFolder/otherFolder/"
));
}
{ // workaround for JENKINS-19947 is still supported, i.e. no parent folder
Expand All @@ -697,22 +698,25 @@ public void symlink_outsideWorkspace_areNotAllowed() throws Exception {
List<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, containsInAnyOrder(
"intermediateFolder/public2.key",
"public1.key"
"public1.key",
"intermediateFolder/",
"intermediateFolder/otherFolder/"

));
}
{ // all the outside folders / files are not included in the zip
Page zipPage = wc.goTo(p.getUrl() + "ws/intermediateFolder/*zip*/intermediateFolder.zip", null);
assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK));

List<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, contains("intermediateFolder/public2.key"));
assertThat(entryNames, containsInAnyOrder("intermediateFolder/public2.key", "intermediateFolder/otherFolder/"));
}
{ // workaround for JENKINS-19947 is still supported, i.e. no parent folder, even inside a sub-folder
Page zipPage = wc.goTo(p.getUrl() + "ws/intermediateFolder/**/*zip*/intermediateFolder.zip", null);
assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK));

List<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, contains("public2.key"));
assertThat(entryNames, containsInAnyOrder("public2.key", "otherFolder/"));
}
}

Expand Down Expand Up @@ -917,15 +921,17 @@ public void junctionAndSymlink_outsideWorkspace_areNotAllowed_windowsJunction()
List<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, containsInAnyOrder(
p.getName() + "/intermediateFolder/public2.key",
p.getName() + "/public1.key"
p.getName() + "/public1.key",
p.getName() + "/intermediateFolder/",
p.getName() + "/intermediateFolder/otherFolder/"
));
}
{ // all the outside folders / files are not included in the zip
Page zipPage = wc.goTo(p.getUrl() + "ws/intermediateFolder/*zip*/intermediateFolder.zip", null);
assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK));

List<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, contains("intermediateFolder/public2.key"));
assertThat(entryNames, containsInAnyOrder("intermediateFolder/public2.key", "intermediateFolder/otherFolder/"));
}
// Explicitly delete everything including junctions, which TemporaryDirectoryAllocator.dispose may have trouble with:
new Launcher.LocalLauncher(StreamTaskListener.fromStderr()).launch().cmds("cmd", "/c", "rmdir", "/s", "/q", j.jenkins.getRootDir().getAbsolutePath()).start().join();
Expand Down Expand Up @@ -987,7 +993,7 @@ public void directSymlink_forTestingZip() throws Exception {
assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK));

List<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, hasSize(0));
assertThat(entryNames, hasSize(6));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add a containsInAnyOrder assertion after this to match the expected 6 entries, like the others in this file.

}
{
Page zipPage = wc.goTo(p.getUrl() + "ws/a1/*zip*/a1.zip", null);
Expand Down Expand Up @@ -1270,20 +1276,23 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK));

List<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, hasSize(2));
assertThat(entryNames, hasSize(4));
assertThat(entryNames, containsInAnyOrder(
"test0/anotherDir/one.txt",
"test0/anotherDir/insideDir/two.txt"
"test0/anotherDir/insideDir/two.txt",
"test0/anotherDir/",
"test0/anotherDir/insideDir/"
));

zipPage = wc.goTo("job/" + p.getName() + "/ws/anotherDir/*zip*/" + p.getName(), null);
assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK));

entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, hasSize(2));
assertThat(entryNames, hasSize(3));
assertThat(entryNames, containsInAnyOrder(
"anotherDir/one.txt",
"anotherDir/insideDir/two.txt"
"anotherDir/insideDir/two.txt",
"anotherDir/insideDir/"
));
}

Expand Down
Loading