From ceaac966a7b977461b69ce9501df6a467f4a93b2 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Thu, 14 Jan 2021 21:57:28 -0800 Subject: [PATCH] remote: set executable bit of an input file based on its real value 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. Fixes #12818. Closes #12820. PiperOrigin-RevId: 351940694 --- .../build/lib/actions/FileArtifactValue.java | 2 +- .../build/lib/remote/RemoteCache.java | 14 ++++---- .../devtools/build/lib/remote/common/BUILD | 1 + .../common/RemoteActionFileArtifactValue.java | 35 +++++++++++++++++++ .../build/lib/remote/merkletree/BUILD | 1 + .../lib/remote/merkletree/DirectoryTree.java | 13 +++++-- .../merkletree/DirectoryTreeBuilder.java | 16 ++++++--- .../lib/remote/merkletree/MerkleTree.java | 2 +- .../ActionInputDirectoryTreeTest.java | 14 +++++--- .../remote/merkletree/DirectoryTreeTest.java | 7 ++-- .../lib/remote/merkletree/MerkleTreeTest.java | 16 +++++---- .../bazel/remote/remote_execution_test.sh | 22 ++++++++++++ 12 files changed, 115 insertions(+), 28 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionFileArtifactValue.java 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 8cef2a631a178f..6c59363fe0ad40 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 @@ -551,7 +551,7 @@ public boolean dataIsShareable() { } /** Metadata for remotely stored files. */ - public static final class RemoteFileArtifactValue extends FileArtifactValue { + public static class RemoteFileArtifactValue extends FileArtifactValue { private final byte[] digest; private final long size; private final int locationIndex; 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 5fc3b3a0aea8cf..ec6e6961b6832f 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 @@ -46,7 +46,6 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; -import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.concurrent.ThreadSafety; @@ -56,6 +55,7 @@ import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.DirectoryMetadata; import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.FileMetadata; import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.SymlinkMetadata; +import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; import com.google.devtools.build.lib.remote.options.RemoteOptions; @@ -647,12 +647,13 @@ private void injectRemoteArtifact( for (FileMetadata file : directory.files()) { TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, file.path().relativeTo(parent.getPath())); - RemoteFileArtifactValue value = - new RemoteFileArtifactValue( + RemoteActionFileArtifactValue value = + new RemoteActionFileArtifactValue( DigestUtil.toBinaryDigest(file.digest()), file.digest().getSizeBytes(), /*locationIndex=*/ 1, - actionId); + actionId, + file.isExecutable()); tree.putChild(child, value); } metadataInjector.injectTree(parent, tree.build()); @@ -665,11 +666,12 @@ private void injectRemoteArtifact( } metadataInjector.injectFile( output, - new RemoteFileArtifactValue( + new RemoteActionFileArtifactValue( DigestUtil.toBinaryDigest(outputMetadata.digest()), outputMetadata.digest().getSizeBytes(), /*locationIndex=*/ 1, - actionId)); + actionId, + outputMetadata.isExecutable())); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/BUILD b/src/main/java/com/google/devtools/build/lib/remote/common/BUILD index 167baad2c1d14a..024321d2695b23 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/common/BUILD @@ -14,6 +14,7 @@ java_library( name = "common", srcs = glob(["*.java"]), deps = [ + "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/vfs", "//third_party:guava", "//third_party/protobuf:protobuf_java", diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionFileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionFileArtifactValue.java new file mode 100644 index 00000000000000..a4d9f8042ab7a2 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionFileArtifactValue.java @@ -0,0 +1,35 @@ +// Copyright 2020 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.remote.common; + +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; + +/** + * A {@link RemoteFileArtifactValue} with additional data only available when using Remote Execution + * API (e.g. {@code isExecutable}). + */ +public class RemoteActionFileArtifactValue extends RemoteFileArtifactValue { + + private final boolean isExecutable; + + public RemoteActionFileArtifactValue( + byte[] digest, long size, int locationIndex, String actionId, boolean isExecutable) { + super(digest, size, locationIndex, actionId); + this.isExecutable = isExecutable; + } + + public boolean isExecutable() { + return isExecutable; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD b/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD index f63fa0e7e6c2ab..2eec7bf4928ebb 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD @@ -18,6 +18,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/profiler", + "//src/main/java/com/google/devtools/build/lib/remote/common", "//src/main/java/com/google/devtools/build/lib/remote/util", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", 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 43e4515a56f819..510920ddaca2ce 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,12 +72,14 @@ 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) { @@ -86,6 +88,7 @@ static class FileNode extends Node { this.data = Preconditions.checkNotNull(data, "data"); ; this.digest = Preconditions.checkNotNull(digest, "digest"); + this.isExecutable = false; } Digest getDigest() { @@ -100,6 +103,10 @@ ByteString getBytes() { return data; } + public boolean isExecutable() { + return isExecutable; + } + @Override public int hashCode() { return Objects.hash(super.hashCode(), path, data, digest); @@ -166,8 +173,8 @@ boolean isEmpty() { } /** - * Traverses the {@link ActionInputsTree} in a depth first search manner. The children are visited - * in lexographical order. + * Traverses the {@link DirectoryTree} in a depth first search manner. The children are visited in + * lexographical order. */ void visit(Visitor visitor) { Preconditions.checkNotNull(visitor, "visitor"); 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..80a34af8ab15db 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 @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.MetadataProvider; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; +import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue; import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.DirectoryNode; import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.FileNode; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -101,7 +102,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 +140,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 RemoteActionFileArtifactValue) { + isExecutable = ((RemoteActionFileArtifactValue) 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/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 e28c616f8c5a87..c511973859a570 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 @@ -70,7 +70,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(); @@ -117,13 +117,19 @@ 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..54c6a5c402c46b 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,10 @@ 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..fbb1577c44af6e 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,11 @@ 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/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 6ed2b6231d3b0f..3a828423b7e458 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -2159,6 +2159,28 @@ EOF @local_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" +} + function test_exclusive_tag() { # Test that the exclusive tag works with the remote cache. mkdir -p a