Skip to content

Commit 7149f57

Browse files
lberkicopybara-github
lberki
authored and
copybara-github
committed
Use the parent directory of the exec root as the input root on RBE.
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
1 parent 349f5f7 commit 7149f57

21 files changed

+264
-157
lines changed

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ public ImmutableList<SpawnResult> exec(
166166
spawnLogContext.logSpawn(
167167
spawn,
168168
actionExecutionContext.getMetadataProvider(),
169-
context.getInputMapping(),
169+
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
170170
context.getTimeout(),
171171
spawnResult);
172172
} catch (IOException e) {
@@ -207,6 +207,7 @@ private final class SpawnExecutionContextImpl implements SpawnExecutionContext {
207207
// Memoize the input mapping so that prefetchInputs can reuse it instead of recomputing it.
208208
// TODO(ulfjack): Guard against client modification of this map.
209209
private SortedMap<PathFragment, ActionInput> lazyInputMapping;
210+
private PathFragment inputMappingBaseDirectory;
210211

211212
SpawnExecutionContextImpl(
212213
Spawn spawn,
@@ -229,7 +230,8 @@ public void prefetchInputs() throws IOException, InterruptedException {
229230
if (Spawns.shouldPrefetchInputsForLocalExecution(spawn)) {
230231
actionExecutionContext
231232
.getActionInputPrefetcher()
232-
.prefetchFiles(getInputMapping().values(), getMetadataProvider());
233+
.prefetchFiles(
234+
getInputMapping(PathFragment.EMPTY_FRAGMENT).values(), getMetadataProvider());
233235
}
234236
}
235237

@@ -281,17 +283,21 @@ public FileOutErr getFileOutErr() {
281283
}
282284

283285
@Override
284-
public SortedMap<PathFragment, ActionInput> getInputMapping() throws IOException {
285-
if (lazyInputMapping == null) {
286+
public SortedMap<PathFragment, ActionInput> getInputMapping(PathFragment baseDirectory)
287+
throws IOException {
288+
if (lazyInputMapping == null || !inputMappingBaseDirectory.equals(baseDirectory)) {
286289
try (SilentCloseable c =
287290
Profiler.instance().profile("AbstractSpawnStrategy.getInputMapping")) {
291+
inputMappingBaseDirectory = baseDirectory;
288292
lazyInputMapping =
289293
spawnInputExpander.getInputMapping(
290294
spawn,
291295
actionExecutionContext.getArtifactExpander(),
296+
baseDirectory,
292297
actionExecutionContext.getMetadataProvider());
293298
}
294299
}
300+
295301
return lazyInputMapping;
296302
}
297303

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

Lines changed: 40 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,10 @@ public SpawnInputExpander(
9696
private void addMapping(
9797
Map<PathFragment, ActionInput> inputMappings,
9898
PathFragment targetLocation,
99-
ActionInput input) {
99+
ActionInput input,
100+
PathFragment baseDirectory) {
100101
Preconditions.checkArgument(!targetLocation.isAbsolute(), targetLocation);
101-
inputMappings.put(targetLocation, input);
102+
inputMappings.put(baseDirectory.getRelative(targetLocation), input);
102103
}
103104

104105
/** Adds runfiles inputs from runfilesSupplier to inputMappings. */
@@ -107,7 +108,8 @@ void addRunfilesToInputs(
107108
Map<PathFragment, ActionInput> inputMap,
108109
RunfilesSupplier runfilesSupplier,
109110
MetadataProvider actionFileCache,
110-
ArtifactExpander artifactExpander)
111+
ArtifactExpander artifactExpander,
112+
PathFragment baseDirectory)
111113
throws IOException {
112114
Map<PathFragment, Map<PathFragment, Artifact>> rootsAndMappings =
113115
runfilesSupplier.getMappings();
@@ -129,7 +131,8 @@ void addRunfilesToInputs(
129131
addMapping(
130132
inputMap,
131133
location.getRelative(((TreeFileArtifact) input).getParentRelativePath()),
132-
input);
134+
input,
135+
baseDirectory);
133136
}
134137
} else if (localArtifact.isFileset()) {
135138
ImmutableList<FilesetOutputSymlink> filesetLinks;
@@ -138,15 +141,15 @@ void addRunfilesToInputs(
138141
} catch (MissingExpansionException e) {
139142
throw new IllegalStateException(e);
140143
}
141-
addFilesetManifest(location, localArtifact, filesetLinks, inputMap);
144+
addFilesetManifest(location, localArtifact, filesetLinks, inputMap, baseDirectory);
142145
} else {
143146
if (strict) {
144147
failIfDirectory(actionFileCache, localArtifact);
145148
}
146-
addMapping(inputMap, location, localArtifact);
149+
addMapping(inputMap, location, localArtifact, baseDirectory);
147150
}
148151
} else {
149-
addMapping(inputMap, location, VirtualActionInput.EMPTY_MARKER);
152+
addMapping(inputMap, location, VirtualActionInput.EMPTY_MARKER, baseDirectory);
150153
}
151154
}
152155
}
@@ -156,10 +159,12 @@ void addRunfilesToInputs(
156159
public Map<PathFragment, ActionInput> addRunfilesToInputs(
157160
RunfilesSupplier runfilesSupplier,
158161
MetadataProvider actionFileCache,
159-
ArtifactExpander artifactExpander)
162+
ArtifactExpander artifactExpander,
163+
PathFragment baseDirectory)
160164
throws IOException {
161165
Map<PathFragment, ActionInput> inputMap = new HashMap<>();
162-
addRunfilesToInputs(inputMap, runfilesSupplier, actionFileCache, artifactExpander);
166+
addRunfilesToInputs(
167+
inputMap, runfilesSupplier, actionFileCache, artifactExpander, baseDirectory);
163168
return inputMap;
164169
}
165170

@@ -174,19 +179,25 @@ private static void failIfDirectory(MetadataProvider actionFileCache, ActionInpu
174179
@VisibleForTesting
175180
void addFilesetManifests(
176181
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
177-
Map<PathFragment, ActionInput> inputMappings)
182+
Map<PathFragment, ActionInput> inputMappings,
183+
PathFragment baseDirectory)
178184
throws IOException {
179185
for (Artifact fileset : filesetMappings.keySet()) {
180186
addFilesetManifest(
181-
fileset.getExecPath(), fileset, filesetMappings.get(fileset), inputMappings);
187+
fileset.getExecPath(),
188+
fileset,
189+
filesetMappings.get(fileset),
190+
inputMappings,
191+
baseDirectory);
182192
}
183193
}
184194

