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.

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();
     }
   }

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.

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;

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());
     }
   }
 

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;
     }

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:

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();
   }
 

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

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) {

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();
     }
 

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;
   }

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;
   }

src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java

@@ -80,8 +80,8 @@ public void testFetching() throws Exception {
     // arrange
     Map<ActionInput, FileArtifactValue> metadata = new HashMap<>();
     Map<Digest, ByteString> 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<ActionInput, FileArtifactValue> 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<ActionInput, FileArtifactValue> metadata = new HashMap<>();
     Map<Digest, ByteString> 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<ActionInput, FileArtifactValue> metadata,
-      Map<Digest, ByteString> cacheEntries) {
+      Map<Digest, ByteString> 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;

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.<PathFragment, RemoteFileArtifactValue>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();

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();
             }