From 46abffb415c32001cb30e3cfa7d0f4aaf68d585d Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 13 Jan 2021 14:58:41 +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 | 2 +- .../build/lib/remote/RemoteCache.java | 6 ++-- .../devtools/build/lib/remote/common/BUILD | 1 + .../common/RemoteActionFileArtifactValue.java | 32 +++++++++++++++++++ .../build/lib/remote/merkletree/BUILD | 1 + .../lib/remote/merkletree/DirectoryTree.java | 12 +++++-- .../merkletree/DirectoryTreeBuilder.java | 16 +++++++--- .../lib/remote/merkletree/MerkleTree.java | 2 +- .../remote/merkletree/DirectoryTreeTest.java | 9 ++++-- .../bazel/remote/remote_execution_test.sh | 24 ++++++++++++++ 10 files changed, 92 insertions(+), 13 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..98e93d1817a948 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 @@ -56,6 +56,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; @@ -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..270dc749c83c79 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionFileArtifactValue.java @@ -0,0 +1,32 @@ +// 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 more information added */ +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 40572bd35f32bb..404aac76ea6878 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 @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.protobuf.ByteString; + import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -72,12 +73,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) { @@ -85,6 +88,7 @@ static class FileNode extends Node { this.path = null; this.data = Preconditions.checkNotNull(data, "data"); this.digest = Preconditions.checkNotNull(digest, "digest"); + this.isExecutable = false; } Digest getDigest() { @@ -99,6 +103,10 @@ ByteString getBytes() { return data; } + public boolean isExecutable() { + return isExecutable; + } + @Override public int hashCode() { return Objects.hash(super.hashCode(), path, data, digest); @@ -165,7 +173,7 @@ boolean isEmpty() { } /** - * Traverses the {@link ActionInputsTree} in a depth first search manner. The children are visited + * Traverses the {@link DirectoryTree} in a depth first search manner. The children are visited * in lexographical order. */ void visit(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/DirectoryTreeTest.java b/src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java index 33b1c36ad11fd5..3a187e7b7fa5a3 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,12 @@ 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"), /* isExecutable */ false); + FileNode expectedBarNode = + new FileNode("bar.cc", bar, digestUtil.computeAsUtf8("bar"), /* isExecutable */ false); + FileNode expectedBuzzNode = + new FileNode("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"), /* isExecutable */ false); assertThat(fileNodesAtDepth(tree, 0)).isEmpty(); assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode, expectedBarNode); assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBuzzNode); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index b05cafe240356c..ddb3e25b125d8a 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.exe"], + outs = ["out.txt"], + cmd = "ls -l $(SRCS); touch $@", +) +EOF + touch foo.txt bar.exe + chmod a+x bar.exe + + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + //:test >& $TEST_log || fail "Failed to build //:test" + + expect_log "-rwxr--r-- .* bar.exe" + expect_log "-rw-r--r-- .* foo.txt" +} + function test_exclusive_tag() { # Test that the exclusive tag works with the remote cache. mkdir -p a @@ -2374,3 +2396,5 @@ end_of_record" } run_suite "Remote execution and remote cache tests" + +} \ No newline at end of file