diff options
Diffstat (limited to 'src/main')
4 files changed, 161 insertions, 43 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java index 8bceb9acd3..d134f2d6f4 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java @@ -18,6 +18,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.base.Functions; import com.google.common.collect.Collections2; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.util.Preconditions; @@ -26,6 +27,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Set; /** * Helper utility to create ActionInput instances. @@ -172,6 +174,20 @@ public final class ActionInputHelper { }); } + /** Returns an Set of ArtifactFiles with the given parent and parent relative paths. */ + public static Set<ArtifactFile> asArtifactFiles( + final Artifact parent, Set<? extends PathFragment> parentRelativePaths) { + Preconditions.checkState(parent.isTreeArtifact(), + "Given parent %s must be a TreeArtifact", parent); + + ImmutableSet.Builder<ArtifactFile> builder = ImmutableSet.builder(); + for (PathFragment path : parentRelativePaths) { + builder.add(artifactFile(parent, path)); + } + + return builder.build(); + } + /** * Expands middleman artifacts in a sequence of {@link ActionInput}s. * diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 0a210149d1..d45d282ac3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -15,8 +15,7 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.base.Function; import com.google.common.base.Predicates; -import com.google.common.collect.Collections2; -import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.common.collect.Sets; @@ -25,6 +24,7 @@ import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionCacheChecker.Token; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionExecutionException; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFile; @@ -133,8 +133,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver // invariant of asking for the same deps each build. Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps = env.getValuesOrThrow(toKeys(state.allInputs.getAllInputs(), - action.discoversInputs() ? action.getMandatoryInputs() : null), - MissingInputFileException.class, ActionExecutionException.class); + action.discoversInputs() ? action.getMandatoryInputs() : null), + MissingInputFileException.class, ActionExecutionException.class); if (!sharedActionAlreadyRan && !state.hasArtifactData()) { // Do we actually need to find our metadata? @@ -281,8 +281,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver Map<SkyKey, ValueOrException2<NoSuchPackageException, InconsistentFilesystemException>> values = - env.getValuesOrThrow(depKeys.values(), NoSuchPackageException.class, - InconsistentFilesystemException.class); + env.getValuesOrThrow(depKeys.values(), NoSuchPackageException.class, + InconsistentFilesystemException.class); // Check values even if some are missing so that we can throw an appropriate exception if // needed. @@ -337,7 +337,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver // We got a hit from the action cache -- no need to execute. return new ActionExecutionValue( metadataHandler.getOutputArtifactFileData(), - ImmutableMap.<Artifact, TreeArtifactValue>of(), + metadataHandler.getOutputTreeArtifactData(), metadataHandler.getAdditionalOutputData()); } @@ -446,6 +446,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver private static Map<Artifact, FileArtifactValue> addDiscoveredInputs( Map<Artifact, FileArtifactValue> originalInputData, Collection<Artifact> discoveredInputs, Environment env) { + // We assume nobody would want to discover a TreeArtifact, since TreeArtifacts are precisely + // for undiscoverable contents. Map<Artifact, FileArtifactValue> result = new HashMap<>(originalInputData); Set<SkyKey> keys = new HashSet<>(); for (Artifact artifact : discoveredInputs) { @@ -542,25 +544,30 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver Artifact input = ArtifactValue.artifact(depsEntry.getKey()); try { ArtifactValue value = (ArtifactValue) depsEntry.getValue().get(); - if (populateInputData && value instanceof AggregatingArtifactValue) { - AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value; - for (Pair<Artifact, FileArtifactValue> entry : aggregatingValue.getInputs()) { - inputArtifactData.put(entry.first, entry.second); + if (populateInputData) { + if (value instanceof AggregatingArtifactValue) { + AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value; + for (Pair<Artifact, FileArtifactValue> entry : aggregatingValue.getInputs()) { + inputArtifactData.put(entry.first, entry.second); + } + // We have to cache the "digest" of the aggregating value itself, + // because the action cache checker may want it. + inputArtifactData.put(input, aggregatingValue.getSelfData()); + ImmutableList.Builder<ArtifactFile> expansionBuilder = ImmutableList.builder(); + for (Pair<Artifact, FileArtifactValue> pair : aggregatingValue.getInputs()) { + expansionBuilder.add(pair.first); + } + expandedArtifacts.put(input, expansionBuilder.build()); + } else if (value instanceof TreeArtifactValue) { + TreeArtifactValue setValue = (TreeArtifactValue) value; + expandedArtifacts.put(input, ActionInputHelper.asArtifactFiles( + input, setValue.getChildPaths())); + // Again, we cache the "digest" of the value for cache checking. + inputArtifactData.put(input, setValue.getSelfData()); + } else if (value instanceof FileArtifactValue) { + // TODO(bazel-team): Make sure middleman "virtual" artifact data is properly processed. + inputArtifactData.put(input, (FileArtifactValue) value); } - // We have to cache the "digest" of the aggregating value itself, because the action cache - // checker may want it. - inputArtifactData.put(input, aggregatingValue.getSelfData()); - expandedArtifacts.put(input, - Collections2.transform(aggregatingValue.getInputs(), - new Function<Pair<Artifact, FileArtifactValue>, ArtifactFile>() { - @Override - public ArtifactFile apply(Pair<Artifact, FileArtifactValue> pair) { - return pair.first; - } - })); - } else if (populateInputData && value instanceof FileArtifactValue) { - // TODO(bazel-team): Make sure middleman "virtual" artifact data is properly processed. - inputArtifactData.put(input, (FileArtifactValue) value); } } catch (MissingInputFileException e) { missingCount++; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 3d845f6858..0f2fb63fdc 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -20,12 +20,14 @@ import com.google.common.collect.Maps; import com.google.common.collect.Sets; import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFile; import com.google.devtools.build.lib.actions.cache.Digest; import com.google.devtools.build.lib.actions.cache.DigestUtils; import com.google.devtools.build.lib.actions.cache.Metadata; import com.google.devtools.build.lib.actions.cache.MetadataHandler; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue.TreeArtifactException; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.FileStatus; @@ -321,12 +323,45 @@ public class ActionMetadataHandler implements MetadataHandler { return value; } - Set<ArtifactFile> contents = outputDirectoryListings.get(artifact); - if (contents != null) { - value = constructTreeArtifactValue(contents); + Set<ArtifactFile> registeredContents = outputDirectoryListings.get(artifact); + if (registeredContents != null) { + // Check that our registered outputs matches on-disk outputs. Only perform this check + // when contents were explicitly registered. + // TODO(bazel-team): Provide a way for actions to register empty TreeArtifacts. + + // By the time we're constructing TreeArtifactValues, use of the metadata handler + // should be single threaded and there should be no race condition. + // The current design of ActionMetadataHandler makes this hard to enforce. + Set<PathFragment> paths = null; + try { + paths = TreeArtifactValue.explodeDirectory(artifact); + } catch (TreeArtifactException e) { + throw new IllegalStateException(e); + } + Set<ArtifactFile> diskFiles = ActionInputHelper.asArtifactFiles(artifact, paths); + if (!diskFiles.equals(registeredContents)) { + // There might be more than one error here. We first look for missing output files. + Set<ArtifactFile> missingFiles = Sets.difference(registeredContents, diskFiles); + if (!missingFiles.isEmpty()) { + // Don't throw IOException--getMetadataMaybe() eats them. + // TODO(bazel-team): Report this error in a better way when called by checkOutputs() + // Currently it's hard to report this error without refactoring, since checkOutputs() + // likes to substitute its own error messages upon catching IOException, and falls + // through to unrecoverable error behavior on any other exception. + throw new IllegalStateException("Output file " + missingFiles.iterator().next() + + " was registered, but not present on disk"); + } + + Set<ArtifactFile> extraFiles = Sets.difference(diskFiles, registeredContents); + // extraFiles cannot be empty + throw new IllegalStateException( + "File " + extraFiles.iterator().next().getParentRelativePath() + + ", present in TreeArtifact " + artifact + ", was not registered"); + } + + value = constructTreeArtifactValue(registeredContents); } else { - // Functionality is planned to construct the TreeArtifactValue from disk here. - throw new UnsupportedOperationException(); + value = constructTreeArtifactValueFromFilesystem(artifact); } TreeArtifactValue oldValue = outputTreeArtifactData.putIfAbsent(artifact, value); @@ -361,6 +396,31 @@ public class ActionMetadataHandler implements MetadataHandler { return TreeArtifactValue.create(values); } + private TreeArtifactValue constructTreeArtifactValueFromFilesystem(Artifact artifact) + throws IOException { + Preconditions.checkState(artifact.isTreeArtifact(), artifact); + + if (!artifact.getPath().isDirectory() || artifact.getPath().isSymbolicLink()) { + return TreeArtifactValue.MISSING_TREE_ARTIFACT; + } + + Set<PathFragment> paths = null; + try { + paths = TreeArtifactValue.explodeDirectory(artifact); + } catch (TreeArtifactException e) { + throw new IllegalStateException(e); + } + // If you're reading tree artifacts from disk while outputDirectoryListings are being injected, + // something has gone terribly wrong. + Object previousDirectoryListing = + outputDirectoryListings.put(artifact, + Collections.newSetFromMap(new ConcurrentHashMap<ArtifactFile, Boolean>())); + Preconditions.checkState(previousDirectoryListing == null, + "Race condition while constructing TreArtifactValue: %s, %s", + artifact, previousDirectoryListing); + return constructTreeArtifactValue(ActionInputHelper.asArtifactFiles(artifact, paths)); + } + @Override public void addExpandedTreeOutput(ArtifactFile output) { Preconditions.checkArgument(output.getParent().isTreeArtifact(), diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 39adae3b33..fb90b58bf0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionStatusReporter; import com.google.devtools.build.lib.actions.ActionGraph; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputFileCache; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator; import com.google.devtools.build.lib.actions.ActionMiddlemanEvent; import com.google.devtools.build.lib.actions.ActionStartedEvent; @@ -439,7 +440,8 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto @Override public void expand(Artifact artifact, Collection<? super ArtifactFile> output) { - Preconditions.checkState(artifact.isMiddlemanArtifact(), artifact); + Preconditions.checkState(artifact.isMiddlemanArtifact() || artifact.isTreeArtifact(), + artifact); Collection<ArtifactFile> result = expandedInputs.get(artifact); // Note that result may be null for non-aggregating middlemen. if (result != null) { @@ -610,14 +612,12 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto } statusReporterRef.get().setPreparing(action); - createOutputDirectories(action); - Preconditions.checkState(actionExecutionContext.getMetadataHandler() == metadataHandler, "%s %s", actionExecutionContext.getMetadataHandler(), metadataHandler); prepareScheduleExecuteAndCompleteAction(action, actionExecutionContext, actionStartTime); return new ActionExecutionValue( metadataHandler.getOutputArtifactFileData(), - ImmutableMap.<Artifact, TreeArtifactValue>of(), + metadataHandler.getOutputTreeArtifactData(), metadataHandler.getAdditionalOutputData()); } finally { profiler.completeTask(ProfilerTask.ACTION); @@ -629,7 +629,13 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto try { Set<Path> done = new HashSet<>(); // avoid redundant calls for the same directory. for (Artifact outputFile : action.getOutputs()) { - Path outputDir = outputFile.getPath().getParentDirectory(); + Path outputDir; + if (outputFile.isTreeArtifact()) { + outputDir = outputFile.getPath(); + } else { + outputDir = outputFile.getPath().getParentDirectory(); + } + if (done.add(outputDir)) { try { createDirectoryAndParents(outputDir); @@ -700,6 +706,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto // the action really does produce the outputs. try { action.prepare(context.getExecutor().getExecRoot()); + createOutputDirectories(action); } catch (IOException e) { reportError("failed to delete output files before executing action", e, action, null); } @@ -842,6 +849,35 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto } } + // TODO TODOTIJDSO: maybe just set directories r/o and executable always + private static void setPathReadOnlyAndExecutable(MetadataHandler metadataHandler, + ArtifactFile file) + throws IOException { + Path path = file.getPath(); + if (path.isFile(Symlinks.NOFOLLOW)) { // i.e. regular files only. + // We trust the files created by the execution-engine to be non symlinks with expected + // chmod() settings already applied. + if (!metadataHandler.isInjected(file)) { + path.chmod(0555); // Sets the file read-only and executable. + } + } + } + + private static void setTreeReadOnlyAndExecutable(MetadataHandler metadataHandler, Artifact parent, + PathFragment subpath) throws IOException { + Path path = parent.getPath().getRelative(subpath); + if (path.isDirectory()) { + path.chmod(0555); + for (Path child : path.getDirectoryEntries()) { + setTreeReadOnlyAndExecutable(metadataHandler, parent, + subpath.getChild(child.getBaseName())); + } + } else { + setPathReadOnlyAndExecutable( + metadataHandler, ActionInputHelper.artifactFile(parent, subpath)); + } + } + /** * For each of the action's outputs that is a regular file (not a symbolic * link or directory), make it read-only and executable. @@ -863,14 +899,13 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto Preconditions.checkState(!action.getActionType().isMiddleman()); for (Artifact output : action.getOutputs()) { - Path path = output.getPath(); - if (metadataHandler.isInjected(output)) { - // We trust the files created by the execution-engine to be non symlinks with expected - // chmod() settings already applied. - continue; - } - if (path.isFile(Symlinks.NOFOLLOW)) { // i.e. regular files only. - path.chmod(0555); // Sets the file read-only and executable. + if (output.isTreeArtifact()) { + // Preserve existing behavior: we don't set non-TreeArtifact directories + // read only and executable. However, it's unusual for non-TreeArtifact outputs + // to be directories. + setTreeReadOnlyAndExecutable(metadataHandler, output, PathFragment.EMPTY_FRAGMENT); + } else { + setPathReadOnlyAndExecutable(metadataHandler, output); } } } |