Skip to content

Commit

Permalink
Add flag --incompatible_symlinked_sandbox_expands_tree_artifacts_in_r…
Browse files Browse the repository at this point in the history
…unfiles_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
  • Loading branch information
philwo authored and Copybara-Service committed Sep 3, 2018
1 parent 3eec3fe commit 9ed9d8a
Show file tree
Hide file tree
Showing 19 changed files with 167 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public List<SpawnResult> exec(
spawnLogContext.logSpawn(
spawn,
actionExecutionContext.getMetadataProvider(),
context.getInputMapping(),
context.getInputMapping(true),
context.getTimeout(),
spawnResult);
} catch (IOException e) {
Expand Down Expand Up @@ -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());
}
}

Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand All @@ -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);
Expand All @@ -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");
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,8 @@ public FileOutErr getFileOutErr() {
}

@Override
public SortedMap<PathFragment, ActionInput> getInputMapping() {
public SortedMap<PathFragment, ActionInput> getInputMapping(
boolean expandTreeArtifactsInRunfiles) {
return inputMapping;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 9ed9d8a

Please sign in to comment.