From e6eaf251acb3b7054c8c5ced58a49c054b5f23b1 Mon Sep 17 00:00:00 2001 From: James Judd Date: Mon, 7 May 2018 05:22:14 -0700 Subject: [PATCH] Sort entries by segment when building a parent node to prevent unordered directory structures. When building a parent node from action inputs, the paths to the files are sorted. These paths are then broken down into segments and a tree structure is created from the segments. Problem is, the segments at each level of the tree structure are not sorted before they are added to the parent node. This can result in an unordered directory tree. For example, the sort order of this list of files ``` /foo/bar-client/bar-client_ijar.jar /foo/bar/bar_ijar.jar ``` is maintained when it becomes a tree structure ``` foo -> bar-client -> bar-client_ijar.jar bar bar_ijar.jar ``` which is out of order. Resolves: #5109 Closes #5110. PiperOrigin-RevId: 195649710 --- .../build/lib/remote/TreeNodeRepository.java | 5 ++++ .../lib/remote/TreeNodeRepositoryTest.java | 30 ++++++++++++++++--- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java b/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java index 7767cb8c7b4991..5012cb74d5a5ee 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java +++ b/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java @@ -42,6 +42,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.List; @@ -123,6 +124,9 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(segment, child); } + + public static Comparator segmentOrder = + Comparator.comparing(ChildEntry::getSegment); } // Should only be called by the TreeNodeRepository. @@ -344,6 +348,7 @@ private TreeNode buildParentNode( } } } + Collections.sort(entries, TreeNode.ChildEntry.segmentOrder); return interner.intern(new TreeNode(entries, null)); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java b/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java index 858c922d258a28..4fa03824668362 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java @@ -151,13 +151,20 @@ public void testDirectoryInput() throws Exception { ActionInput fooH = ActionInputHelper.fromPath("/exec/root/a/foo/foo.h"); scratch.file("/exec/root/a/foo/foo.cc", "2"); ActionInput fooCc = ActionInputHelper.fromPath("/exec/root/a/foo/foo.cc"); + Artifact bar = new Artifact(scratch.file("/exec/root/a/bar.txt"), rootDir); TreeNodeRepository repo = createTestTreeNodeRepository(); - TreeNode root = buildFromActionInputs(repo, foo, bar); + Artifact aClient = new Artifact(scratch.dir("/exec/root/a-client"), rootDir); + scratch.file("/exec/root/a-client/baz.txt", "3"); + ActionInput baz = ActionInputHelper.fromPath("/exec/root/a-client/baz.txt"); + + TreeNode root = buildFromActionInputs(repo, foo, aClient, bar); TreeNode aNode = root.getChildEntries().get(0).getChild(); TreeNode fooNode = aNode.getChildEntries().get(1).getChild(); // foo > bar in sort order! TreeNode barNode = aNode.getChildEntries().get(0).getChild(); + TreeNode aClientNode = root.getChildEntries().get(1).getChild(); // a-client > a in sort order + TreeNode bazNode = aClientNode.getChildEntries().get(0).getChild(); TreeNode fooHNode = fooNode.getChildEntries().get(1).getChild(); // foo.h > foo.cc in sort order! @@ -170,18 +177,30 @@ public void testDirectoryInput() throws Exception { Digest fooDigest = repo.getMerkleDigest(fooNode); Digest fooHDigest = repo.getMerkleDigest(fooHNode); Digest fooCcDigest = repo.getMerkleDigest(fooCcNode); + Digest aClientDigest = repo.getMerkleDigest(aClientNode); + Digest bazDigest = repo.getMerkleDigest(bazNode); Digest barDigest = repo.getMerkleDigest(barNode); assertThat(digests) - .containsExactly(rootDigest, aDigest, barDigest, fooDigest, fooHDigest, fooCcDigest); + .containsExactly( + rootDigest, + aDigest, + barDigest, + fooDigest, + fooCcDigest, + fooHDigest, + aClientDigest, + bazDigest); ArrayList directories = new ArrayList<>(); ArrayList actionInputs = new ArrayList<>(); repo.getDataFromDigests(digests, actionInputs, directories); - assertThat(actionInputs).containsExactly(bar, fooH, fooCc); - assertThat(directories).hasSize(3); // root, root/a and root/a/foo + assertThat(actionInputs).containsExactly(bar, fooH, fooCc, baz); + assertThat(directories).hasSize(4); // root, root/a, root/a/foo, and root/a-client Directory rootDirectory = directories.get(0); assertThat(rootDirectory.getDirectories(0).getName()).isEqualTo("a"); assertThat(rootDirectory.getDirectories(0).getDigest()).isEqualTo(aDigest); + assertThat(rootDirectory.getDirectories(1).getName()).isEqualTo("a-client"); + assertThat(rootDirectory.getDirectories(1).getDigest()).isEqualTo(aClientDigest); Directory aDirectory = directories.get(1); assertThat(aDirectory.getFiles(0).getName()).isEqualTo("bar.txt"); assertThat(aDirectory.getFiles(0).getDigest()).isEqualTo(barDigest); @@ -192,5 +211,8 @@ public void testDirectoryInput() throws Exception { assertThat(fooDirectory.getFiles(0).getDigest()).isEqualTo(fooCcDigest); assertThat(fooDirectory.getFiles(1).getName()).isEqualTo("foo.h"); assertThat(fooDirectory.getFiles(1).getDigest()).isEqualTo(fooHDigest); + Directory aClientDirectory = directories.get(3); + assertThat(aClientDirectory.getFiles(0).getName()).isEqualTo("baz.txt"); + assertThat(aClientDirectory.getFiles(0).getDigest()).isEqualTo(bazDigest); } }