Give up on executable bit

bazelbuild · Jan 29, 2025 · e1dc9ba · e1dc9ba
1 parent e50ecb2
commit e1dc9ba
Show file tree

Hide file tree

Showing 6 changed files with 37 additions and 40 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
@@ -888,11 +888,10 @@ public static final class FileWriteOutputArtifactValue extends FileArtifactValue
     private final DeterministicWriter writer;
     private final long size;
     private final byte[] digest;
-    private final boolean isExecutable;
     @Nullable private final PathFragment materializationExecPath;
 
     public static FileWriteOutputArtifactValue hashAndCreate(
-        DeterministicWriter writer, HashFunction hashFunction, boolean isExecutable) {
+        DeterministicWriter writer, HashFunction hashFunction) {
       long size;
       byte[] digest;
       try (CountingOutputStream countingOut =
@@ -906,20 +905,7 @@ public static FileWriteOutputArtifactValue hashAndCreate(
         throw new IllegalStateException(e);
       }
       return new FileWriteOutputArtifactValue(
-          writer, size, digest, isExecutable, /* materializationExecPath= */ null);
-    }
-
-    private FileWriteOutputArtifactValue(
-        DeterministicWriter writer,
-        long size,
-        byte[] digest,
-        boolean isExecutable,
-        @Nullable PathFragment materializationExecPath) {
-      this.writer = writer;
-      this.size = size;
-      this.digest = digest;
-      this.isExecutable = isExecutable;
-      this.materializationExecPath = materializationExecPath;
+          writer, size, digest, /* materializationExecPath= */ null);
     }
 
     /**
@@ -933,11 +919,18 @@ public static FileWriteOutputArtifactValue createFromExistingWithMaterialization
         return metadata;
       }
       return new FileWriteOutputArtifactValue(
-          metadata.writer,
-          metadata.size,
-          metadata.digest,
-          metadata.isExecutable,
-          materializationExecPath);
+          metadata.writer, metadata.size, metadata.digest, materializationExecPath);
+    }
+
+    private FileWriteOutputArtifactValue(
+        DeterministicWriter writer,
+        long size,
+        byte[] digest,
+        @Nullable PathFragment materializationExecPath) {
+      this.writer = writer;
+      this.size = size;
+      this.digest = digest;
+      this.materializationExecPath = materializationExecPath;
     }
 
     @Override
@@ -955,10 +948,6 @@ public long getSize() {
       return size;
     }
 
-    public boolean isExecutable() {
-      return isExecutable;
-    }
-
     @Override
     public void writeTo(OutputStream out) throws IOException {
       writer.writeOutputFile(out);

diff --git a/src/main/java/com/google/devtools/build/lib/exec/LazyFileWriteStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/LazyFileWriteStrategy.java
@@ -51,6 +51,9 @@ public ImmutableList<SpawnResult> writeOutputToFile(
     actionExecutionContext.getEventHandler().post(new RunningActionEvent(action, "local"));
     try (AutoProfiler p =
         GoogleAutoProfilerUtils.logged("hashing output of " + action.prettyPrint(), MIN_LOGGING)) {
+      // TODO: Bazel currently marks all output files as executable after local execution and stages
+      // all files as executable for remote execution, so we don't keep track of the executable
+      // bit yet.
       actionExecutionContext
           .getOutputMetadataStore()
           .injectFile(
@@ -60,8 +63,7 @@ public ImmutableList<SpawnResult> writeOutputToFile(
                   actionExecutionContext
                       .getActionFileSystem()
                       .getDigestFunction()
-                      .getHashFunction(),
-                  makeExecutable));
+                      .getHashFunction()));
     }
     return ImmutableList.of();
   }

diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
@@ -568,10 +568,6 @@ private Completable downloadFileNoCheckRx(
                   finalizeDownload(
                       fileWriteOutputArtifactValue, tempPath, finalPath, dirsWithOutputPermissions);
                   alreadyDeleted.set(true);
-                  // finalizeDownload always makes the file executable.
-                  if (!fileWriteOutputArtifactValue.isExecutable()) {
-                    finalPath.setExecutable(false);
-                  }
                   return Completable.complete();
                 } catch (IOException e) {
                   return Completable.error(e);

diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD
@@ -212,6 +212,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/analysis:analysis_cluster",
+        "//src/main/java/com/google/devtools/build/lib/exec:execution_options",
         "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
         "//src/main/java/com/google/devtools/build/lib/util:command",

diff --git a/...st/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/...st/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.analysis.TargetCompleteEvent;
 import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
+import com.google.devtools.build.lib.exec.ExecutionOptions.FileWriteStrategy;
 import com.google.devtools.build.lib.skyframe.ActionExecutionValue;
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.util.CommandBuilder;
@@ -1097,7 +1098,9 @@ public void downloadToplevel_unresolvedSymlink() throws Exception {
   }
 
   @Test
-  public void downloadMinimal_fileWrite(@TestParameter boolean isExecutable) throws Exception {
+  public void downloadMinimal_fileWrite(
+      @TestParameter boolean isExecutable, @TestParameter FileWriteStrategy fileWriteStrategy)
+      throws Exception {
     writeWriteFileRule();
     writeSymlinkRule();
     // Remote execution stages all files as executable.
@@ -1122,16 +1125,21 @@ public void downloadMinimal_fileWrite(@TestParameter boolean isExecutable) throw
         """
             .formatted(isExecutable ? "True" : "False"));
 
-    addOptions("--file_write_strategy=lazy");
+    addOptions("--file_write_strategy=" + fileWriteStrategy);
 
     buildTarget("//:gen");
-    buildTarget("//:foo");
-    assertOutputsDoNotExist("//:foo");
+    if (fileWriteStrategy == FileWriteStrategy.LAZY) {
+      assertOutputsDoNotExist("//:foo");
+    } else {
+      assertValidOutputFile("foo", "hello");
+    }
     assertOutputsDoNotExist("//:gen");
   }
 
   @Test
-  public void downloadToplevel_fileWrite(@TestParameter boolean isExecutable) throws Exception {
+  public void downloadToplevel_fileWrite(
+      @TestParameter boolean isExecutable, @TestParameter FileWriteStrategy fileWriteStrategy)
+      throws Exception {
     setDownloadToplevel();
     writeWriteFileRule();
     write(
@@ -1145,11 +1153,12 @@ public void downloadToplevel_fileWrite(@TestParameter boolean isExecutable) thro
         )
         """.formatted(isExecutable ? "True" : "False"));
 
-    addOptions("--file_write_strategy=lazy");
+    addOptions("--file_write_strategy=" + fileWriteStrategy);
 
     buildTarget("//:foo");
     assertOnlyOutputContent("//:foo", "foo", "hello");
-    assertThat(getOutputPath("foo").isExecutable()).isEqualTo(isExecutable);
+    // TODO: Bazel doesn't honor the executable bit.
+    assertThat(getOutputPath("foo").isExecutable()).isTrue();
 
     // Delete file, re-create it
     getOutputPath("foo-link").delete();

diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
@@ -2091,7 +2091,7 @@ def _symlink_rule_impl(ctx):
         outputs = [file],
         command = "echo 'Hello World!' > " + file.path,
     )
-    ctx.actions.write(executable, "[[ -L symlink.txt ]] && cat symlink.txt", is_executable = True)
+    ctx.actions.write(executable, "[[ -L symlink.txt ]] && cat symlink.txt")
     return [DefaultInfo(
         runfiles = ctx.runfiles(
             symlinks = {"symlink.txt": file},
@@ -2137,7 +2137,7 @@ def _symlink_rule_impl(ctx):
         outputs = [file],
         command = "echo 'Hello World!' > " + file.path,
     )
-    ctx.actions.write(executable, "[[ -L ../symlink.txt ]] && cat ../symlink.txt", is_executable = True)
+    ctx.actions.write(executable, "[[ -L ../symlink.txt ]] && cat ../symlink.txt")
     return [DefaultInfo(
         runfiles = ctx.runfiles(
             root_symlinks = {"symlink.txt": file},