From 2727758a3bf33c829388f4b0886bb3f8011de960 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Mon, 18 Jan 2021 14:23:34 +0800 Subject: [PATCH] remote: set executable bit of an input file based on its real value. When build without bytes is enabled, we use isExecutable field of OutputFile for intermediate input files. This is achieved by injecting the metadata into the MetadataProvider. The "always mark" was introduced by 3e3b71ae038bc9ac90456f93697c640ab9ed8a55 which was a workaround for #4751. However, that issue was then fixed by fc448916ae04d22743fa4ae44d954f9faedb4f3a. There is no reason to keep the workaround which is causing other issues e.g. #12818. --- .../build/lib/actions/FileArtifactValue.java | 12 ++++++++-- .../lib/actions/cache/MetadataInjector.java | 2 +- .../lib/concurrent/MultisetSemaphore.java | 2 +- .../build/lib/remote/RemoteCache.java | 6 +++-- .../lib/remote/merkletree/DirectoryTree.java | 15 +++++++++---- .../merkletree/DirectoryTreeBuilder.java | 17 ++++++++++---- .../lib/remote/merkletree/MerkleTree.java | 2 +- .../lib/skyframe/ActionMetadataHandler.java | 4 ++-- .../build/lib/skyframe/OutputStore.java | 4 ++-- .../lib/actions/util/ActionsTestUtil.java | 2 +- ...eStreamBuildEventArtifactUploaderTest.java | 6 ++--- .../remote/RemoteActionFileSystemTest.java | 10 ++++----- .../remote/RemoteActionInputFetcherTest.java | 13 ++++++----- .../build/lib/remote/RemoteCacheTests.java | 15 ++++++------- .../lib/remote/RemoteSpawnCacheTest.java | 2 +- .../ActionInputDirectoryTreeTest.java | 8 +++---- .../remote/merkletree/DirectoryTreeTest.java | 6 ++--- .../lib/remote/merkletree/MerkleTreeTest.java | 12 +++++----- .../util/FakeSpawnExecutionContext.java | 2 +- .../skyframe/ActionMetadataHandlerTest.java | 8 +++---- .../bazel/remote/remote_execution_test.sh | 22 +++++++++++++++++++ 21 files changed, 109 insertions(+), 61 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index b988f18b69c8b1..69c7ae2978083d 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -542,11 +542,13 @@ public static final class RemoteFileArtifactValue extends FileArtifactValue { private final byte[] digest; private final long size; private final int locationIndex; + private final boolean isExecutable; - public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex) { + public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex, boolean isExecutable) { this.digest = digest; this.size = size; this.locationIndex = locationIndex; + this.isExecutable = isExecutable; } @Override @@ -558,7 +560,8 @@ public boolean equals(Object o) { RemoteFileArtifactValue that = (RemoteFileArtifactValue) o; return Arrays.equals(digest, that.digest) && size == that.size - && locationIndex == that.locationIndex; + && locationIndex == that.locationIndex + && isExecutable == that.isExecutable; } @Override @@ -597,6 +600,10 @@ public int getLocationIndex() { return locationIndex; } + public boolean isExecutable() { + return isExecutable; + } + @Override public boolean wasModifiedSinceDigest(Path path) { return false; @@ -613,6 +620,7 @@ public String toString() { .add("digest", bytesToString(digest)) .add("size", size) .add("locationIndex", locationIndex) + .add("isExecutable", isExecutable) .toString(); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java index 78ed3899c1ea84..288aa7ae24d950 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java @@ -32,7 +32,7 @@ public interface MetadataInjector { * @param size the size of the file in bytes. * @param locationIndex is only used in Blaze. */ - void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex); + void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, boolean isExecutable); /** * Inject the metadata of a tree artifact whose contents are stored remotely. diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/MultisetSemaphore.java b/src/main/java/com/google/devtools/build/lib/concurrent/MultisetSemaphore.java index 94611b5ef33e4b..471a6dff5bad03 100644 --- a/src/main/java/com/google/devtools/build/lib/concurrent/MultisetSemaphore.java +++ b/src/main/java/com/google/devtools/build/lib/concurrent/MultisetSemaphore.java @@ -86,7 +86,7 @@ private Builder() { public Builder maxNumUniqueValues(int maxNumUniqueValues) { Preconditions.checkState( maxNumUniqueValues > 0, - "maxNumUniqueValues must be positive (was %d)", + "maxNumUniqueValues must be positive (was %s)", maxNumUniqueValues); this.maxNumUniqueValues = maxNumUniqueValues; return this; diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index e104671786de78..7c9a305906ab9d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -612,7 +612,8 @@ private void injectRemoteArtifact( new RemoteFileArtifactValue( DigestUtil.toBinaryDigest(file.digest()), file.digest().getSizeBytes(), - /* locationIndex= */ 1); + /* locationIndex= */ 1, + file.isExecutable()); childMetadata.put(p, r); } metadataInjector.injectRemoteDirectory( @@ -628,7 +629,8 @@ private void injectRemoteArtifact( output, DigestUtil.toBinaryDigest(outputMetadata.digest()), outputMetadata.digest().getSizeBytes(), - /* locationIndex= */ 1); + /* locationIndex= */ 1, + outputMetadata.isExecutable()); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java index 09bc166d0a6cac..256d7a002185b8 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java @@ -72,20 +72,22 @@ static class FileNode extends Node { private final Path path; private final ByteString data; private final Digest digest; + private final boolean isExecutable; - FileNode(String pathSegment, Path path, Digest digest) { + FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable) { super(pathSegment); this.path = Preconditions.checkNotNull(path, "path"); this.data = null; this.digest = Preconditions.checkNotNull(digest, "digest"); + this.isExecutable = isExecutable; } FileNode(String pathSegment, ByteString data, Digest digest) { super(pathSegment); this.path = null; this.data = Preconditions.checkNotNull(data, "data"); - ; this.digest = Preconditions.checkNotNull(digest, "digest"); + this.isExecutable = false; } Digest getDigest() { @@ -100,9 +102,13 @@ ByteString getBytes() { return data; } + public boolean isExecutable() { + return isExecutable; + } + @Override public int hashCode() { - return Objects.hash(super.hashCode(), path, data, digest); + return Objects.hash(super.hashCode(), path, data, digest, isExecutable); } @Override @@ -112,7 +118,8 @@ public boolean equals(Object o) { return super.equals(other) && Objects.equals(path, other.path) && Objects.equals(data, other.data) - && Objects.equals(digest, other.digest); + && Objects.equals(digest, other.digest) + && Objects.equals(isExecutable, other.isExecutable); } return false; } diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java index 54bbeef43277a2..14c4e1e8bde82c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java @@ -34,6 +34,8 @@ import java.util.SortedMap; import java.util.TreeMap; +import static com.google.devtools.build.lib.actions.FileArtifactValue.*; + /** Builder for directory trees. */ class DirectoryTreeBuilder { @@ -101,7 +103,7 @@ private static int buildFromPaths( throw new IOException(String.format("Input '%s' is not a file.", input)); } Digest d = digestUtil.compute(input); - currDir.addChild(new FileNode(path.getBaseName(), input, d)); + currDir.addChild(new FileNode(path.getBaseName(), input, d, input.isExecutable())); return 1; }); } @@ -139,9 +141,16 @@ private static int buildFromActionInputs( switch (metadata.getType()) { case REGULAR_FILE: Digest d = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize()); - currDir.addChild( - new FileNode( - path.getBaseName(), ActionInputHelper.toInputPath(input, execRoot), d)); + Path inputPath = ActionInputHelper.toInputPath(input, execRoot); + + boolean isExecutable; + if (metadata instanceof RemoteFileArtifactValue) { + isExecutable = ((RemoteFileArtifactValue) metadata).isExecutable(); + } else { + isExecutable = inputPath.isExecutable(); + } + + currDir.addChild(new FileNode(path.getBaseName(), inputPath, d, isExecutable)); return 1; case DIRECTORY: diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java index 7fac053ae32aa4..5b33c57e53cc18 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java @@ -198,7 +198,7 @@ private static FileNode buildProto(DirectoryTree.FileNode file) { return FileNode.newBuilder() .setName(file.getPathSegment()) .setDigest(file.getDigest()) - .setIsExecutable(true) + .setIsExecutable(file.isExecutable()) .build(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index bbf13fede32cd6..5db23535bed4e7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -541,7 +541,7 @@ public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] dig } @Override - public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) { + public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, boolean isExecutable) { Preconditions.checkArgument( isKnownOutput(output), output + " is not a declared output of this action"); Preconditions.checkArgument( @@ -550,7 +550,7 @@ public void injectRemoteFile(Artifact output, byte[] digest, long size, int loca output); Preconditions.checkState( executionMode.get(), "Tried to inject %s outside of execution", output); - store.injectRemoteFile(output, digest, size, locationIndex); + store.injectRemoteFile(output, digest, size, locationIndex, isExecutable); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java index bc7525845e60a4..f4f7ef8f86ad59 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java @@ -95,9 +95,9 @@ void addTreeArtifactContents(Artifact artifact, TreeFileArtifact contents) { treeArtifactContents.computeIfAbsent(artifact, a -> Sets.newConcurrentHashSet()).add(contents); } - void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) { + void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, boolean isExecutable) { injectOutputData( - output, new FileArtifactValue.RemoteFileArtifactValue(digest, size, locationIndex)); + output, new FileArtifactValue.RemoteFileArtifactValue(digest, size, locationIndex, isExecutable)); } final void injectOutputData(Artifact output, FileArtifactValue artifactValue) { diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index 1f73d08879d437..82ef95b6a9e96d 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java @@ -959,7 +959,7 @@ public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] dig } @Override - public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) { + public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, boolean isExecutable) { throw new UnsupportedOperationException(); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index 7fc54691a61fc4..9b50a5dfb042f2 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -282,7 +282,7 @@ public void remoteFileShouldNotBeUploaded_actionFs() throws Exception { ByteStreamBuildEventArtifactUploader artifactUploader = newArtifactUploader(uploader); ActionInputMap outputs = new ActionInputMap(2); - Artifact artifact = createRemoteArtifact("file1.txt", "foo", outputs); + Artifact artifact = createRemoteArtifact("file1.txt", "foo", outputs, false); RemoteActionFileSystem remoteFs = new RemoteActionFileSystem( @@ -354,13 +354,13 @@ public void remoteFileShouldNotBeUploaded_findMissingDigests() throws Exception /** Returns a remote artifact and puts its metadata into the action input map. */ private Artifact createRemoteArtifact( - String pathFragment, String contents, ActionInputMap inputs) { + String pathFragment, String contents, ActionInputMap inputs, boolean isExecutable) { Path p = outputRoot.getRoot().asPath().getRelative(pathFragment); Artifact a = ActionsTestUtil.createArtifact(outputRoot, p); byte[] b = contents.getBytes(StandardCharsets.UTF_8); HashCode h = HashCode.fromString(DIGEST_UTIL.compute(b).getHash()); FileArtifactValue f = - new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1); + new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1, isExecutable); inputs.putWithNoDepOwner(a, f); return a; } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index cdc5946f379ce5..e5d97c6d686159 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java @@ -67,7 +67,7 @@ public void setUp() throws IOException { public void testGetInputStream() throws Exception { // arrange ActionInputMap inputs = new ActionInputMap(2); - Artifact remoteArtifact = createRemoteArtifact("remote-file", "remote contents", inputs); + Artifact remoteArtifact = createRemoteArtifact("remote-file", "remote contents", inputs, false); Artifact localArtifact = createLocalArtifact("local-file", "local contents", inputs); FileSystem actionFs = newRemoteActionFileSystem(inputs); doAnswer( @@ -100,7 +100,7 @@ public void testGetInputStream() throws Exception { public void testCreateSymbolicLink() throws InterruptedException, IOException { // arrange ActionInputMap inputs = new ActionInputMap(1); - Artifact remoteArtifact = createRemoteArtifact("remote-file", "remote contents", inputs); + Artifact remoteArtifact = createRemoteArtifact("remote-file", "remote contents", inputs, false); Path symlink = outputRoot.getRoot().getRelative("symlink"); FileSystem actionFs = newRemoteActionFileSystem(inputs); doAnswer( @@ -130,7 +130,7 @@ public void testCreateSymbolicLink() throws InterruptedException, IOException { public void testDeleteRemoteFile() throws Exception { // arrange ActionInputMap inputs = new ActionInputMap(1); - Artifact remoteArtifact = createRemoteArtifact("remote-file", "remote contents", inputs); + Artifact remoteArtifact = createRemoteArtifact("remote-file", "remote contents", inputs, false); FileSystem actionFs = newRemoteActionFileSystem(inputs); // act @@ -166,13 +166,13 @@ private FileSystem newRemoteActionFileSystem(ActionInputMap inputs) { /** Returns a remote artifact and puts its metadata into the action input map. */ private Artifact createRemoteArtifact( - String pathFragment, String contents, ActionInputMap inputs) { + String pathFragment, String contents, ActionInputMap inputs, boolean isExecutable) { Path p = outputRoot.getRoot().asPath().getRelative(pathFragment); Artifact a = ActionsTestUtil.createArtifact(outputRoot, p); byte[] b = contents.getBytes(StandardCharsets.UTF_8); HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b); FileArtifactValue f = - new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1); + new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1, isExecutable); inputs.putWithNoDepOwner(a, f); return a; } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java index 2453cfed5e0a5c..491c4b8162d211 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java @@ -80,8 +80,8 @@ public void testFetching() throws Exception { // arrange Map metadata = new HashMap<>(); Map cacheEntries = new HashMap<>(); - Artifact a1 = createRemoteArtifact("file1", "hello world", metadata, cacheEntries); - Artifact a2 = createRemoteArtifact("file2", "fizz buzz", metadata, cacheEntries); + Artifact a1 = createRemoteArtifact("file1", "hello world", metadata, cacheEntries, false); + Artifact a2 = createRemoteArtifact("file2", "fizz buzz", metadata, cacheEntries, false); MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); RemoteCache remoteCache = newCache(options, digestUtil, cacheEntries); RemoteActionInputFetcher actionInputFetcher = @@ -129,7 +129,7 @@ public void testFileNotFound() throws Exception { // arrange Map metadata = new HashMap<>(); Artifact a = - createRemoteArtifact("file1", "hello world", metadata, /* cacheEntries= */ new HashMap<>()); + createRemoteArtifact("file1", "hello world", metadata, /* cacheEntries= */ new HashMap<>(), false); MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); RemoteCache remoteCache = newCache(options, digestUtil, new HashMap<>()); RemoteActionInputFetcher actionInputFetcher = @@ -172,7 +172,7 @@ public void testDownloadFile() throws Exception { // arrange Map metadata = new HashMap<>(); Map cacheEntries = new HashMap<>(); - Artifact a1 = createRemoteArtifact("file1", "hello world", metadata, cacheEntries); + Artifact a1 = createRemoteArtifact("file1", "hello world", metadata, cacheEntries, false); RemoteCache remoteCache = newCache(options, digestUtil, cacheEntries); RemoteActionInputFetcher actionInputFetcher = new RemoteActionInputFetcher(remoteCache, execRoot, Context.current()); @@ -192,13 +192,14 @@ private Artifact createRemoteArtifact( String pathFragment, String contents, Map metadata, - Map cacheEntries) { + Map cacheEntries, + boolean isExecutable) { Path p = artifactRoot.getRoot().getRelative(pathFragment); Artifact a = ActionsTestUtil.createArtifact(artifactRoot, p); byte[] b = contents.getBytes(StandardCharsets.UTF_8); HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b); FileArtifactValue f = - new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1); + new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1, isExecutable); metadata.put(a, f); cacheEntries.put(DigestUtil.buildDigest(h.asBytes(), b.length), ByteString.copyFrom(b)); return a; diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java index af617f49a95cbc..f9861d9a75a135 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java @@ -17,8 +17,7 @@ import static com.google.devtools.build.lib.remote.util.DigestUtil.toBinaryDigest; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertThrows; -import static org.mockito.ArgumentMatchers.anyInt; -import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.*; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -926,9 +925,9 @@ public void testDownloadMinimalFiles() throws Exception { // assert assertThat(inMemoryOutput).isNull(); verify(injector) - .injectRemoteFile(eq(a1), eq(toBinaryDigest(d1)), eq(d1.getSizeBytes()), anyInt()); + .injectRemoteFile(eq(a1), eq(toBinaryDigest(d1)), eq(d1.getSizeBytes()), anyInt(), anyBoolean()); verify(injector) - .injectRemoteFile(eq(a2), eq(toBinaryDigest(d2)), eq(d2.getSizeBytes()), anyInt()); + .injectRemoteFile(eq(a2), eq(toBinaryDigest(d2)), eq(d2.getSizeBytes()), anyInt(), anyBoolean()); Path outputBase = artifactRoot.getRoot().asPath(); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); @@ -992,10 +991,10 @@ public void testDownloadMinimalDirectory() throws Exception { ImmutableMap.builder() .put( PathFragment.create("file1"), - new RemoteFileArtifactValue(toBinaryDigest(d1), d1.getSizeBytes(), 1)) + new RemoteFileArtifactValue(toBinaryDigest(d1), d1.getSizeBytes(), 1, false)) .put( PathFragment.create("a/file2"), - new RemoteFileArtifactValue(toBinaryDigest(d2), d2.getSizeBytes(), 1)) + new RemoteFileArtifactValue(toBinaryDigest(d2), d2.getSizeBytes(), 1, false)) .build(); verify(injector).injectRemoteDirectory(eq(dir), eq(m)); @@ -1140,9 +1139,9 @@ public void testDownloadMinimalWithInMemoryOutput() throws Exception { assertThat(inMemoryOutput.getOutput()).isEqualTo(a1); // The in memory file also needs to be injected as an output verify(injector) - .injectRemoteFile(eq(a1), eq(toBinaryDigest(d1)), eq(d1.getSizeBytes()), anyInt()); + .injectRemoteFile(eq(a1), eq(toBinaryDigest(d1)), eq(d1.getSizeBytes()), anyInt(), anyBoolean()); verify(injector) - .injectRemoteFile(eq(a2), eq(toBinaryDigest(d2)), eq(d2.getSizeBytes()), anyInt()); + .injectRemoteFile(eq(a2), eq(toBinaryDigest(d2)), eq(d2.getSizeBytes()), anyInt(), anyBoolean()); Path outputBase = artifactRoot.getRoot().asPath(); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index 50fe92ceb55447..54bb06cb274f49 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -180,7 +180,7 @@ public MetadataInjector getMetadataInjector() { return new MetadataInjector() { @Override public void injectRemoteFile( - Artifact output, byte[] digest, long size, int locationIndex) { + Artifact output, byte[] digest, long size, int locationIndex, boolean isExecutable) { throw new UnsupportedOperationException(); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java b/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java index 79408f4892d636..451f5795d25855 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java @@ -71,7 +71,7 @@ public void virtualActionInputShouldWork() throws Exception { assertThat(directoriesAtDepth(1, tree)).isEmpty(); FileNode expectedFooNode = - new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo")); + new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false); FileNode expectedBarNode = new FileNode("bar.cc", bar.getBytes(), digestUtil.computeAsUtf8("bar")); assertThat(fileNodesAtDepth(tree, 0)).isEmpty(); @@ -118,13 +118,13 @@ public void directoryInputShouldBeExpanded() throws Exception { assertThat(directoriesAtDepth(3, tree)).isEmpty(); FileNode expectedFooNode = - new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo")); + new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false); FileNode expectedBarNode = new FileNode( - "bar.cc", execRoot.getRelative(bar.getExecPath()), digestUtil.computeAsUtf8("bar")); + "bar.cc", execRoot.getRelative(bar.getExecPath()), digestUtil.computeAsUtf8("bar"), false); FileNode expectedBuzzNode = new FileNode( - "buzz.cc", execRoot.getRelative(buzz.getExecPath()), digestUtil.computeAsUtf8("buzz")); + "buzz.cc", execRoot.getRelative(buzz.getExecPath()), digestUtil.computeAsUtf8("buzz"), false); assertThat(fileNodesAtDepth(tree, 0)).isEmpty(); assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode); assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBarNode); diff --git a/src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java b/src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java index 33b1c36ad11fd5..e888c5cd5dbe43 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java @@ -74,9 +74,9 @@ public void buildingATreeOfFilesShouldWork() throws Exception { assertThat(directoriesAtDepth(1, tree)).containsExactly("fizz"); assertThat(directoriesAtDepth(2, tree)).isEmpty(); - FileNode expectedFooNode = new FileNode("foo.cc", foo, digestUtil.computeAsUtf8("foo")); - FileNode expectedBarNode = new FileNode("bar.cc", bar, digestUtil.computeAsUtf8("bar")); - FileNode expectedBuzzNode = new FileNode("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz")); + FileNode expectedFooNode = new FileNode("foo.cc", foo, digestUtil.computeAsUtf8("foo"), false); + FileNode expectedBarNode = new FileNode("bar.cc", bar, digestUtil.computeAsUtf8("bar"), false); + FileNode expectedBuzzNode = new FileNode("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"), false); assertThat(fileNodesAtDepth(tree, 0)).isEmpty(); assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode, expectedBarNode); assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBuzzNode); diff --git a/src/test/java/com/google/devtools/build/lib/remote/merkletree/MerkleTreeTest.java b/src/test/java/com/google/devtools/build/lib/remote/merkletree/MerkleTreeTest.java index 754a95ce6fc3d5..49112dfa2a9109 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/merkletree/MerkleTreeTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/merkletree/MerkleTreeTest.java @@ -87,13 +87,13 @@ public void buildMerkleTree() throws IOException { Directory fizzDir = Directory.newBuilder() - .addFiles(newFileNode("buzz.cc", digestUtil.computeAsUtf8("buzz"))) - .addFiles(newFileNode("fizzbuzz.cc", digestUtil.computeAsUtf8("fizzbuzz"))) + .addFiles(newFileNode("buzz.cc", digestUtil.computeAsUtf8("buzz"), false)) + .addFiles(newFileNode("fizzbuzz.cc", digestUtil.computeAsUtf8("fizzbuzz"), false)) .build(); Directory srcsDir = Directory.newBuilder() - .addFiles(newFileNode("bar.cc", digestUtil.computeAsUtf8("bar"))) - .addFiles(newFileNode("foo.cc", digestUtil.computeAsUtf8("foo"))) + .addFiles(newFileNode("bar.cc", digestUtil.computeAsUtf8("bar"), false)) + .addFiles(newFileNode("foo.cc", digestUtil.computeAsUtf8("foo"), false)) .addDirectories( DirectoryNode.newBuilder().setName("fizz").setDigest(digestUtil.compute(fizzDir))) .build(); @@ -153,7 +153,7 @@ private Artifact addFile( return a; } - private static FileNode newFileNode(String name, Digest digest) { - return FileNode.newBuilder().setName(name).setDigest(digest).setIsExecutable(true).build(); + private static FileNode newFileNode(String name, Digest digest, boolean isExecutable) { + return FileNode.newBuilder().setName(name).setDigest(digest).setIsExecutable(isExecutable).build(); } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java b/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java index 5bab193558aa67..134d017dac222e 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java +++ b/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java @@ -132,7 +132,7 @@ public void report(ProgressStatus state, String name) { public MetadataInjector getMetadataInjector() { return new MetadataInjector() { @Override - public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex) { + public void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex, boolean isExecutable) { throw new UnsupportedOperationException(); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java index a5c43a1bc10e92..d20839f77945ad 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java @@ -332,7 +332,7 @@ public void resettingOutputs() throws Exception { assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(10); // Inject a remote file of size 42. - handler.injectRemoteFile(artifact, new byte[] {1, 2, 3}, 42, 0); + handler.injectRemoteFile(artifact, new byte[] {1, 2, 3}, 42, 0, false); assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(42); // Reset this output, which will make the handler stat the file again. @@ -358,7 +358,7 @@ public void injectRemoteArtifactMetadata() throws Exception { byte[] digest = new byte[] {1, 2, 3}; int size = 10; - handler.injectRemoteFile(artifact, digest, size, /* locationIndex= */ 1); + handler.injectRemoteFile(artifact, digest, size, /* locationIndex= */ 1, false); FileArtifactValue v = handler.getMetadata(artifact); assertThat(v).isNotNull(); @@ -386,8 +386,8 @@ public void injectRemoteTreeArtifactMetadata() throws Exception { outputRoot.getRoot().asPath()); handler.discardOutputMetadata(); - RemoteFileArtifactValue fooValue = new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1); - RemoteFileArtifactValue barValue = new RemoteFileArtifactValue(new byte[] {4, 5, 6}, 10, 1); + RemoteFileArtifactValue fooValue = new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1, false); + RemoteFileArtifactValue barValue = new RemoteFileArtifactValue(new byte[] {4, 5, 6}, 10, 1, false); Map children = ImmutableMap.builder() .put(PathFragment.create("foo"), fooValue) diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index f42ea8af79e56b..6bddd7ea549d97 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -1841,6 +1841,28 @@ EOF @default_foo//:all } +function test_remote_input_files_executable_bit() { + # Test that input files uploaded to remote executor have the same executable bit with local files. #12818 + touch WORKSPACE + cat > BUILD <<'EOF' +genrule( + name = "test", + srcs = ["foo.txt", "bar.sh"], + outs = ["out.txt"], + cmd = "ls -l $(SRCS); touch $@", +) +EOF + touch foo.txt bar.sh + chmod a+x bar.sh + + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + //:test >& $TEST_log || fail "Failed to build //:test" + + expect_log "-rwxr--r-- .* bar.sh" + expect_log "-rw-r--r-- .* foo.txt" +} + # TODO(alpha): Add a test that fails remote execution when remote worker # supports sandbox.