185195
void addFilesetManifest(
186196
PathFragment location,
187197
Artifact filesetArtifact,
188198
ImmutableList<FilesetOutputSymlink> filesetLinks,
189-
Map<PathFragment, ActionInput> inputMappings)
199+
Map<PathFragment, ActionInput> inputMappings,
200+
PathFragment baseDirectory)
190201
throws IOException {
191202
Preconditions.checkState(filesetArtifact.isFileset(), filesetArtifact);
192203
FilesetManifest filesetManifest =
@@ -198,16 +209,19 @@ void addFilesetManifest(
198209
value == null
199210
? VirtualActionInput.EMPTY_MARKER
200211
: ActionInputHelper.fromPath(execRoot.getRelative(value).getPathString());
201-
addMapping(inputMappings, mapping.getKey(), artifact);
212+
addMapping(inputMappings, mapping.getKey(), artifact, baseDirectory);
202213
}
203214
}
204215

205216
private void addInputs(
206-
Map<PathFragment, ActionInput> inputMap, Spawn spawn, ArtifactExpander artifactExpander) {
217+
Map<PathFragment, ActionInput> inputMap,
218+
Spawn spawn,
219+
ArtifactExpander artifactExpander,
220+
PathFragment baseDirectory) {
207221
List<ActionInput> inputs =
208222
ActionInputHelper.expandArtifacts(spawn.getInputFiles(), artifactExpander);
209223
for (ActionInput input : inputs) {
210-
addMapping(inputMap, input.getExecPath(), input);
224+
addMapping(inputMap, input.getExecPath(), input, baseDirectory);
211225
}
212226
}
213227

