Add flag --incompatible_symlinked_sandbox_expands_tree_artifacts_in_runfiles_tree.

In the future, Bazel will expand tree artifacts in runfiles, too, which causes the sandbox to link each file individually into the sandbox directory, instead of symlinking
the entire directory.

In bazelbuild#5971 it was shown that this might have user-visible impact, so the flag provides a mechanism to try it out before Bazel turns it on by default and also to temporarily revert to the previous behavior during the migration window.

The flag will be removed once migration has been completed.

RELNOTES: In the future, Bazel will expand tree artifacts in runfiles, too, which causes the sandbox to link each file individually into the sandbox directory, instead of symlinking the entire directory. In this release, the behavior is not enabled by default yet. Please try it out via --incompatible_symlinked_sandbox_expands_tree_artifacts_in_runfiles_tree and let us know if it causes issues. If everything looks good, this behavior will become the default in a following release.
PiperOrigin-RevId: 211354053

Author: philwo

src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java

@@ -124,7 +124,7 @@ public List<SpawnResult> exec(
         spawnLogContext.logSpawn(
             spawn,
             actionExecutionContext.getMetadataProvider(),
-            context.getInputMapping(),
+            context.getInputMapping(true),
             context.getTimeout(),
             spawnResult);
       } catch (IOException e) {
@@ -184,10 +184,9 @@ public int getId() {
     @Override
     public void prefetchInputs() throws IOException {
       if (Spawns.shouldPrefetchInputsForLocalExecution(spawn)) {
-        // TODO(philwo): Benchmark whether using an ExecutionService to do multiple operations in
-        // parallel speeds up prefetching of inputs.
-        // TODO(philwo): Do we have to expand middleman artifacts here?
-        actionExecutionContext.getActionInputPrefetcher().prefetchFiles(getInputMapping().values());
+        actionExecutionContext
+            .getActionInputPrefetcher()
+            .prefetchFiles(getInputMapping(true).values());
       }
     }
 
@@ -232,13 +231,15 @@ public FileOutErr getFileOutErr() {
     }
 
     @Override
-    public SortedMap<PathFragment, ActionInput> getInputMapping() throws IOException {
+    public SortedMap<PathFragment, ActionInput> getInputMapping(
+        boolean expandTreeArtifactsInRunfiles) throws IOException {
       if (lazyInputMapping == null) {
         lazyInputMapping =
             spawnInputExpander.getInputMapping(
                 spawn,
                 actionExecutionContext.getArtifactExpander(),
-                actionExecutionContext.getMetadataProvider());
+                actionExecutionContext.getMetadataProvider(),
+                expandTreeArtifactsInRunfiles);
       }
       return lazyInputMapping;
     }

src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java

@@ -106,7 +106,8 @@ void addRunfilesToInputs(
       Map<PathFragment, ActionInput> inputMap,
       RunfilesSupplier runfilesSupplier,
       MetadataProvider actionFileCache,
-      ArtifactExpander artifactExpander)
+      ArtifactExpander artifactExpander,
+      boolean expandTreeArtifactsInRunfiles)
       throws IOException {
     Map<PathFragment, Map<PathFragment, Artifact>> rootsAndMappings =
         runfilesSupplier.getMappings();
@@ -120,7 +121,7 @@ void addRunfilesToInputs(
         Artifact localArtifact = mapping.getValue();
         if (localArtifact != null) {
           Preconditions.checkState(!localArtifact.isMiddlemanArtifact());
-          if (localArtifact.isTreeArtifact()) {
+          if (expandTreeArtifactsInRunfiles && localArtifact.isTreeArtifact()) {
             List<ActionInput> expandedInputs =
                 ActionInputHelper.expandArtifacts(
                     Collections.singletonList(localArtifact), artifactExpander);
@@ -200,8 +201,8 @@ private void addInputs(
 
   /**
    * Convert the inputs and runfiles of the given spawn to a map from exec-root relative paths to
-   * {@link ActionInput}s. The returned map does not contain tree artifacts as they are expanded
-   * to file artifacts.
+   * {@link ActionInput}s. The returned map does not contain tree artifacts as they are expanded to
+   * file artifacts.
    *
    * <p>The returned map never contains {@code null} values; it uses {@link #EMPTY_FILE} for empty
    * files, which is an instance of {@link
@@ -210,12 +211,19 @@ private void addInputs(
    * <p>The returned map contains all runfiles, but not the {@code MANIFEST}.
    */
   public SortedMap<PathFragment, ActionInput> getInputMapping(
-      Spawn spawn, ArtifactExpander artifactExpander, MetadataProvider actionInputFileCache)
+      Spawn spawn,
+      ArtifactExpander artifactExpander,
+      MetadataProvider actionInputFileCache,
+      boolean expandTreeArtifactsInRunfiles)
       throws IOException {
     TreeMap<PathFragment, ActionInput> inputMap = new TreeMap<>();
     addInputs(inputMap, spawn, artifactExpander);
     addRunfilesToInputs(
-        inputMap, spawn.getRunfilesSupplier(), actionInputFileCache, artifactExpander);
+        inputMap,
+        spawn.getRunfilesSupplier(),
+        actionInputFileCache,
+        artifactExpander,
+        expandTreeArtifactsInRunfiles);
     addFilesetManifests(spawn.getFilesetMappings(), inputMap);
     return inputMap;
   }

src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java

@@ -193,7 +193,8 @@ default ArtifactPathResolver getPathResolver() {
     /** The files to which to write stdout and stderr. */
     FileOutErr getFileOutErr();
 
-    SortedMap<PathFragment, ActionInput> getInputMapping() throws IOException;
+    SortedMap<PathFragment, ActionInput> getInputMapping(boolean expandTreeArtifactsInRunfiles)
+        throws IOException;
 
     /** Reports a progress update to the Spawn strategy. */
     void report(ProgressStatus state, String name);

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

@@ -97,7 +97,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
     // Temporary hack: the TreeNodeRepository should be created and maintained upstream!
     TreeNodeRepository repository =
         new TreeNodeRepository(execRoot, context.getMetadataProvider(), digestUtil);
-    SortedMap<PathFragment, ActionInput> inputMap = context.getInputMapping();
+    SortedMap<PathFragment, ActionInput> inputMap = context.getInputMapping(true);
     TreeNode inputRoot = repository.buildFromActionInputs(inputMap);
     repository.computeMerkleDigests(inputRoot);
     Command command =

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

@@ -143,7 +143,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
     // Temporary hack: the TreeNodeRepository should be created and maintained upstream!
     MetadataProvider inputFileCache = context.getMetadataProvider();
     TreeNodeRepository repository = new TreeNodeRepository(execRoot, inputFileCache, digestUtil);
-    SortedMap<PathFragment, ActionInput> inputMap = context.getInputMapping();
+    SortedMap<PathFragment, ActionInput> inputMap = context.getInputMapping(true);
     TreeNode inputRoot = repository.buildFromActionInputs(inputMap);
     repository.computeMerkleDigests(inputRoot);
     maybeWriteParamFilesLocally(spawn);

src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java

@@ -259,7 +259,12 @@ protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionContext context)
         allowNetwork
             || Spawns.requiresNetwork(spawn, getSandboxOptions().defaultSandboxAllowNetwork);
 
-    Map<PathFragment, Path> inputs = SandboxHelpers.processInputFiles(spawn, context, execRoot);
+    Map<PathFragment, Path> inputs =
+        SandboxHelpers.processInputFiles(
+            spawn,
+            context,
+            execRoot,
+            getSandboxOptions().symlinkedSandboxExpandsTreeArtifactsInRunfilesTree);
 
     SandboxedSpawn sandbox;
     if (sandboxfsProcess != null) {

src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java

@@ -263,7 +263,11 @@ protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionContext context)
             sandboxExecRoot,
             cmdLine.build(),
             cmdEnv.getClientEnv(),
-            SandboxHelpers.processInputFiles(spawn, context, execRoot),
+            SandboxHelpers.processInputFiles(
+                spawn,
+                context,
+                execRoot,
+                getSandboxOptions().symlinkedSandboxExpandsTreeArtifactsInRunfilesTree),
             outputs,
             ImmutableSet.of());
 

src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java

@@ -190,7 +190,11 @@ spawn, getSandboxOptions().defaultSandboxAllowNetwork)))
               sandboxPath,
               commandLineBuilder.build(),
               environment,
-              SandboxHelpers.processInputFiles(spawn, context, execRoot),
+              SandboxHelpers.processInputFiles(
+                  spawn,
+                  context,
+                  execRoot,
+                  getSandboxOptions().symlinkedSandboxExpandsTreeArtifactsInRunfilesTree),
               outputs,
               ImmutableSet.of());
     } else {
@@ -200,7 +204,11 @@ spawn, getSandboxOptions().defaultSandboxAllowNetwork)))
               sandboxExecRoot,
               commandLineBuilder.build(),
               environment,
