diff options
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib')
4 files changed, 63 insertions, 93 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/InjectedStat.java b/src/main/java/com/google/devtools/build/lib/actions/cache/InjectedStat.java index 858edba762..7ed0e2e406 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/InjectedStat.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/InjectedStat.java @@ -17,6 +17,9 @@ import com.google.devtools.build.lib.vfs.FileStatus; /** * A FileStatus corresponding to a file that is not determined by querying the file system. + * + * <p>Do not use this in combination with MetadataHandler or FileContentsProxy! FileContentsProxy + * may use ctime, which this class does not support. */ public class InjectedStat implements FileStatus { diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java index aa26ed5a12..0874097ef7 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java @@ -24,6 +24,9 @@ import java.io.IOException; * Retrieves {@link Metadata} of {@link Artifact}s, and inserts virtual metadata as well. Some * methods on this interface may only be called after a call to {@link #discardOutputMetadata}. * Calling them before such a call results in an {@link IllegalStateException}. + * + * <p>Note that implementations of this interface call chmod on output files if + * {@link #discardOutputMetadata} has been called. */ public interface MetadataHandler { /** @@ -87,16 +90,6 @@ public interface MetadataHandler { boolean artifactOmitted(Artifact artifact); /** - * @return Whether the artifact's data was injected. - * @throws IOException if implementation tried to stat the Artifact which threw an exception. - * Technically, this means that the artifact could not have been injected, but by throwing - * here we save the caller trying to stat this file on their own and throwing the same - * exception. Implementations are not guaranteed to throw in this case if they are able to - * determine that the artifact is not injected without statting it. - */ - boolean isInjected(Artifact file) throws IOException; - - /** * Discards all known output artifact metadata, presumably because outputs will be modified. May * only be called before any metadata is injected using {@link #injectDigest} or {@link * #markOmitted}; 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 d2e8097e40..99b4030e0f 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 @@ -49,10 +49,14 @@ import javax.annotation.Nullable; /** * Cache provided by an {@link ActionExecutionFunction}, allowing Blaze to obtain data from the - * graph and to inject data (e.g. file digests) back into the graph. + * graph and to inject data (e.g. file digests) back into the graph. The cache can be in one of two + * modes. After construction it acts as a cache for input and output metadata for the purpose of + * checking for an action cache hit. When {@link #discardOutputMetadata} is called, it switches to + * a mode where it calls chmod on output files before statting them. This is done here to ensure + * that the chmod always comes before the stat in order to ensure that the stat is up to date. * - * <p>Data for the action's inputs is injected into this cache on construction, using the graph as - * the source of truth. + * <p>Data for the action's inputs is injected into this cache on construction, using the Skyframe + * graph as the source of truth. * * <p>As well, this cache collects data about the action's output files, which is used in three * ways. First, it is served as requested during action execution, primarily by the {@code @@ -321,6 +325,13 @@ public class ActionMetadataHandler implements MetadataHandler { return value; } + if (executionMode.get()) { + // 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(artifact, PathFragment.EMPTY_FRAGMENT); + } + Set<TreeFileArtifact> registeredContents = outputDirectoryListings.get(artifact); if (registeredContents != null) { // Check that our registered outputs matches on-disk outputs. Only perform this check @@ -441,6 +452,8 @@ public class ActionMetadataHandler implements MetadataHandler { // Assumption: any non-Artifact output is 'virtual' and should be ignored here. if (output instanceof Artifact) { final Artifact artifact = (Artifact) output; + // We have to add the artifact to injectedFiles before calling constructFileValue to avoid + // duplicate chmod calls. Preconditions.checkState(injectedFiles.add(artifact), artifact); FileValue fileValue; try { @@ -522,12 +535,6 @@ public class ActionMetadataHandler implements MetadataHandler { return artifact.getPath().isFile(); } - @Override - public boolean isInjected(Artifact file) { - Preconditions.checkState(executionMode.get()); - return injectedFiles.contains(file); - } - /** @return data for output files that was computed during execution. */ Map<Artifact, FileValue> getOutputArtifactData() { return outputArtifactData; @@ -560,10 +567,20 @@ public class ActionMetadataHandler implements MetadataHandler { return additionalOutputData; } - /** Constructs a new FileValue, saves it, and checks inconsistent data. */ + /** + * Constructs a new FileValue, saves it, and checks inconsistent data. This calls chmod on the + * file if we're in executionMode. + */ private FileValue constructFileValue( Artifact artifact, @Nullable FileStatusWithDigest statNoFollow) throws IOException { + // We first chmod the output files before we construct the FileContentsProxy. The proxy may use + // ctime, which is affected by chmod. + if (executionMode.get()) { + Preconditions.checkState(!artifact.isTreeArtifact()); + setPathReadOnlyAndExecutable(artifact); + } + FileValue value = fileValueFromArtifact(artifact, statNoFollow, getTimestampGranularityMonitor(artifact)); FileValue oldFsValue = outputArtifactData.putIfAbsent(artifact, value); @@ -611,4 +628,31 @@ public class ActionMetadataHandler implements MetadataHandler { } return FileValue.value(rootedPath, fileStateValue, realRootedPath, realFileStateValue); } + + private void setPathReadOnlyAndExecutable(Artifact artifact) throws IOException { + // If the metadata was injected, we assume the mode is set correct and bail out early to avoid + // the additional overhead of resetting it. + if (injectedFiles.contains(artifact)) { + return; + } + Path path = artifact.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. + path.chmod(0555); // Sets the file read-only and executable. + } + } + + private void setTreeReadOnlyAndExecutable(Artifact parent, PathFragment subpath) + throws IOException { + Path path = parent.getPath().getRelative(subpath); + if (path.isDirectory()) { + path.chmod(0555); + for (Path child : path.getDirectoryEntries()) { + setTreeReadOnlyAndExecutable(parent, subpath.getChild(child.getBaseName())); + } + } else { + setPathReadOnlyAndExecutable(ActionInputHelper.treeFileArtifact(parent, subpath)); + } + } } 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 c8436576f1..09072e3fb7 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 @@ -33,7 +33,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.ActionLookupData; import com.google.devtools.build.lib.actions.ActionLookupValue; @@ -810,14 +809,6 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto reportError("not all outputs were created or valid", null, action, outputAlreadyDumped ? null : fileOutErr); } - // Prevent accidental stomping on files. - // This will also throw a FileNotFoundException - // if any of the output files doesn't exist. - try { - setOutputsReadOnlyAndExecutable(action, metadataHandler); - } catch (IOException e) { - reportError("failed to set outputs read-only", e, action, null); - } } finally { profiler.completeTask(ProfilerTask.ACTION_COMPLETE); } @@ -843,68 +834,6 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto } } - private static void setPathReadOnlyAndExecutable(MetadataHandler metadataHandler, - Artifact artifact) - throws IOException { - // If the metadata was injected, we assume the mode is set correct and bail out early to avoid - // the additional overhead of resetting it. - if (metadataHandler.isInjected(artifact)) { - return; - } - Path path = artifact.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. - 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.treeFileArtifact(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. - * - * <p>Making the outputs read-only helps preventing accidental editing of them (e.g. in case of - * generated source code), while making them executable helps running generated files (such as - * generated shell scripts) on the command line. - * - * <p>May execute in a worker thread. - * - * <p>Note: setting these bits maintains transparency regarding the locality of the build; because - * the remote execution engine sets them, they should be set for local builds too. - * - * @throws IOException if an I/O error occurred. - */ - private static void setOutputsReadOnlyAndExecutable( - Action action, MetadataHandler metadataHandler) throws IOException { - 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); - } - } - } - private static void reportMissingOutputFile( Action action, Artifact output, Reporter reporter, boolean isSymlink, IOException exception) { boolean genrule = action.getMnemonic().equals("Genrule"); @@ -943,7 +872,8 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto } /** - * Validates that all action outputs were created or intentionally omitted. + * Validates that all action outputs were created or intentionally omitted. This can result in + * chmod calls on the output files; see {@link ActionMetadataHandler}. * * @return false if some outputs are missing, true - otherwise. */ @@ -953,7 +883,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto // artifactExists has the side effect of potentially adding the artifact to the cache, // therefore we only call it if we know the artifact is indeed not omitted to avoid any // unintended side effects. - if (!(metadataHandler.artifactOmitted(output))) { + if (!metadataHandler.artifactOmitted(output)) { try { metadataHandler.getMetadata(output); } catch (IOException e) { |