diff options
author | Michajlo Matijkiw <michajlo@google.com> | 2016-02-10 16:26:54 +0000 |
---|---|---|
committer | Dmitry Lomov <dslomov@google.com> | 2016-02-11 11:48:34 +0000 |
commit | a0eefb52f529b73c6cb92f0a762853646ea2eae6 (patch) | |
tree | a1fa72ad1cab1cc543edbc4c292784464471585f /src/main/java/com | |
parent | 466873e272d040f466150469ceb172e80a6a67f4 (diff) |
Rollback of commit df03e10f6552566982399b8779fe7bc7a17d75dc.
--
MOS_MIGRATED_REVID=114329043
Diffstat (limited to 'src/main/java/com')
4 files changed, 43 insertions, 161 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 d134f2d6f4..8bceb9acd3 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,7 +18,6 @@ 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; @@ -27,7 +26,6 @@ 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. @@ -174,20 +172,6 @@ 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 d45d282ac3..0a210149d1 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,7 +15,8 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.base.Function; import com.google.common.base.Predicates; -import com.google.common.collect.ImmutableList; +import com.google.common.collect.Collections2; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.common.collect.Sets; @@ -24,7 +25,6 @@ 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(), - metadataHandler.getOutputTreeArtifactData(), + ImmutableMap.<Artifact, TreeArtifactValue>of(), metadataHandler.getAdditionalOutputData()); } @@ -446,8 +446,6 @@ 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) { @@ -544,30 +542,25 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver Artifact input = ArtifactValue.artifact(depsEntry.getKey()); try { ArtifactValue value = (ArtifactValue) depsEntry.getValue().get(); - 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); + if (populateInputData && 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()); + 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 0f2fb63fdc..3d845f6858 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,14 +20,12 @@ 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; @@ -323,45 +321,12 @@ public class ActionMetadataHandler implements MetadataHandler { return value; } - 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); + Set<ArtifactFile> contents = outputDirectoryListings.get(artifact); + if (contents != null) { + value = constructTreeArtifactValue(contents); } else { - value = constructTreeArtifactValueFromFilesystem(artifact); + // Functionality is planned to construct the TreeArtifactValue from disk here. + throw new UnsupportedOperationException(); } TreeArtifactValue oldValue = outputTreeArtifactData.putIfAbsent(artifact, value); @@ -396,31 +361,6 @@ 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 fb90b58bf0..39adae3b33 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,7 +32,6 @@ 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; @@ -440,8 +439,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto @Override public void expand(Artifact artifact, Collection<? super ArtifactFile> output) { - Preconditions.checkState(artifact.isMiddlemanArtifact() || artifact.isTreeArtifact(), - artifact); + Preconditions.checkState(artifact.isMiddlemanArtifact(), artifact); Collection<ArtifactFile> result = expandedInputs.get(artifact); // Note that result may be null for non-aggregating middlemen. if (result != null) { @@ -612,12 +610,14 @@ 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(), - metadataHandler.getOutputTreeArtifactData(), + ImmutableMap.<Artifact, TreeArtifactValue>of(), metadataHandler.getAdditionalOutputData()); } finally { profiler.completeTask(ProfilerTask.ACTION); @@ -629,13 +629,7 @@ 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; - if (outputFile.isTreeArtifact()) { - outputDir = outputFile.getPath(); - } else { - outputDir = outputFile.getPath().getParentDirectory(); - } - + Path outputDir = outputFile.getPath().getParentDirectory(); if (done.add(outputDir)) { try { createDirectoryAndParents(outputDir); @@ -706,7 +700,6 @@ 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); } @@ -849,35 +842,6 @@ 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. @@ -899,13 +863,14 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto Preconditions.checkState(!action.getActionType().isMiddleman()); for (Artifact output : action.getOutputs()) { - 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); + 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. } } } |