Skip to content

Commit

Permalink
Use the parent directory of the exec root as the input root on RBE.
Browse files Browse the repository at this point in the history 
This allows --experimental_sibling_repository_layout to work with RBE (otherwise, we'd tell RBE that the action requires inputs like `../$REPO/$PATH`, which is outside of the allowable subtree)

RELNOTES: None.
PiperOrigin-RevId: 356234098
  • Loading branch information
@lberki @coeuvre
lberki authored and coeuvre committed Jul 16, 2021
1 parent 4dd15c3 commit 1f3f9f4
Show file tree
Hide file tree
Showing 21 changed files with 264 additions and 157 deletions.
14 changes: 10 additions & 4 deletions src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public ImmutableList<SpawnResult> exec(
spawnLogContext.logSpawn(
spawn,
actionExecutionContext.getMetadataProvider(),
context.getInputMapping(),
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
context.getTimeout(),
spawnResult);
} catch (IOException e) {
Expand Down Expand Up @@ -213,6 +213,7 @@ private final class SpawnExecutionContextImpl implements SpawnExecutionContext {
// Memoize the input mapping so that prefetchInputs can reuse it instead of recomputing it.
// TODO(ulfjack): Guard against client modification of this map.
private SortedMap<PathFragment, ActionInput> lazyInputMapping;
private PathFragment inputMappingBaseDirectory;

SpawnExecutionContextImpl(
Spawn spawn,
Expand All @@ -235,7 +236,8 @@ public void prefetchInputs() throws IOException, InterruptedException {
if (Spawns.shouldPrefetchInputsForLocalExecution(spawn)) {
actionExecutionContext
.getActionInputPrefetcher()
.prefetchFiles(getInputMapping().values(), getMetadataProvider());
.prefetchFiles(
getInputMapping(PathFragment.EMPTY_FRAGMENT).values(), getMetadataProvider());
}
}

Expand Down Expand Up @@ -287,17 +289,21 @@ public FileOutErr getFileOutErr() {
}

@Override
public SortedMap<PathFragment, ActionInput> getInputMapping() throws IOException {
if (lazyInputMapping == null) {
public SortedMap<PathFragment, ActionInput> getInputMapping(PathFragment baseDirectory)
throws IOException {
if (lazyInputMapping == null || !inputMappingBaseDirectory.equals(baseDirectory)) {
try (SilentCloseable c =
Profiler.instance().profile("AbstractSpawnStrategy.getInputMapping")) {
inputMappingBaseDirectory = baseDirectory;
lazyInputMapping =
spawnInputExpander.getInputMapping(
spawn,
actionExecutionContext.getArtifactExpander(),
baseDirectory,
actionExecutionContext.getMetadataProvider());
}
}

return lazyInputMapping;
}

Expand Down
60 changes: 40 additions & 20 deletions src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,10 @@ public SpawnInputExpander(
private void addMapping(
Map<PathFragment, ActionInput> inputMappings,
PathFragment targetLocation,
ActionInput input) {
ActionInput input,
PathFragment baseDirectory) {
Preconditions.checkArgument(!targetLocation.isAbsolute(), targetLocation);
inputMappings.put(targetLocation, input);
inputMappings.put(baseDirectory.getRelative(targetLocation), input);
}

/** Adds runfiles inputs from runfilesSupplier to inputMappings. */
Expand All @@ -107,7 +108,8 @@ void addRunfilesToInputs(
Map<PathFragment, ActionInput> inputMap,
RunfilesSupplier runfilesSupplier,
MetadataProvider actionFileCache,
ArtifactExpander artifactExpander)
ArtifactExpander artifactExpander,
PathFragment baseDirectory)
throws IOException {
Map<PathFragment, Map<PathFragment, Artifact>> rootsAndMappings =
runfilesSupplier.getMappings();
Expand All @@ -129,7 +131,8 @@ void addRunfilesToInputs(
addMapping(
inputMap,
location.getRelative(((TreeFileArtifact) input).getParentRelativePath()),
input);
input,
baseDirectory);
}
} else if (localArtifact.isFileset()) {
ImmutableList<FilesetOutputSymlink> filesetLinks;
Expand All @@ -138,15 +141,15 @@ void addRunfilesToInputs(
} catch (MissingExpansionException e) {
throw new IllegalStateException(e);
}
addFilesetManifest(location, localArtifact, filesetLinks, inputMap);
addFilesetManifest(location, localArtifact, filesetLinks, inputMap, baseDirectory);
} else {
if (strict) {
failIfDirectory(actionFileCache, localArtifact);
}
addMapping(inputMap, location, localArtifact);
addMapping(inputMap, location, localArtifact, baseDirectory);
}
} else {
addMapping(inputMap, location, VirtualActionInput.EMPTY_MARKER);
addMapping(inputMap, location, VirtualActionInput.EMPTY_MARKER, baseDirectory);
}
}
}
Expand All @@ -156,10 +159,12 @@ void addRunfilesToInputs(
public Map<PathFragment, ActionInput> addRunfilesToInputs(
RunfilesSupplier runfilesSupplier,
MetadataProvider actionFileCache,
ArtifactExpander artifactExpander)
ArtifactExpander artifactExpander,
PathFragment baseDirectory)
throws IOException {
Map<PathFragment, ActionInput> inputMap = new HashMap<>();
addRunfilesToInputs(inputMap, runfilesSupplier, actionFileCache, artifactExpander);
addRunfilesToInputs(
inputMap, runfilesSupplier, actionFileCache, artifactExpander, baseDirectory);
return inputMap;
}

Expand All @@ -174,19 +179,25 @@ private static void failIfDirectory(MetadataProvider actionFileCache, ActionInpu
@VisibleForTesting
void addFilesetManifests(
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
Map<PathFragment, ActionInput> inputMappings)
Map<PathFragment, ActionInput> inputMappings,
PathFragment baseDirectory)
throws IOException {
for (Artifact fileset : filesetMappings.keySet()) {
addFilesetManifest(
fileset.getExecPath(), fileset, filesetMappings.get(fileset), inputMappings);
fileset.getExecPath(),
fileset,
filesetMappings.get(fileset),
inputMappings,
baseDirectory);
}
}

void addFilesetManifest(
PathFragment location,
Artifact filesetArtifact,
ImmutableList<FilesetOutputSymlink> filesetLinks,
Map<PathFragment, ActionInput> inputMappings)
Map<PathFragment, ActionInput> inputMappings,
PathFragment baseDirectory)
throws IOException {
Preconditions.checkState(filesetArtifact.isFileset(), filesetArtifact);
FilesetManifest filesetManifest =
Expand All @@ -198,16 +209,19 @@ void addFilesetManifest(
value == null
? VirtualActionInput.EMPTY_MARKER
: ActionInputHelper.fromPath(execRoot.getRelative(value).getPathString());
addMapping(inputMappings, mapping.getKey(), artifact);
addMapping(inputMappings, mapping.getKey(), artifact, baseDirectory);
}
}

