[6.1.0] Flag for writable outputs (experimental) (bazelbuild#17617)

This feature is tied to an experimental flag `--experimental_writable_outputs`. When enabled, Bazel will set the permissions of all output files to 0755 instead of 0555.

RELNOTES: None.
PiperOrigin-RevId: 500786227
Change-Id: I59e15f3fec09c40a052a60b00da209547f10d7fc

Co-authored-by: Googler <[email protected]>
Co-authored-by: kshyanashree <[email protected]>
Co-authored-by: keertk <[email protected]>

Author: 4 people

src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java

@@ -41,6 +41,7 @@
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation;
+import com.google.devtools.build.lib.vfs.OutputPermissions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.FileNotFoundException;
@@ -291,9 +292,10 @@ private static Map<String, String> computeUsedEnv(
     Map<String, String> usedClientEnv = computeUsedClientEnv(action, clientEnv);
     Map<String, String> usedExecProperties =
         computeUsedExecProperties(action, remoteDefaultPlatformProperties);
-    // Combining the Client environment with the Remote Default Execution Properties, because
-    // the Miss Reason is not used currently by Bazel, therefore there is no need to distinguish
-    // between these two cases. This also saves memory used for the Action Cache.
+    // Combining the Client environment with the Remote Default Execution Properties and Output
+    // Permissions, because the Miss Reason is not used currently by Bazel, therefore there is no
+    // need to distinguish between these property types. This also saves memory used for the Action
+    // Cache.
     Map<String, String> usedEnvironment = new HashMap<>();
     usedEnvironment.putAll(usedClientEnv);
     usedEnvironment.putAll(usedExecProperties);
@@ -419,6 +421,7 @@ public Token getTokenIfNeedToExecute(
       Action action,
       List<Artifact> resolvedCacheArtifacts,
       Map<String, String> clientEnv,
+      OutputPermissions outputPermissions,
       EventHandler handler,
       MetadataHandler metadataHandler,
       ArtifactExpander artifactExpander,
@@ -481,6 +484,7 @@ public Token getTokenIfNeedToExecute(
         artifactExpander,
         actionInputs,
         clientEnv,
+        outputPermissions,
         remoteDefaultPlatformProperties,
         cachedOutputMetadata)) {
       if (entry != null) {
@@ -510,6 +514,7 @@ private boolean mustExecute(
       ArtifactExpander artifactExpander,
       NestedSet<Artifact> actionInputs,
       Map<String, String> clientEnv,
+      OutputPermissions outputPermissions,
       Map<String, String> remoteDefaultPlatformProperties,
       @Nullable CachedOutputMetadata cachedOutputMetadata)
       throws InterruptedException {
@@ -542,7 +547,7 @@ private boolean mustExecute(
     }
     Map<String, String> usedEnvironment =
         computeUsedEnv(action, clientEnv, remoteDefaultPlatformProperties);
-    if (!entry.usedSameClientEnv(usedEnvironment)) {
+    if (!entry.sameActionProperties(usedEnvironment, outputPermissions)) {
       reportClientEnv(handler, action, usedEnvironment);
       actionCache.accountMiss(MissReason.DIFFERENT_ENVIRONMENT);
       return true;
@@ -580,6 +585,7 @@ public void updateActionCache(
       MetadataHandler metadataHandler,
       ArtifactExpander artifactExpander,
       Map<String, String> clientEnv,
+      OutputPermissions outputPermissions,
       Map<String, String> remoteDefaultPlatformProperties)
       throws IOException, InterruptedException {
     checkState(cacheConfig.enabled(), "cache unexpectedly disabled, action: %s", action);
@@ -595,7 +601,8 @@ public void updateActionCache(
         new ActionCache.Entry(
             action.getKey(actionKeyContext, artifactExpander),
             usedEnvironment,
-            action.discoversInputs());
+            action.discoversInputs(),
+            outputPermissions);
     for (Artifact output : action.getOutputs()) {
       // Remove old records from the cache if they used different key.
       String execPath = output.getExecPathString();
@@ -746,7 +753,7 @@ private void checkMiddlemanAction(
       // Compute the aggregated middleman digest.
       // Since we never validate action key for middlemen, we should not store
       // it in the cache entry and just use empty string instead.
-      entry = new ActionCache.Entry("", ImmutableMap.of(), false);
+      entry = new ActionCache.Entry("", ImmutableMap.of(), false, OutputPermissions.READONLY);
       for (Artifact input : action.getInputs().toList()) {
         entry.addInputFile(
             input.getExecPath(), getMetadataMaybe(metadataHandler, input), /*saveExecPath=*/ true);
@@ -768,6 +775,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
       Action action,
       List<Artifact> resolvedCacheArtifacts,
       Map<String, String> clientEnv,
+      OutputPermissions outputPermissions,
       EventHandler handler,
       MetadataHandler metadataHandler,
       ArtifactExpander artifactExpander,
@@ -781,6 +789,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
         action,
         resolvedCacheArtifacts,
         clientEnv,
+        outputPermissions,
         handler,
         metadataHandler,
         artifactExpander,

src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java

@@ -33,6 +33,7 @@
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.DigestUtils;
+import com.google.devtools.build.lib.vfs.OutputPermissions;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
 import java.io.PrintStream;
@@ -84,9 +85,11 @@ public interface ActionCache {
    * will continue to return same result regardless of internal data transformations).
    */
   final class Entry {
+    private static final byte[] EMPTY_CLIENT_ENV_DIGEST = new byte[0];
+
     /** Unique instance to represent a corrupted cache entry. */
     public static final ActionCache.Entry CORRUPTED =
-        new ActionCache.Entry(null, ImmutableMap.of(), false);
+        new ActionCache.Entry(null, ImmutableMap.of(), false, OutputPermissions.READONLY);
 
     private final String actionKey;
     @Nullable
@@ -95,7 +98,7 @@ final class Entry {
     // If null, digest is non-null and the entry is immutable.
     private Map<String, FileArtifactValue> mdMap;
     private byte[] digest;
-    private final byte[] usedClientEnvDigest;
+    private final byte[] actionPropertiesDigest;
     private final Map<String, RemoteFileArtifactValue> outputFileMetadata;
     private final Map<String, SerializableTreeArtifactValue> outputTreeMetadata;
 
@@ -160,9 +163,13 @@ public static Optional<SerializableTreeArtifactValue> createSerializable(
       public abstract Optional<PathFragment> materializationExecPath();
     }
 
-    public Entry(String key, Map<String, String> usedClientEnv, boolean discoversInputs) {
+    public Entry(
+        String key,
+        Map<String, String> usedClientEnv,
+        boolean discoversInputs,
+        OutputPermissions outputPermissions) {
       actionKey = key;
-      this.usedClientEnvDigest = digestClientEnv(usedClientEnv);
+      this.actionPropertiesDigest = digestActionProperties(usedClientEnv, outputPermissions);
       files = discoversInputs ? new ArrayList<>() : null;
       mdMap = new HashMap<>();
       outputFileMetadata = new HashMap<>();
@@ -171,39 +178,46 @@ public Entry(String key, Map<String, String> usedClientEnv, boolean discoversInp
 
     public Entry(
         String key,
-        byte[] usedClientEnvDigest,
+        byte[] actionPropertiesDigest,
         @Nullable List<String> files,
         byte[] digest,
         Map<String, RemoteFileArtifactValue> outputFileMetadata,
         Map<String, SerializableTreeArtifactValue> outputTreeMetadata) {
       actionKey = key;
-      this.usedClientEnvDigest = usedClientEnvDigest;
+      this.actionPropertiesDigest = actionPropertiesDigest;
       this.files = files;
       this.digest = digest;
       mdMap = null;
       this.outputFileMetadata = outputFileMetadata;
       this.outputTreeMetadata = outputTreeMetadata;
     }
 
-    private static final byte[] EMPTY_CLIENT_ENV_DIGEST = new byte[0];
-
     /**
-     * Computes an order-independent digest of a map of environment variables.
+     * Computes an order-independent digest of action properties. This includes a map of client
+     * environment variables and the non-default permissions for output artifacts of the action.
      *
      * <p>Note that as discussed in https://github.com/bazelbuild/bazel/issues/15660, using {@link
      * DigestUtils#xor} to achieve order-independence is questionable in case it is possible that
      * multiple string keys map to the same bytes when passed through {@link Fingerprint#addString}
      * (due to lossy conversion from UTF-16 to UTF-8). We could instead use a sorted map, however
      * changing the digest function would cause action cache misses across bazel versions.
      */
-    private static byte[] digestClientEnv(Map<String, String> clientEnv) {
+    private static byte[] digestActionProperties(
+        Map<String, String> clientEnv, OutputPermissions outputPermissions) {
       byte[] result = EMPTY_CLIENT_ENV_DIGEST;
       Fingerprint fp = new Fingerprint();
       for (Map.Entry<String, String> entry : clientEnv.entrySet()) {
         fp.addString(entry.getKey());
         fp.addString(entry.getValue());
         result = DigestUtils.xor(result, fp.digestAndReset());
       }
+      // Add the permissions mode to the digest if it differs from the default.
+      // This is a bit of a hack to save memory on entries which have the default permissions mode
+      // and no client env.
+      if (outputPermissions != OutputPermissions.READONLY) {
+        fp.addInt(outputPermissions.getPermissionsMode());
+        result = DigestUtils.xor(result, fp.digestAndReset());
+      }
       return result;
     }
 
@@ -288,14 +302,16 @@ public String getActionKey() {
       return actionKey;
     }
 
-    /** @return the effectively used client environment */
-    public byte[] getUsedClientEnvDigest() {
-      return usedClientEnvDigest;
+    /** Returns the effectively used client environment. */
+    public byte[] getActionPropertiesDigest() {
+      return actionPropertiesDigest;
     }
 
-    /** Determines whether this entry used the same client environment as the one given. */
-    public boolean usedSameClientEnv(Map<String, String> clientEnv) {
-      return Arrays.equals(digestClientEnv(clientEnv), usedClientEnvDigest);
+    /** Determines whether this entry has the same action properties as the one given. */
+    public boolean sameActionProperties(
+        Map<String, String> clientEnv, OutputPermissions outputPermissions) {
+      return Arrays.equals(
+          digestActionProperties(clientEnv, outputPermissions), actionPropertiesDigest);
     }
 
     /**
@@ -343,7 +359,7 @@ public String toString() {
       builder.append("      actionKey = ").append(actionKey).append("\n");
       builder
           .append("      usedClientEnvKey = ")
-          .append(formatDigest(usedClientEnvDigest))
+          .append(formatDigest(actionPropertiesDigest))
           .append("\n");
       builder.append("      digestKey = ");
       if (digest == null) {

src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java

@@ -571,7 +571,7 @@ private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) thr
       VarInt.putVarInt(indexer.getOrCreateIndex(file), sink);
     }
 
-    MetadataDigestUtils.write(entry.getUsedClientEnvDigest(), sink);
+    MetadataDigestUtils.write(entry.getActionPropertiesDigest(), sink);
 
     VarInt.putVarInt(entry.getOutputFiles().size(), sink);
     for (Map.Entry<String, RemoteFileArtifactValue> file : entry.getOutputFiles().entrySet()) {

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

@@ -170,6 +170,17 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
               + "disabled.")
   public boolean strictFilesets;
 
+  // This option is only used during execution. However, it is a required input to the analysis
+  // phase, as otherwise flipping this flag would not invalidate already-executed actions.
+  @Option(
+      name = "experimental_writable_outputs",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+      effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
+      metadataTags = {OptionMetadataTag.EXPERIMENTAL},
+      help = "If true, the file permissions of action outputs are set to 0755 instead of 0555")
+  public boolean experimentalWritableOutputs;
+
   @Option(
       name = "incompatible_strict_conflict_checks",
       oldName = "experimental_strict_conflict_checks",
@@ -970,6 +981,7 @@ public FragmentOptions getHost() {
     host.includeRequiredConfigFragmentsProvider = includeRequiredConfigFragmentsProvider;
     host.debugSelectsAlwaysSucceed = debugSelectsAlwaysSucceed;
     host.checkTestonlyForOutputFiles = checkTestonlyForOutputFiles;
+    host.experimentalWritableOutputs = experimentalWritableOutputs;
     host.strictConflictChecks = strictConflictChecks;
 
     // === Runfiles ===

src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java

@@ -48,6 +48,7 @@
 import com.google.devtools.build.lib.remote.util.RxUtils.TransferResult;
 import com.google.devtools.build.lib.remote.util.TempPathGenerator;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.OutputPermissions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import io.reactivex.rxjava3.core.Completable;
@@ -74,6 +75,7 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet
   private final Reporter reporter;
   private final AsyncTaskCache.NoResult<Path> downloadCache = AsyncTaskCache.NoResult.create();
   private final TempPathGenerator tempPathGenerator;
+  private final OutputPermissions outputPermissions;
   protected final Set<Artifact> outputsAreInputs = Sets.newConcurrentHashSet();
 
   protected final Path execRoot;
@@ -123,11 +125,13 @@ protected AbstractActionInputPrefetcher(
       Reporter reporter,
       Path execRoot,
       TempPathGenerator tempPathGenerator,
-      ImmutableList<Pattern> patternsToDownload) {
+      ImmutableList<Pattern> patternsToDownload,
+      OutputPermissions outputPermissions) {
     this.reporter = reporter;
     this.execRoot = execRoot;
     this.tempPathGenerator = tempPathGenerator;
     this.patternsToDownload = patternsToDownload;
+    this.outputPermissions = outputPermissions;
   }
 
   private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) {
@@ -352,12 +356,12 @@ private Completable prefetchInputTree(
                   }
 
                   for (Path dir : dirs) {
-                    // Change permission of all directories of a tree artifact to 0555 (files are
+                    // Change permission of all directories of a tree artifact (files are
                     // changed inside {@code finalizeDownload}) in order to match the behaviour when
                     // the tree artifact is generated locally. In that case, permission of all files
-                    // and directories inside a tree artifact is changed to 0555 within {@code
+                    // and directories inside a tree artifact is changed within {@code
                     // checkOutputs()}.
-                    dir.chmod(0555);
+                    dir.chmod(outputPermissions.getPermissionsMode());
                   }
 
                   completed.set(true);
@@ -537,9 +541,9 @@ private void finalizeDownload(Context context, Path tmpPath, Path path) throws I
       parentDir.setWritable(true);
     }
 
-    // The permission of output file is changed to 0555 after action execution. We manually change
+    // The permission of output file is changed after action execution. We manually change
     // the permission here for the downloaded file to keep this behaviour consistent.
-    tmpPath.chmod(0555);
+    tmpPath.chmod(outputPermissions.getPermissionsMode());
     FileSystemUtils.moveFile(tmpPath, path);
   }
 

src/main/java/com/google/devtools/build/lib/remote/BUILD

@@ -61,6 +61,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform:platform_utils",
         "//src/main/java/com/google/devtools/build/lib/authandtls",

src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java

@@ -38,6 +38,7 @@
 import com.google.devtools.build.lib.server.FailureDetails;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code;
+import com.google.devtools.build.lib.vfs.OutputPermissions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import io.reactivex.rxjava3.core.Completable;
@@ -65,8 +66,9 @@ class RemoteActionInputFetcher extends AbstractActionInputPrefetcher {
       Path execRoot,
       TempPathGenerator tempPathGenerator,
       ImmutableList<Pattern> patternsToDownload,
+      OutputPermissions outputPermissions,
       boolean useNewExitCodeForLostInputs) {
-    super(reporter, execRoot, tempPathGenerator, patternsToDownload);
+    super(reporter, execRoot, tempPathGenerator, patternsToDownload, outputPermissions);
     this.buildRequestId = Preconditions.checkNotNull(buildRequestId);
     this.commandId = Preconditions.checkNotNull(commandId);
     this.remoteCache = Preconditions.checkNotNull(remoteCache);

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

@@ -41,6 +41,7 @@
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
 import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
 import com.google.devtools.build.lib.authandtls.CallCredentialsProvider;
@@ -94,6 +95,7 @@
 import com.google.devtools.build.lib.util.io.AsynchronousFileOutputStream;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.OutputPermissions;
 import com.google.devtools.build.lib.vfs.OutputService;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.common.options.OptionsBase;
@@ -908,6 +910,11 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
     RemoteOptions remoteOptions =
         Preconditions.checkNotNull(
             env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions");
+    CoreOptions coreOptions = env.getOptions().getOptions(CoreOptions.class);
+    OutputPermissions outputPermissions =
+        coreOptions.experimentalWritableOutputs
+            ? OutputPermissions.WRITABLE
+            : OutputPermissions.READONLY;
     RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
 
     if (!remoteOutputsMode.downloadAllOutputs() && actionContextProvider.getRemoteCache() != null) {
@@ -921,6 +928,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
               env.getExecRoot(),
               tempPathGenerator,
               patternsToDownload,
+              outputPermissions,
               remoteOptions.useNewExitCodeForLostInputs);
       env.getEventBus().register(actionInputFetcher);
       builder.setActionInputPrefetcher(actionInputFetcher);

src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java

@@ -742,6 +742,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
             state.inputArtifactData,
             action.discoversInputs(),
             skyframeActionExecutor.useArchivedTreeArtifacts(action),
+            skyframeActionExecutor.getOutputPermissions(),
             action.getOutputs(),
             skyframeActionExecutor.getXattrProvider(),
             tsgm.get(),