-              SandboxHelpers.processInputFiles(spawn, context, execRoot),
+              SandboxHelpers.processInputFiles(
+                  spawn,
+                  context,
+                  execRoot,
+                  getSandboxOptions().symlinkedSandboxExpandsTreeArtifactsInRunfilesTree),
               outputs,
               writableDirs);
     }

src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java

@@ -101,7 +101,11 @@ protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionContext context)
             sandboxExecRoot,
             commandLineBuilder.build(),
             environment,
-            SandboxHelpers.processInputFiles(spawn, context, execRoot),
+            SandboxHelpers.processInputFiles(
+                spawn,
+                context,
+                execRoot,
+                getSandboxOptions().symlinkedSandboxExpandsTreeArtifactsInRunfilesTree),
             SandboxHelpers.getOutputFiles(spawn),
             getWritableDirs(sandboxExecRoot, environment));
 

src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

@@ -48,9 +48,16 @@ public final class SandboxHelpers {
    * @throws IOException If any files could not be written.
    */
   public static Map<PathFragment, Path> processInputFiles(
-      Spawn spawn, SpawnExecutionContext context, Path execRoot) throws IOException {
+      Spawn spawn,
+      SpawnExecutionContext context,
+      Path execRoot,
+      boolean expandTreeArtifactsInRunfiles)
+      throws IOException {
     return processInputFiles(
-        context.getInputMapping(), spawn, context.getArtifactExpander(), execRoot);
+        context.getInputMapping(expandTreeArtifactsInRunfiles),
+        spawn,
+        context.getArtifactExpander(),
+        execRoot);
   }
 
   /**

src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java

@@ -24,6 +24,7 @@
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
+import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.io.IOException;
@@ -281,4 +282,18 @@ public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) {
       effectTags = {OptionEffectTag.UNKNOWN},
       help = "Allow network access by default for actions.")
   public boolean defaultSandboxAllowNetwork;
+
+  @Option(
+      name = "incompatible_symlinked_sandbox_expands_tree_artifacts_in_runfiles_tree",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+      effectTags = {OptionEffectTag.EXECUTION},
+      metadataTags = {
+        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES,
+        OptionMetadataTag.INCOMPATIBLE_CHANGE
+      },
+      help =
+          "If enabled, the sandbox will expand tree artifacts in runfiles, thus the files that "
+              + "are contained in the tree artifact will be symlinked as individual files.")
+  public boolean symlinkedSandboxExpandsTreeArtifactsInRunfilesTree;
 }

src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java

@@ -37,6 +37,7 @@
 import com.google.devtools.build.lib.runtime.Command;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.runtime.commands.CleanCommand.CleanStartingEvent;
+import com.google.devtools.build.lib.sandbox.SandboxOptions;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.common.options.OptionsBase;
@@ -145,7 +146,10 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
             workerPool,
             extraFlags,
             env.getReporter(),
-            createFallbackRunner(env));
+            createFallbackRunner(env),
+            env.getOptions()
+                .getOptions(SandboxOptions.class)
+                .symlinkedSandboxExpandsTreeArtifactsInRunfilesTree);
     builder.addActionContext(new WorkerSpawnStrategy(env.getExecRoot(), spawnRunner));
 
     builder.addStrategyByContext(SpawnActionContext.class, "standalone");

src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java

@@ -76,18 +76,21 @@ final class WorkerSpawnRunner implements SpawnRunner {
   private final Multimap<String, String> extraFlags;
   private final EventHandler reporter;
   private final SpawnRunner fallbackRunner;
+  private final boolean sandboxUsesExpandedTreeArtifactsInRunfiles;
 
   public WorkerSpawnRunner(
       Path execRoot,
       WorkerPool workers,
       Multimap<String, String> extraFlags,
       EventHandler reporter,
-      SpawnRunner fallbackRunner) {
+      SpawnRunner fallbackRunner,
+      boolean sandboxUsesExpandedTreeArtifactsInRunfiles) {
     this.execRoot = execRoot;
     this.workers = Preconditions.checkNotNull(workers);
     this.extraFlags = extraFlags;
     this.reporter = reporter;
     this.fallbackRunner = fallbackRunner;
+    this.sandboxUsesExpandedTreeArtifactsInRunfiles = sandboxUsesExpandedTreeArtifactsInRunfiles;
   }
 
   @Override
@@ -135,7 +138,9 @@ private SpawnResult actuallyExec(Spawn spawn, SpawnExecutionContext context)
 
     HashCode workerFilesCombinedHash = WorkerFilesHash.getCombinedHash(workerFiles);
 
-    Map<PathFragment, Path> inputFiles = SandboxHelpers.processInputFiles(spawn, context, execRoot);
+    Map<PathFragment, Path> inputFiles =
+        SandboxHelpers.processInputFiles(
+            spawn, context, execRoot, sandboxUsesExpandedTreeArtifactsInRunfiles);
     Set<PathFragment> outputFiles = SandboxHelpers.getOutputFiles(spawn);
 
     WorkerKey key =

src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java

@@ -88,7 +88,7 @@ private void scratchFile(String file, String... lines) throws Exception {
   public void testEmptyRunfiles() throws Exception {
     RunfilesSupplier supplier = EmptyRunfilesSupplier.INSTANCE;
     FakeActionInputFileCache mockCache = new FakeActionInputFileCache();
-    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER);
+    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, true);
     assertThat(inputMappings).isEmpty();
   }
 
@@ -103,7 +103,7 @@ public void testRunfilesSingleFile() throws Exception {
     FakeActionInputFileCache mockCache = new FakeActionInputFileCache();
     mockCache.put(artifact, FileArtifactValue.createNormalFile(FAKE_DIGEST, 0));
 
-    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER);
+    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, true);
     assertThat(inputMappings).hasSize(1);
     assertThat(inputMappings)
         .containsEntry(PathFragment.create("runfiles/workspace/dir/file"), artifact);
@@ -121,7 +121,7 @@ public void testRunfilesDirectoryStrict() {
     mockCache.put(artifact, FileArtifactValue.createDirectory(-1));
 
     try {
-      expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER);
+      expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, true);
       fail();
     } catch (IOException expected) {
       assertThat(expected).hasMessageThat().isEqualTo("Not a file: dir/file");
@@ -140,7 +140,7 @@ public void testRunfilesDirectoryNonStrict() throws Exception {
     mockCache.put(artifact, FileArtifactValue.createDirectory(-1));
 
     expander = new SpawnInputExpander(execRoot, /*strict=*/ false);
