remote: set executable bit of an input file based on its real value

The "always mark" was introduced by 3e3b71a which was a workaround for #4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. #12818. Fixes #12818. Closes #12820. PiperOrigin-RevId: 351940694
bazelbuild · Jan 15, 2021 · b6e3ba8 · b6e3ba8
1 parent 95ebfd2
commit b6e3ba8
Show file tree

Hide file tree

Showing 12 changed files with 115 additions and 28 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -72,19 +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() {
@@ -99,6 +102,10 @@ ByteString getBytes() {
       return data;
     }
 
+    public boolean isExecutable() {
+      return isExecutable;
+    }
+
     @Override
     public int hashCode() {
       return Objects.hash(super.hashCode(), path, data, digest);
@@ -165,8 +172,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
@@ -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
@@ -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/...st/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java b/...st/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
@@ -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
@@ -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
@@ -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