Skip to content

Commit

Permalink
Temporarily set parent directory of the input to writable if it is not.
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 483615392
Change-Id: Ic8b301ef6fd004a88f9a563e04ba961f6dfb5708
  • Loading branch information
coeuvre authored and copybara-github committed Oct 25, 2022
1 parent 5cea7dd commit ec7be34
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,20 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet
protected final Path execRoot;
protected final ImmutableList<Pattern> patternsToDownload;

private static class Context {
private final Set<Path> nonWritableDirs = Sets.newConcurrentHashSet();

public void addNonWritableDir(Path dir) {
nonWritableDirs.add(dir);
}

public void finalizeContext() throws IOException {
for (Path path : nonWritableDirs) {
path.setWritable(false);
}
}
}

/** Priority for the staging task. */
protected enum Priority {
/**
Expand Down Expand Up @@ -176,27 +190,37 @@ protected ListenableFuture<Void> prefetchFiles(
files.add(input);
}

Context context = new Context();

Flowable<TransferResult> treeDownloads =
Flowable.fromIterable(trees.entrySet())
.flatMapSingle(
entry ->
toTransferResult(
prefetchInputTreeOrSymlink(
metadataProvider, entry.getKey(), entry.getValue(), priority)));
context,
metadataProvider,
entry.getKey(),
entry.getValue(),
priority)));

Flowable<TransferResult> fileDownloads =
Flowable.fromIterable(files)
.flatMapSingle(
input ->
toTransferResult(
prefetchInputFileOrSymlink(metadataProvider, input, priority)));
prefetchInputFileOrSymlink(context, metadataProvider, input, priority)));

Flowable<TransferResult> transfers = Flowable.merge(treeDownloads, fileDownloads);
Completable prefetch = mergeBulkTransfer(transfers).onErrorResumeNext(this::onErrorResumeNext);
Completable prefetch =
Completable.using(
() -> context, ctx -> mergeBulkTransfer(transfers), Context::finalizeContext)
.onErrorResumeNext(this::onErrorResumeNext);
return toListenableFuture(prefetch);
}

private Completable prefetchInputTreeOrSymlink(
Context context,
MetadataProvider provider,
SpecialArtifact tree,
List<TreeFileArtifact> treeFiles,
Expand All @@ -216,7 +240,7 @@ private Completable prefetchInputTreeOrSymlink(
PathFragment prefetchExecPath = treeMetadata.getMaterializationExecPath().orElse(execPath);

Completable prefetch =
prefetchInputTree(provider, prefetchExecPath, treeFiles, treeMetadata, priority);
prefetchInputTree(context, provider, prefetchExecPath, treeFiles, treeMetadata, priority);

// If prefetching to a different path, plant a symlink into it.
if (!prefetchExecPath.equals(execPath)) {
Expand Down Expand Up @@ -249,6 +273,7 @@ private boolean shouldDownloadAnyTreeFiles(
}

private Completable prefetchInputTree(
Context context,
MetadataProvider provider,
PathFragment execPath,
List<TreeFileArtifact> treeFiles,
Expand Down Expand Up @@ -303,7 +328,7 @@ private Completable prefetchInputTree(
}
}
checkState(dir.equals(path));
finalizeDownload(tempPath, path);
finalizeDownload(context, tempPath, path);
}

for (Path dir : dirs) {
Expand Down Expand Up @@ -337,7 +362,8 @@ private Completable prefetchInputTree(
}

private Completable prefetchInputFileOrSymlink(
MetadataProvider metadataProvider, ActionInput input, Priority priority) throws IOException {
Context context, MetadataProvider metadataProvider, ActionInput input, Priority priority)
throws IOException {
if (input instanceof VirtualActionInput) {
prefetchVirtualActionInput((VirtualActionInput) input);
return Completable.complete();
Expand All @@ -353,7 +379,7 @@ private Completable prefetchInputFileOrSymlink(
PathFragment prefetchExecPath = metadata.getMaterializationExecPath().orElse(execPath);

Completable prefetch =
downloadFileNoCheckRx(execRoot.getRelative(prefetchExecPath), metadata, priority);
downloadFileNoCheckRx(context, execRoot.getRelative(prefetchExecPath), metadata, priority);

// If prefetching to a different path, plant a symlink into it.
if (!prefetchExecPath.equals(execPath)) {
Expand All @@ -371,15 +397,16 @@ private Completable prefetchInputFileOrSymlink(
* <p>The file will be written into a temporary file and moved to the final destination after the
* download finished.
*/
private Completable downloadFileRx(Path path, FileArtifactValue metadata, Priority priority) {
private Completable downloadFileRx(
Context context, Path path, FileArtifactValue metadata, Priority priority) {
if (!canDownloadFile(path, metadata)) {
return Completable.complete();
}
return downloadFileNoCheckRx(path, metadata, priority);
return downloadFileNoCheckRx(context, path, metadata, priority);
}

private Completable downloadFileNoCheckRx(
Path path, FileArtifactValue metadata, Priority priority) {
Context context, Path path, FileArtifactValue metadata, Priority priority) {
if (path.isSymbolicLink()) {
try {
path = path.getRelative(path.readSymbolicLink());
Expand All @@ -402,7 +429,7 @@ private Completable downloadFileNoCheckRx(
directExecutor())
.doOnComplete(
() -> {
finalizeDownload(tempPath, finalPath);
finalizeDownload(context, tempPath, finalPath);
completed.set(true);
}),
tempPath -> {
Expand Down Expand Up @@ -439,11 +466,24 @@ public void downloadFile(Path path, FileArtifactValue metadata)

protected ListenableFuture<Void> downloadFileAsync(
PathFragment path, FileArtifactValue metadata, Priority priority) {
Context context = new Context();
return toListenableFuture(
downloadFileRx(execRoot.getFileSystem().getPath(path), metadata, priority));
Completable.using(
() -> context,
ctx ->
downloadFileRx(context, execRoot.getFileSystem().getPath(path), metadata, priority),
Context::finalizeContext));
}

private void finalizeDownload(Path tmpPath, Path path) throws IOException {
private void finalizeDownload(Context context, Path tmpPath, Path path) throws IOException {
Path parentDir = path.getParentDirectory();
// In case the parent directory of the destination is not writable, temporarily change it to
// writable. b/254844173.
if (parentDir != null && !parentDir.isWritable()) {
context.addNonWritableDir(parentDir);
parentDir.setWritable(true);
}

// The permission of output file is changed to 0555 after action execution. We manually change
// the permission here for the downloaded file to keep this behaviour consistent.
tmpPath.chmod(0555);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -727,8 +727,10 @@ void updateActionCache(
// Skyframe has already done all the filesystem access needed for outputs and swallows
// IOExceptions for inputs. So an IOException is impossible here.
throw new IllegalStateException(
"failed to update action cache for " + action.prettyPrint()
+ ", but all outputs should already have been checked", e);
"failed to update action cache for "
+ action.prettyPrint()
+ ", but all outputs should already have been checked",
e);
}
}

Expand Down Expand Up @@ -866,12 +868,12 @@ void recordExecutionError() {
}

/**
* Returns true if the Builder is winding down (i.e. cancelling outstanding
* actions and preparing to abort.)
* The builder is winding down iff:
* Returns true if the Builder is winding down (i.e. cancelling outstanding actions and preparing
* to abort.) The builder is winding down iff:
*
* <ul>
* <li>we had an execution error
* <li>we are not running with --keep_going
* <li>we had an execution error
* <li>we are not running with --keep_going
* </ul>
*/
private boolean isBuilderAborting() {
Expand Down Expand Up @@ -1197,8 +1199,10 @@ private ActionExecutionValue actuallyCompleteAction(
Artifact primaryOutput = action.getPrimaryOutput();
Path primaryOutputPath = actionExecutionContext.getInputPath(primaryOutput);
try {
Preconditions.checkState(action.inputsDiscovered(),
"Action %s successfully executed, but inputs still not known", action);
Preconditions.checkState(
action.inputsDiscovered(),
"Action %s successfully executed, but inputs still not known",
action);

try {
flushActionFileSystem(actionExecutionContext.getActionFileSystem(), outputService);
Expand Down Expand Up @@ -1481,12 +1485,15 @@ private static void reportMissingOutputFile(
String msg = prefix + "is a dangling symbolic link";
reporter.handle(Event.error(action.getOwner().getLocation(), msg));
} else {
String suffix = genrule ? " by genrule. This is probably "
+ "because the genrule actually didn't create this output, or because the output was a "
+ "directory and the genrule was run remotely (note that only the contents of "
+ "declared file outputs are copied from genrules run remotely)" : "";
reporter.handle(Event.error(
action.getOwner().getLocation(), prefix + "was not created" + suffix));
String suffix =
genrule
? " by genrule. This is probably because the genrule actually didn't create this"
+ " output, or because the output was a directory and the genrule was run"
+ " remotely (note that only the contents of declared file outputs are copied"
+ " from genrules run remotely)"
: "";
reporter.handle(
Event.error(action.getOwner().getLocation(), prefix + "was not created" + suffix));
}
}

Expand All @@ -1496,8 +1503,9 @@ private static void reportOutputTreeArtifactErrors(
if (e instanceof FileNotFoundException) {
errorMessage = String.format("TreeArtifact %s was not created", output.prettyPrint());
} else {
errorMessage = String.format(
"Error while validating output TreeArtifact %s : %s", output, e.getMessage());
errorMessage =
String.format(
"Error while validating output TreeArtifact %s : %s", output, e.getMessage());
}

reporter.handle(Event.error(action.getOwner().getLocation(), errorMessage));
Expand Down

0 comments on commit ec7be34

Please sign in to comment.