@@ -223,14 +237,20 @@ private void addInputs(
223237
* <p>The returned map contains all runfiles, but not the {@code MANIFEST}.
224238
*/
225239
public SortedMap<PathFragment, ActionInput> getInputMapping(
226-
Spawn spawn, ArtifactExpander artifactExpander, MetadataProvider actionInputFileCache)
240+
Spawn spawn,
241+
ArtifactExpander artifactExpander,
242+
PathFragment baseDirectory,
243+
MetadataProvider actionInputFileCache)
227244
throws IOException {
228-
229245
TreeMap<PathFragment, ActionInput> inputMap = new TreeMap<>();
230-
addInputs(inputMap, spawn, artifactExpander);
246+
addInputs(inputMap, spawn, artifactExpander, baseDirectory);
231247
addRunfilesToInputs(
232-
inputMap, spawn.getRunfilesSupplier(), actionInputFileCache, artifactExpander);
233-
addFilesetManifests(spawn.getFilesetMappings(), inputMap);
248+
inputMap,
249+
spawn.getRunfilesSupplier(),
250+
actionInputFileCache,
251+
artifactExpander,
252+
baseDirectory);
253+
addFilesetManifests(spawn.getFilesetMappings(), inputMap, baseDirectory);
234254
return inputMap;
235255
}
236256
}

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,19 @@ default ArtifactPathResolver getPathResolver() {
198198
/** The files to which to write stdout and stderr. */
199199
FileOutErr getFileOutErr();
200200

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

203215
/** Reports a progress update to the Spawn strategy. */
204216
void report(ProgressStatus state, String name);

‎src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,10 @@ public void download(
314314
FileOutErr origOutErr,
315315
OutputFilesLocker outputFilesLocker)
316316
throws ExecException, IOException, InterruptedException {
317-
ActionResultMetadata metadata = parseActionResultMetadata(context, result, execRoot);
317+
// The input root for RBE is the parent directory of the exec root so that paths to files in
318+
// external repositories don't start with an uplevel reference
319+
Path inputRoot = execRoot.getParentDirectory();
320+
ActionResultMetadata metadata = parseActionResultMetadata(context, result, inputRoot);
318321

319322
List<ListenableFuture<FileMetadata>> downloads =
320323
Stream.concat(
@@ -349,12 +352,12 @@ public void download(
349352
try {
350353
// Delete any (partially) downloaded output files.
351354
for (OutputFile file : result.getOutputFilesList()) {
352-
toTmpDownloadPath(execRoot.getRelative(file.getPath())).delete();
355+
toTmpDownloadPath(inputRoot.getRelative(file.getPath())).delete();
353356
}
354357
for (OutputDirectory directory : result.getOutputDirectoriesList()) {
355358
// Only delete the directories below the output directories because the output
356359
// directories will not be re-created
357-
execRoot.getRelative(directory.getPath()).deleteTreesBelow();
360+
inputRoot.getRelative(directory.getPath()).deleteTreesBelow();
358361
}
359362
if (tmpOutErr != null) {
360363
tmpOutErr.clearOut();
@@ -589,7 +592,11 @@ public InMemoryOutput downloadMinimal(
589592

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

595602
if (!metadata.symlinks().isEmpty()) {
@@ -720,14 +727,14 @@ private DirectoryMetadata parseDirectory(
720727
}
721728

722729
private ActionResultMetadata parseActionResultMetadata(
723-
RemoteActionExecutionContext context, ActionResult actionResult, Path execRoot)
730+
RemoteActionExecutionContext context, ActionResult actionResult, Path inputRoot)
724731
throws IOException, InterruptedException {
725732
Preconditions.checkNotNull(actionResult, "actionResult");
726733
Map<Path, ListenableFuture<Tree>> dirMetadataDownloads =
727734
Maps.newHashMapWithExpectedSize(actionResult.getOutputDirectoriesCount());
728735
for (OutputDirectory dir : actionResult.getOutputDirectoriesList()) {
729736
dirMetadataDownloads.put(
730-
execRoot.getRelative(dir.getPath()),
737+
inputRoot.getRelative(dir.getPath()),
731738
Futures.transform(
732739
downloadBlob(context, dir.getTreeDigest()),
733740
(treeBytes) -> {
@@ -758,9 +765,9 @@ private ActionResultMetadata parseActionResultMetadata(
758765
ImmutableMap.Builder<Path, FileMetadata> files = ImmutableMap.builder();
759766
for (OutputFile outputFile : actionResult.getOutputFilesList()) {
760767
files.put(
761-
execRoot.getRelative(outputFile.getPath()),
768+
inputRoot.getRelative(outputFile.getPath()),
762769
new FileMetadata(
763-
execRoot.getRelative(outputFile.getPath()),
770+
inputRoot.getRelative(outputFile.getPath()),
764771
outputFile.getDigest(),
765772
outputFile.getIsExecutable()));
766773
}
@@ -772,9 +779,9 @@ private ActionResultMetadata parseActionResultMetadata(
772779
actionResult.getOutputDirectorySymlinksList());
773780
for (OutputSymlink symlink : outputSymlinks) {
774781
symlinks.put(
775-
execRoot.getRelative(symlink.getPath()),
782+
inputRoot.getRelative(symlink.getPath()),
776783
new SymlinkMetadata(
777-
execRoot.getRelative(symlink.getPath()), PathFragment.create(symlink.getTarget())));
784+
inputRoot.getRelative(symlink.getPath()), PathFragment.create(symlink.getTarget())));
778785
}
779786

780787
return new ActionResultMetadata(files.build(), symlinks.build(), directories.build());

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,8 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
125125

126126
Stopwatch totalTime = Stopwatch.createStarted();
127127

128-
SortedMap<PathFragment, ActionInput> inputMap = context.getInputMapping();
128+
SortedMap<PathFragment, ActionInput> inputMap =
129+
context.getInputMapping(PathFragment.create(execRoot.getBaseName()));
129130
MerkleTree merkleTree =
130131
MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil);
131132
SpawnMetrics.Builder spawnMetrics =
@@ -143,7 +144,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
143144
spawn.getArguments(),
144145
spawn.getEnvironment(),
145146
platform,
146-
/* workingDirectory= */ null);
147+
execRoot.getBaseName());
147148
RemoteOutputsMode remoteOutputsMode = options.remoteOutputsMode;
148149
Action action =
149150
RemoteSpawnRunner.buildAction(
@@ -290,7 +291,7 @@ public void store(SpawnResult result) throws ExecException, InterruptedException
290291
actionKey,
291292
action,
292293
command,
293-
execRoot,
294+
execRoot.getParentDirectory(),
294295
files,
295296
context.getFileOutErr());
296297
} catch (IOException e) {

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

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,14 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
213213

214214
context.report(ProgressStatus.SCHEDULING, getName());
215215
RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
216-
SortedMap<PathFragment, ActionInput> inputMap = context.getInputMapping();
216+
// The "root directory" of the action from the point of view of RBE is the parent directory of
217+
// the execroot locally. This is so that paths of artifacts in external repositories don't
218+
// start with an uplevel reference...
219+
SortedMap<PathFragment, ActionInput> inputMap =
220+
context.getInputMapping(PathFragment.create(execRoot.getBaseName()));
221+
222+
// ...however, MerkleTree.build() uses its execRoot parameter to resolve artifacts based on
223+
// ActionInput.getExecPath(), so it needs the execroot and not its parent directory.
217224
final MerkleTree merkleTree =
218225
MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil);
219226
SpawnMetrics.Builder spawnMetrics =
@@ -231,7 +238,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
231238
spawn.getArguments(),
232239
spawn.getEnvironment(),
233240
platform,
234-
/* workingDirectory= */ null);
241+
execRoot.getBaseName());
235242
Digest commandHash = digestUtil.compute(command);
236243
Action action =
237244
buildAction(
@@ -707,12 +714,17 @@ static Command buildCommand(
707714
List<String> arguments,
708715
ImmutableMap<String, String> env,
709716
@Nullable Platform platform,
710-
@Nullable String workingDirectory) {
717+
@Nullable String workingDirectoryString) {
711718
Command.Builder command = Command.newBuilder();
712719
ArrayList<String> outputFiles = new ArrayList<>();
713720
ArrayList<String> outputDirectories = new ArrayList<>();
721+
PathFragment workingDirectoryPathFragment =
722+
workingDirectoryString == null
723+
? PathFragment.EMPTY_FRAGMENT
724+
: PathFragment.create(workingDirectoryString);
714725
for (ActionInput output : outputs) {
715-
String pathString = output.getExecPathString();
726+
String pathString =
727+
workingDirectoryPathFragment.getRelative(output.getExecPath()).getPathString();
716728
if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
717729
outputDirectories.add(pathString);
718730
} else {
@@ -734,8 +746,8 @@ static Command buildCommand(
734746
command.addEnvironmentVariablesBuilder().setName(var).setValue(env.get(var));
735747
}
736748

737-
if (!Strings.isNullOrEmpty(workingDirectory)) {
738-
command.setWorkingDirectory(workingDirectory);
749+
if (!Strings.isNullOrEmpty(workingDirectoryString)) {
750+
command.setWorkingDirectory(workingDirectoryString);
739751
}
740752
return command.build();
741753
}

0 commit comments

Comments
 (0)