-    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER);
+    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, true);
     assertThat(inputMappings).hasSize(1);
     assertThat(inputMappings)
         .containsEntry(PathFragment.create("runfiles/workspace/dir/file"), artifact);
@@ -163,7 +163,7 @@ public void testRunfilesTwoFiles() throws Exception {
     mockCache.put(artifact1, FileArtifactValue.createNormalFile(FAKE_DIGEST, 1));
     mockCache.put(artifact2, FileArtifactValue.createNormalFile(FAKE_DIGEST, 2));
 
-    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER);
+    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, true);
     assertThat(inputMappings).hasSize(2);
     assertThat(inputMappings)
         .containsEntry(PathFragment.create("runfiles/workspace/dir/file"), artifact1);
@@ -185,7 +185,7 @@ public void testRunfilesSymlink() throws Exception {
     FakeActionInputFileCache mockCache = new FakeActionInputFileCache();
     mockCache.put(artifact, FileArtifactValue.createNormalFile(FAKE_DIGEST, 1));
 
-    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER);
+    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, true);
     assertThat(inputMappings).hasSize(1);
     assertThat(inputMappings)
         .containsEntry(PathFragment.create("runfiles/workspace/symlink"), artifact);
@@ -205,7 +205,7 @@ public void testRunfilesRootSymlink() throws Exception {
     FakeActionInputFileCache mockCache = new FakeActionInputFileCache();
     mockCache.put(artifact, FileArtifactValue.createNormalFile(FAKE_DIGEST, 1));
 
-    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER);
+    expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, true);
     assertThat(inputMappings).hasSize(2);
     assertThat(inputMappings).containsEntry(PathFragment.create("runfiles/symlink"), artifact);
     // If there's no other entry, Runfiles adds an empty file in the workspace to make sure the