private void addInputs(
Map<PathFragment, ActionInput> inputMap, Spawn spawn, ArtifactExpander artifactExpander) {
Map<PathFragment, ActionInput> inputMap,
Spawn spawn,
ArtifactExpander artifactExpander,
PathFragment baseDirectory) {
List<ActionInput> inputs =
ActionInputHelper.expandArtifacts(spawn.getInputFiles(), artifactExpander);
for (ActionInput input : inputs) {
addMapping(inputMap, input.getExecPath(), input);
addMapping(inputMap, input.getExecPath(), input, baseDirectory);
}
}

Expand All @@ -223,14 +237,20 @@ 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,
PathFragment baseDirectory,
MetadataProvider actionInputFileCache)
throws IOException {

TreeMap<PathFragment, ActionInput> inputMap = new TreeMap<>();
addInputs(inputMap, spawn, artifactExpander);
addInputs(inputMap, spawn, artifactExpander, baseDirectory);
addRunfilesToInputs(
inputMap, spawn.getRunfilesSupplier(), actionInputFileCache, artifactExpander);
addFilesetManifests(spawn.getFilesetMappings(), inputMap);
inputMap,
spawn.getRunfilesSupplier(),
actionInputFileCache,
artifactExpander,
baseDirectory);
addFilesetManifests(spawn.getFilesetMappings(), inputMap, baseDirectory);
return inputMap;
}
}
14 changes: 13 additions & 1 deletion src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,19 @@ default ArtifactPathResolver getPathResolver() {
/** The files to which to write stdout and stderr. */
FileOutErr getFileOutErr();

SortedMap<PathFragment, ActionInput> getInputMapping() throws IOException;
/**
* Returns a sorted map from input paths to action inputs.
*
* <p>Resolves cases where a single input of the {@link Spawn} gives rise to multiple files in
* the input tree, for example, tree artifacts, runfiles trees and {@code Fileset} input
* manifests.
*
* <p>{@code baseDirectory} is prepended to every path in the input key. This is useful if the
* mapping is used in a context where the directory relative to which the keys are interpreted
* is not the same as the execroot.
*/
SortedMap<PathFragment, ActionInput> getInputMapping(PathFragment baseDirectory)
throws IOException;

/** Reports a progress update to the Spawn strategy. */
void report(ProgressStatus state, String name);
Expand Down
27 changes: 17 additions & 10 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,10 @@ public void download(
FileOutErr origOutErr,
OutputFilesLocker outputFilesLocker)
throws ExecException, IOException, InterruptedException {
ActionResultMetadata metadata = parseActionResultMetadata(context, result, execRoot);
// The input root for RBE is the parent directory of the exec root so that paths to files in
// external repositories don't start with an uplevel reference
Path inputRoot = execRoot.getParentDirectory();
ActionResultMetadata metadata = parseActionResultMetadata(context, result, inputRoot);

List<ListenableFuture<FileMetadata>> downloads =
Stream.concat(
Expand Down Expand Up @@ -349,12 +352,12 @@ public void download(
try {
// Delete any (partially) downloaded output files.
for (OutputFile file : result.getOutputFilesList()) {
toTmpDownloadPath(execRoot.getRelative(file.getPath())).delete();
toTmpDownloadPath(inputRoot.getRelative(file.getPath())).delete();
}
for (OutputDirectory directory : result.getOutputDirectoriesList()) {
// Only delete the directories below the output directories because the output
// directories will not be re-created
execRoot.getRelative(directory.getPath()).deleteTreesBelow();
inputRoot.getRelative(directory.getPath()).deleteTreesBelow();
}
if (tmpOutErr != null) {
tmpOutErr.clearOut();
Expand Down Expand Up @@ -589,7 +592,11 @@ public InMemoryOutput downloadMinimal(

ActionResultMetadata metadata;
try (SilentCloseable c = Profiler.instance().profile("Remote.parseActionResultMetadata")) {
metadata = parseActionResultMetadata(context, result, execRoot);
// We tell RBE that the input root of the action is the parent directory of what is locally
// the execroot. This is so that paths of artifacts in external repositories don't start with
// an uplevel reference.
Path inputRoot = execRoot.getParentDirectory();
metadata = parseActionResultMetadata(context, result, inputRoot);
}

if (!metadata.symlinks().isEmpty()) {
Expand Down Expand Up @@ -720,14 +727,14 @@ private DirectoryMetadata parseDirectory(
}

private ActionResultMetadata parseActionResultMetadata(
RemoteActionExecutionContext context, ActionResult actionResult, Path execRoot)
RemoteActionExecutionContext context, ActionResult actionResult, Path inputRoot)
throws IOException, InterruptedException {
Preconditions.checkNotNull(actionResult, "actionResult");
Map<Path, ListenableFuture<Tree>> dirMetadataDownloads =
Maps.newHashMapWithExpectedSize(actionResult.getOutputDirectoriesCount());
for (OutputDirectory dir : actionResult.getOutputDirectoriesList()) {
dirMetadataDownloads.put(
execRoot.getRelative(dir.getPath()),
inputRoot.getRelative(dir.getPath()),
Futures.transform(
downloadBlob(context, dir.getTreeDigest()),
(treeBytes) -> {
Expand Down Expand Up @@ -758,9 +765,9 @@ private ActionResultMetadata parseActionResultMetadata(
ImmutableMap.Builder<Path, FileMetadata> files = ImmutableMap.builder();
for (OutputFile outputFile : actionResult.getOutputFilesList()) {
files.put(
execRoot.getRelative(outputFile.getPath()),
inputRoot.getRelative(outputFile.getPath()),
new FileMetadata(
execRoot.getRelative(outputFile.getPath()),
inputRoot.getRelative(outputFile.getPath()),
outputFile.getDigest(),
outputFile.getIsExecutable()));
}
Expand All @@ -772,9 +779,9 @@ private ActionResultMetadata parseActionResultMetadata(
actionResult.getOutputDirectorySymlinksList());
for (OutputSymlink symlink : outputSymlinks) {
symlinks.put(
execRoot.getRelative(symlink.getPath()),
inputRoot.getRelative(symlink.getPath()),
new SymlinkMetadata(
execRoot.getRelative(symlink.getPath()), PathFragment.create(symlink.getTarget())));
inputRoot.getRelative(symlink.getPath()), PathFragment.create(symlink.getTarget())));
}

return new ActionResultMetadata(files.build(), symlinks.build(), directories.build());
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)

Stopwatch totalTime = Stopwatch.createStarted();

SortedMap<PathFragment, ActionInput> inputMap = context.getInputMapping();
SortedMap<PathFragment, ActionInput> inputMap =
context.getInputMapping(PathFragment.create(execRoot.getBaseName()));
MerkleTree merkleTree =
MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil);
SpawnMetrics.Builder spawnMetrics =
Expand All @@ -143,7 +144,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
spawn.getArguments(),
spawn.getEnvironment(),
platform,
/* workingDirectory= */ null);
execRoot.getBaseName());
RemoteOutputsMode remoteOutputsMode = options.remoteOutputsMode;
Action action =
RemoteSpawnRunner.buildAction(
Expand Down Expand Up @@ -290,7 +291,7 @@ public void store(SpawnResult result) throws ExecException, InterruptedException
actionKey,
action,
command,
execRoot,
execRoot.getParentDirectory(),
files,
context.getFileOutErr());
} catch (IOException e) {
Expand Down
24 changes: 18 additions & 6 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,14 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)

context.report(ProgressStatus.SCHEDULING, getName());
RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
SortedMap<PathFragment, ActionInput> inputMap = context.getInputMapping();
// The "root directory" of the action from the point of view of RBE is the parent directory of
// the execroot locally. This is so that paths of artifacts in external repositories don't
// start with an uplevel reference...
SortedMap<PathFragment, ActionInput> inputMap =
context.getInputMapping(PathFragment.create(execRoot.getBaseName()));

// ...however, MerkleTree.build() uses its execRoot parameter to resolve artifacts based on
// ActionInput.getExecPath(), so it needs the execroot and not its parent directory.
final MerkleTree merkleTree =
MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil);
SpawnMetrics.Builder spawnMetrics =
Expand All @@ -231,7 +238,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
spawn.getArguments(),
spawn.getEnvironment(),
platform,
/* workingDirectory= */ null);
execRoot.getBaseName());
Digest commandHash = digestUtil.compute(command);
Action action =
buildAction(
Expand Down Expand Up @@ -719,12 +726,17 @@ static Command buildCommand(
List<String> arguments,
ImmutableMap<String, String> env,
@Nullable Platform platform,
@Nullable String workingDirectory) {
@Nullable String workingDirectoryString) {
Command.Builder command = Command.newBuilder();
ArrayList<String> outputFiles = new ArrayList<>();
ArrayList<String> outputDirectories = new ArrayList<>();
PathFragment workingDirectoryPathFragment =
workingDirectoryString == null
? PathFragment.EMPTY_FRAGMENT
: PathFragment.create(workingDirectoryString);
for (ActionInput output : outputs) {
String pathString = output.getExecPathString();
String pathString =
workingDirectoryPathFragment.getRelative(output.getExecPath()).getPathString();
if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
outputDirectories.add(pathString);
} else {
Expand All @@ -746,8 +758,8 @@ static Command buildCommand(
command.addEnvironmentVariablesBuilder().setName(var).setValue(env.get(var));
}

if (!Strings.isNullOrEmpty(workingDirectory)) {
command.setWorkingDirectory(workingDirectory);
if (!Strings.isNullOrEmpty(workingDirectoryString)) {
command.setWorkingDirectory(workingDirectoryString);
}
return command.build();
}
Expand Down
Loading

0 comments on commit 1f3f9f4

Please sign in to comment.