From 9ed9d8ac4347408d15c8fce7c9c07e5c8e658b30 Mon Sep 17 00:00:00 2001 From: philwo Date: Mon, 3 Sep 2018 07:30:26 -0700 Subject: [PATCH] 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 #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 --- .../build/lib/exec/AbstractSpawnStrategy.java | 15 ++--- .../build/lib/exec/SpawnInputExpander.java | 20 +++++-- .../devtools/build/lib/exec/SpawnRunner.java | 3 +- .../build/lib/remote/RemoteSpawnCache.java | 2 +- .../build/lib/remote/RemoteSpawnRunner.java | 2 +- .../sandbox/DarwinSandboxedSpawnRunner.java | 7 ++- .../sandbox/DockerSandboxedSpawnRunner.java | 6 +- .../sandbox/LinuxSandboxedSpawnRunner.java | 12 +++- .../ProcessWrapperSandboxedSpawnRunner.java | 6 +- .../build/lib/sandbox/SandboxHelpers.java | 11 +++- .../build/lib/sandbox/SandboxOptions.java | 15 +++++ .../build/lib/worker/WorkerModule.java | 6 +- .../build/lib/worker/WorkerSpawnRunner.java | 9 ++- .../lib/exec/SpawnInputExpanderTest.java | 20 +++---- .../lib/exec/local/LocalSpawnRunnerTest.java | 3 +- .../remote/GrpcRemoteExecutionClientTest.java | 5 +- .../lib/remote/RemoteSpawnCacheTest.java | 5 +- .../lib/remote/RemoteSpawnRunnerTest.java | 5 +- src/test/shell/bazel/bazel_sandboxing_test.sh | 59 ++++++++++++++++++- 19 files changed, 167 insertions(+), 44 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java index 4be5e977c4fa17..3848b4291a0953 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java @@ -124,7 +124,7 @@ public List 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 getInputMapping() throws IOException { + public SortedMap getInputMapping( + boolean expandTreeArtifactsInRunfiles) throws IOException { if (lazyInputMapping == null) { lazyInputMapping = spawnInputExpander.getInputMapping( spawn, actionExecutionContext.getArtifactExpander(), - actionExecutionContext.getMetadataProvider()); + actionExecutionContext.getMetadataProvider(), + expandTreeArtifactsInRunfiles); } return lazyInputMapping; } diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java index 0e965b32dd3b77..a2a875cf90b801 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java @@ -106,7 +106,8 @@ void addRunfilesToInputs( Map inputMap, RunfilesSupplier runfilesSupplier, MetadataProvider actionFileCache, - ArtifactExpander artifactExpander) + ArtifactExpander artifactExpander, + boolean expandTreeArtifactsInRunfiles) throws IOException { Map> 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 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. * *

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( *

The returned map contains all runfiles, but not the {@code MANIFEST}. */ public SortedMap getInputMapping( - Spawn spawn, ArtifactExpander artifactExpander, MetadataProvider actionInputFileCache) + Spawn spawn, + ArtifactExpander artifactExpander, + MetadataProvider actionInputFileCache, + boolean expandTreeArtifactsInRunfiles) throws IOException { TreeMap 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; } diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java index 2fcfb05817c1ff..7a3c6c2b7f35c4 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java +++ b/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 getInputMapping() throws IOException; + SortedMap getInputMapping(boolean expandTreeArtifactsInRunfiles) + throws IOException; /** Reports a progress update to the Spawn strategy. */ void report(ProgressStatus state, String name); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index 619a67b2942c73..410ca80e67fc37 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/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 inputMap = context.getInputMapping(); + SortedMap inputMap = context.getInputMapping(true); TreeNode inputRoot = repository.buildFromActionInputs(inputMap); repository.computeMerkleDigests(inputRoot); Command command = diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index 18eb6bb8eaf290..d5bb71671dfeaa 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/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 inputMap = context.getInputMapping(); + SortedMap inputMap = context.getInputMapping(true); TreeNode inputRoot = repository.buildFromActionInputs(inputMap); repository.computeMerkleDigests(inputRoot); maybeWriteParamFilesLocally(spawn); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java index 6da13d34520269..e1b44433d29278 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java +++ b/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 inputs = SandboxHelpers.processInputFiles(spawn, context, execRoot); + Map inputs = + SandboxHelpers.processInputFiles( + spawn, + context, + execRoot, + getSandboxOptions().symlinkedSandboxExpandsTreeArtifactsInRunfilesTree); SandboxedSpawn sandbox; if (sandboxfsProcess != null) { diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java index 9b2f468066fa03..17dcd038228ed5 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java +++ b/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()); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java index 5e1339d255cc31..6d6255260fd0bf 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java +++ b/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); } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java index af5444fd90f8d0..be864c03e36a20 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java +++ b/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)); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java index ee6a959398c88d..289e2308642074 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java +++ b/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 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); } /** diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java index b22095dbfc59ff..d228a36a1faa01 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java +++ b/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 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; } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java index 2dce384801391f..46bb0dc554547c 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java +++ b/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"); diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java index 958b38f68adc97..e3af3f41a33fd4 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java @@ -76,18 +76,21 @@ final class WorkerSpawnRunner implements SpawnRunner { private final Multimap extraFlags; private final EventHandler reporter; private final SpawnRunner fallbackRunner; + private final boolean sandboxUsesExpandedTreeArtifactsInRunfiles; public WorkerSpawnRunner( Path execRoot, WorkerPool workers, Multimap 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 inputFiles = SandboxHelpers.processInputFiles(spawn, context, execRoot); + Map inputFiles = + SandboxHelpers.processInputFiles( + spawn, context, execRoot, sandboxUsesExpandedTreeArtifactsInRunfiles); Set outputFiles = SandboxHelpers.getOutputFiles(spawn); WorkerKey key = diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java index 201da354368651..99ed136cb78733 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java +++ b/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); diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java index 64b8b3e04db825..bc9fa265555988 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java @@ -242,7 +242,8 @@ public FileOutErr getFileOutErr() { } @Override - public SortedMap getInputMapping() { + public SortedMap getInputMapping( + boolean expandTreeArtifactsInRunfiles) { return inputMapping; } diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java index f87e78b42b83f6..8b392e23ce6c9c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java @@ -181,9 +181,10 @@ public FileOutErr getFileOutErr() { } @Override - public SortedMap getInputMapping() throws IOException { + public SortedMap 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 diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index b3387bd2d3dd91..7aba2488799435 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -144,9 +144,10 @@ public FileOutErr getFileOutErr() { } @Override - public SortedMap getInputMapping() throws IOException { + public SortedMap 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 diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index 105a881deac785..8d8123622677f9 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -1094,9 +1094,10 @@ public FileOutErr getFileOutErr() { } @Override - public SortedMap getInputMapping() throws IOException { + public SortedMap getInputMapping( + boolean expandTreeArtifactsInRunfiles) throws IOException { return new SpawnInputExpander(execRoot, /*strict*/ false) - .getInputMapping(spawn, artifactExpander, fakeFileCache); + .getInputMapping(spawn, artifactExpander, fakeFileCache, true); } @Override diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh index bd7a6d6db14182..0daee01d296c27 100755 --- a/src/test/shell/bazel/bazel_sandboxing_test.sh +++ b/src/test/shell/bazel/bazel_sandboxing_test.sh @@ -608,7 +608,6 @@ EOF } function test_sandbox_mount_customized_path () { - if ! [ "${PLATFORM-}" = "linux" -a \ "$(cat /dev/null /etc/*release | grep 'DISTRIB_CODENAME=' | sed 's/^.*=//')" = "trusty" ]; then echo "Skipping test: the toolchain used in this test is only supported on trusty." @@ -704,6 +703,64 @@ EOF rm -rf ${target_root}/x86_64-unknown-linux-gnu } +function test_experimental_symlinked_sandbox_uses_expanded_tree_artifacts_in_runfiles_tree() { + touch WORKSPACE + + cat > def.bzl <<'EOF' +def _mkdata_impl(ctx): + out = ctx.actions.declare_directory(ctx.label.name + ".d") + script = "mkdir -p {out}; touch {out}/file; ln -s file {out}/link".format(out = out.path) + ctx.actions.run_shell( + outputs = [out], + command = script, + ) + runfiles = ctx.runfiles(files = [out]) + return [DefaultInfo( + files = depset([out]), + runfiles = runfiles, + )] + +mkdata = rule( + _mkdata_impl, +) +EOF + + cat > mkdata_test.sh <<'EOF' +#!/bin/bash + +set -euo pipefail + +test_dir="$1" +cd "$test_dir" +ls -l | cut -f1,9 -d' ' >&2 + +if [ ! -f file -o -L file ]; then + echo "'file' is not a regular file" >&2 + exit 1 +fi +EOF + chmod +x mkdata_test.sh + + cat > BUILD <<'EOF' +load("//:def.bzl", "mkdata") + +mkdata(name = "mkdata") + +sh_test( + name = "mkdata_test", + srcs = ["mkdata_test.sh"], + args = ["$(location :mkdata)"], + data = [":mkdata"], +) +EOF + + bazel test --incompatible_symlinked_sandbox_expands_tree_artifacts_in_runfiles_tree \ + --test_output=streamed :mkdata_test &>$TEST_log && fail "expected test to fail" || true + + bazel test --noincompatible_symlinked_sandbox_expands_tree_artifacts_in_runfiles_tree \ + --test_output=streamed :mkdata_test &>$TEST_log || fail "expected test to pass" +} + # The test shouldn't fail if the environment doesn't support running it. check_supported_platform || exit 0 check_sandbox_allowed || exit 0