@@ -236,7 +236,7 @@ public void testRunfilesWithTreeArtifacts() throws Exception {
     fakeCache.put(file1, FileArtifactValue.create(file1));
     fakeCache.put(file2, FileArtifactValue.create(file2));
 
-    expander.addRunfilesToInputs(inputMappings, supplier, fakeCache, artifactExpander);
+    expander.addRunfilesToInputs(inputMappings, supplier, fakeCache, artifactExpander, true);
     assertThat(inputMappings).hasSize(2);
     assertThat(inputMappings)
         .containsEntry(PathFragment.create("runfiles/workspace/treeArtifact/file1"), file1);
@@ -268,7 +268,7 @@ public void testRunfilesWithTreeArtifactsInSymlinks() throws Exception {
     fakeCache.put(file1, FileArtifactValue.create(file1));
     fakeCache.put(file2, FileArtifactValue.create(file2));
 
-    expander.addRunfilesToInputs(inputMappings, supplier, fakeCache, artifactExpander);
+    expander.addRunfilesToInputs(inputMappings, supplier, fakeCache, artifactExpander, true);
     assertThat(inputMappings).hasSize(2);
     assertThat(inputMappings)
         .containsEntry(PathFragment.create("runfiles/workspace/symlink/file1"), file1);
@@ -296,7 +296,7 @@ public void testTreeArtifactsInInputs() throws Exception {
     fakeCache.put(file2, FileArtifactValue.create(file2));
 
     Spawn spawn = new SpawnBuilder("/bin/echo", "Hello World").withInput(treeArtifact).build();
-    inputMappings = expander.getInputMapping(spawn, artifactExpander, fakeCache);
+    inputMappings = expander.getInputMapping(spawn, artifactExpander, fakeCache, true);
     assertThat(inputMappings).hasSize(2);
     assertThat(inputMappings).containsEntry(PathFragment.create("treeArtifact/file1"), file1);
     assertThat(inputMappings).containsEntry(PathFragment.create("treeArtifact/file2"), file2);

src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java

@@ -242,7 +242,8 @@ public FileOutErr getFileOutErr() {
     }
 
     @Override
-    public SortedMap<PathFragment, ActionInput> getInputMapping() {
+    public SortedMap<PathFragment, ActionInput> getInputMapping(
+        boolean expandTreeArtifactsInRunfiles) {
       return inputMapping;
     }
 

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

@@ -181,9 +181,10 @@ public FileOutErr getFileOutErr() {
         }
 
         @Override
-        public SortedMap<PathFragment, ActionInput> getInputMapping() throws IOException {
+        public SortedMap<PathFragment, ActionInput> getInputMapping(
+            boolean expandTreeArtifactsInRunfiles) throws IOException {
           return new SpawnInputExpander(execRoot, /*strict*/ false)
-              .getInputMapping(simpleSpawn, SIMPLE_ARTIFACT_EXPANDER, fakeFileCache);
+              .getInputMapping(simpleSpawn, SIMPLE_ARTIFACT_EXPANDER, fakeFileCache, true);
         }
 
         @Override