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 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
coeuvre · Jan 13, 2021 · ecb9ef3 · ecb9ef3
1 parent 28fa193
commit ecb9ef3
Show file tree

Hide file tree

Showing 9 changed files with 73 additions and 10 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
@@ -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
@@ -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,19 @@
+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
@@ -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
@@ -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,19 +73,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 +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
@@ -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/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"
+
+}