From 5afbce52c70cf974eaa4a3bbbc376f398271427d Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Tue, 28 Feb 2023 20:05:32 +0100 Subject: [PATCH] [6.1.0] Flag for writable outputs (experimental) (#17617) This feature is tied to an experimental flag `--experimental_writable_outputs`. When enabled, Bazel will set the permissions of all output files to 0755 instead of 0555. RELNOTES: None. PiperOrigin-RevId: 500786227 Change-Id: I59e15f3fec09c40a052a60b00da209547f10d7fc Co-authored-by: Googler Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com> Co-authored-by: keertk <110264242+keertk@users.noreply.github.com> --- .../build/lib/actions/ActionCacheChecker.java | 21 ++- .../build/lib/actions/cache/ActionCache.java | 50 ++++-- .../cache/CompactPersistentActionCache.java | 2 +- .../lib/analysis/config/CoreOptions.java | 12 ++ .../remote/AbstractActionInputPrefetcher.java | 16 +- .../google/devtools/build/lib/remote/BUILD | 1 + .../lib/remote/RemoteActionInputFetcher.java | 4 +- .../build/lib/remote/RemoteModule.java | 8 + .../lib/skyframe/ActionExecutionFunction.java | 1 + .../lib/skyframe/ActionMetadataHandler.java | 25 ++- .../lib/skyframe/SkyframeActionExecutor.java | 21 ++- .../build/lib/vfs/OutputPermissions.java | 30 ++++ .../lib/actions/ActionCacheCheckerTest.java | 96 ++++++----- .../CompactPersistentActionCacheTest.java | 28 ++- .../google/devtools/build/lib/exec/util/BUILD | 1 + .../lib/exec/util/TestExecutorBuilder.java | 3 +- .../remote/RemoteActionInputFetcherTest.java | 4 + .../skyframe/ActionMetadataHandlerTest.java | 74 ++++++-- src/test/shell/bazel/BUILD | 9 + .../shell/bazel/bazel_permissions_test.sh | 146 ++++++++++++++++ .../remote/build_without_the_bytes_test.sh | 162 ++++++++++++++++++ 21 files changed, 605 insertions(+), 109 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/vfs/OutputPermissions.java create mode 100755 src/test/shell/bazel/bazel_permissions_test.sh diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index 0612e5109bfe8f..a053d76695ff22 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation; +import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.FileNotFoundException; @@ -291,9 +292,10 @@ private static Map computeUsedEnv( Map usedClientEnv = computeUsedClientEnv(action, clientEnv); Map usedExecProperties = computeUsedExecProperties(action, remoteDefaultPlatformProperties); - // Combining the Client environment with the Remote Default Execution Properties, because - // the Miss Reason is not used currently by Bazel, therefore there is no need to distinguish - // between these two cases. This also saves memory used for the Action Cache. + // Combining the Client environment with the Remote Default Execution Properties and Output + // Permissions, because the Miss Reason is not used currently by Bazel, therefore there is no + // need to distinguish between these property types. This also saves memory used for the Action + // Cache. Map usedEnvironment = new HashMap<>(); usedEnvironment.putAll(usedClientEnv); usedEnvironment.putAll(usedExecProperties); @@ -419,6 +421,7 @@ public Token getTokenIfNeedToExecute( Action action, List resolvedCacheArtifacts, Map clientEnv, + OutputPermissions outputPermissions, EventHandler handler, MetadataHandler metadataHandler, ArtifactExpander artifactExpander, @@ -481,6 +484,7 @@ public Token getTokenIfNeedToExecute( artifactExpander, actionInputs, clientEnv, + outputPermissions, remoteDefaultPlatformProperties, cachedOutputMetadata)) { if (entry != null) { @@ -510,6 +514,7 @@ private boolean mustExecute( ArtifactExpander artifactExpander, NestedSet actionInputs, Map clientEnv, + OutputPermissions outputPermissions, Map remoteDefaultPlatformProperties, @Nullable CachedOutputMetadata cachedOutputMetadata) throws InterruptedException { @@ -542,7 +547,7 @@ private boolean mustExecute( } Map usedEnvironment = computeUsedEnv(action, clientEnv, remoteDefaultPlatformProperties); - if (!entry.usedSameClientEnv(usedEnvironment)) { + if (!entry.sameActionProperties(usedEnvironment, outputPermissions)) { reportClientEnv(handler, action, usedEnvironment); actionCache.accountMiss(MissReason.DIFFERENT_ENVIRONMENT); return true; @@ -580,6 +585,7 @@ public void updateActionCache( MetadataHandler metadataHandler, ArtifactExpander artifactExpander, Map clientEnv, + OutputPermissions outputPermissions, Map remoteDefaultPlatformProperties) throws IOException, InterruptedException { checkState(cacheConfig.enabled(), "cache unexpectedly disabled, action: %s", action); @@ -595,7 +601,8 @@ public void updateActionCache( new ActionCache.Entry( action.getKey(actionKeyContext, artifactExpander), usedEnvironment, - action.discoversInputs()); + action.discoversInputs(), + outputPermissions); for (Artifact output : action.getOutputs()) { // Remove old records from the cache if they used different key. String execPath = output.getExecPathString(); @@ -746,7 +753,7 @@ private void checkMiddlemanAction( // Compute the aggregated middleman digest. // Since we never validate action key for middlemen, we should not store // it in the cache entry and just use empty string instead. - entry = new ActionCache.Entry("", ImmutableMap.of(), false); + entry = new ActionCache.Entry("", ImmutableMap.of(), false, OutputPermissions.READONLY); for (Artifact input : action.getInputs().toList()) { entry.addInputFile( input.getExecPath(), getMetadataMaybe(metadataHandler, input), /*saveExecPath=*/ true); @@ -768,6 +775,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit( Action action, List resolvedCacheArtifacts, Map clientEnv, + OutputPermissions outputPermissions, EventHandler handler, MetadataHandler metadataHandler, ArtifactExpander artifactExpander, @@ -781,6 +789,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit( action, resolvedCacheArtifacts, clientEnv, + outputPermissions, handler, metadataHandler, artifactExpander, diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java index 6204e6f06038f1..8f91a182befc2c 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java @@ -33,6 +33,7 @@ import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.DigestUtils; +import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.io.PrintStream; @@ -84,9 +85,11 @@ public interface ActionCache { * will continue to return same result regardless of internal data transformations). */ final class Entry { + private static final byte[] EMPTY_CLIENT_ENV_DIGEST = new byte[0]; + /** Unique instance to represent a corrupted cache entry. */ public static final ActionCache.Entry CORRUPTED = - new ActionCache.Entry(null, ImmutableMap.of(), false); + new ActionCache.Entry(null, ImmutableMap.of(), false, OutputPermissions.READONLY); private final String actionKey; @Nullable @@ -95,7 +98,7 @@ final class Entry { // If null, digest is non-null and the entry is immutable. private Map mdMap; private byte[] digest; - private final byte[] usedClientEnvDigest; + private final byte[] actionPropertiesDigest; private final Map outputFileMetadata; private final Map outputTreeMetadata; @@ -160,9 +163,13 @@ public static Optional createSerializable( public abstract Optional materializationExecPath(); } - public Entry(String key, Map usedClientEnv, boolean discoversInputs) { + public Entry( + String key, + Map usedClientEnv, + boolean discoversInputs, + OutputPermissions outputPermissions) { actionKey = key; - this.usedClientEnvDigest = digestClientEnv(usedClientEnv); + this.actionPropertiesDigest = digestActionProperties(usedClientEnv, outputPermissions); files = discoversInputs ? new ArrayList<>() : null; mdMap = new HashMap<>(); outputFileMetadata = new HashMap<>(); @@ -171,13 +178,13 @@ public Entry(String key, Map usedClientEnv, boolean discoversInp public Entry( String key, - byte[] usedClientEnvDigest, + byte[] actionPropertiesDigest, @Nullable List files, byte[] digest, Map outputFileMetadata, Map outputTreeMetadata) { actionKey = key; - this.usedClientEnvDigest = usedClientEnvDigest; + this.actionPropertiesDigest = actionPropertiesDigest; this.files = files; this.digest = digest; mdMap = null; @@ -185,10 +192,9 @@ public Entry( this.outputTreeMetadata = outputTreeMetadata; } - private static final byte[] EMPTY_CLIENT_ENV_DIGEST = new byte[0]; - /** - * Computes an order-independent digest of a map of environment variables. + * Computes an order-independent digest of action properties. This includes a map of client + * environment variables and the non-default permissions for output artifacts of the action. * *

Note that as discussed in https://github.com/bazelbuild/bazel/issues/15660, using {@link * DigestUtils#xor} to achieve order-independence is questionable in case it is possible that @@ -196,7 +202,8 @@ public Entry( * (due to lossy conversion from UTF-16 to UTF-8). We could instead use a sorted map, however * changing the digest function would cause action cache misses across bazel versions. */ - private static byte[] digestClientEnv(Map clientEnv) { + private static byte[] digestActionProperties( + Map clientEnv, OutputPermissions outputPermissions) { byte[] result = EMPTY_CLIENT_ENV_DIGEST; Fingerprint fp = new Fingerprint(); for (Map.Entry entry : clientEnv.entrySet()) { @@ -204,6 +211,13 @@ private static byte[] digestClientEnv(Map clientEnv) { fp.addString(entry.getValue()); result = DigestUtils.xor(result, fp.digestAndReset()); } + // Add the permissions mode to the digest if it differs from the default. + // This is a bit of a hack to save memory on entries which have the default permissions mode + // and no client env. + if (outputPermissions != OutputPermissions.READONLY) { + fp.addInt(outputPermissions.getPermissionsMode()); + result = DigestUtils.xor(result, fp.digestAndReset()); + } return result; } @@ -288,14 +302,16 @@ public String getActionKey() { return actionKey; } - /** @return the effectively used client environment */ - public byte[] getUsedClientEnvDigest() { - return usedClientEnvDigest; + /** Returns the effectively used client environment. */ + public byte[] getActionPropertiesDigest() { + return actionPropertiesDigest; } - /** Determines whether this entry used the same client environment as the one given. */ - public boolean usedSameClientEnv(Map clientEnv) { - return Arrays.equals(digestClientEnv(clientEnv), usedClientEnvDigest); + /** Determines whether this entry has the same action properties as the one given. */ + public boolean sameActionProperties( + Map clientEnv, OutputPermissions outputPermissions) { + return Arrays.equals( + digestActionProperties(clientEnv, outputPermissions), actionPropertiesDigest); } /** @@ -343,7 +359,7 @@ public String toString() { builder.append(" actionKey = ").append(actionKey).append("\n"); builder .append(" usedClientEnvKey = ") - .append(formatDigest(usedClientEnvDigest)) + .append(formatDigest(actionPropertiesDigest)) .append("\n"); builder.append(" digestKey = "); if (digest == null) { diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java index b2d44b8065e970..937b3538e20afc 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java @@ -571,7 +571,7 @@ private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) thr VarInt.putVarInt(indexer.getOrCreateIndex(file), sink); } - MetadataDigestUtils.write(entry.getUsedClientEnvDigest(), sink); + MetadataDigestUtils.write(entry.getActionPropertiesDigest(), sink); VarInt.putVarInt(entry.getOutputFiles().size(), sink); for (Map.Entry file : entry.getOutputFiles().entrySet()) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index d028ad81e253a9..e683417e7e6ff9 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java @@ -170,6 +170,17 @@ public class CoreOptions extends FragmentOptions implements Cloneable { + "disabled.") public boolean strictFilesets; + // This option is only used during execution. However, it is a required input to the analysis + // phase, as otherwise flipping this flag would not invalidate already-executed actions. + @Option( + name = "experimental_writable_outputs", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION}, + metadataTags = {OptionMetadataTag.EXPERIMENTAL}, + help = "If true, the file permissions of action outputs are set to 0755 instead of 0555") + public boolean experimentalWritableOutputs; + @Option( name = "incompatible_strict_conflict_checks", oldName = "experimental_strict_conflict_checks", @@ -970,6 +981,7 @@ public FragmentOptions getHost() { host.includeRequiredConfigFragmentsProvider = includeRequiredConfigFragmentsProvider; host.debugSelectsAlwaysSucceed = debugSelectsAlwaysSucceed; host.checkTestonlyForOutputFiles = checkTestonlyForOutputFiles; + host.experimentalWritableOutputs = experimentalWritableOutputs; host.strictConflictChecks = strictConflictChecks; // === Runfiles === diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index bcdf604b2f81bc..3c325684ee2ad5 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -48,6 +48,7 @@ import com.google.devtools.build.lib.remote.util.RxUtils.TransferResult; import com.google.devtools.build.lib.remote.util.TempPathGenerator; import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import io.reactivex.rxjava3.core.Completable; @@ -74,6 +75,7 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet private final Reporter reporter; private final AsyncTaskCache.NoResult downloadCache = AsyncTaskCache.NoResult.create(); private final TempPathGenerator tempPathGenerator; + private final OutputPermissions outputPermissions; protected final Set outputsAreInputs = Sets.newConcurrentHashSet(); protected final Path execRoot; @@ -123,11 +125,13 @@ protected AbstractActionInputPrefetcher( Reporter reporter, Path execRoot, TempPathGenerator tempPathGenerator, - ImmutableList patternsToDownload) { + ImmutableList patternsToDownload, + OutputPermissions outputPermissions) { this.reporter = reporter; this.execRoot = execRoot; this.tempPathGenerator = tempPathGenerator; this.patternsToDownload = patternsToDownload; + this.outputPermissions = outputPermissions; } private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) { @@ -352,12 +356,12 @@ private Completable prefetchInputTree( } for (Path dir : dirs) { - // Change permission of all directories of a tree artifact to 0555 (files are + // Change permission of all directories of a tree artifact (files are // changed inside {@code finalizeDownload}) in order to match the behaviour when // the tree artifact is generated locally. In that case, permission of all files - // and directories inside a tree artifact is changed to 0555 within {@code + // and directories inside a tree artifact is changed within {@code // checkOutputs()}. - dir.chmod(0555); + dir.chmod(outputPermissions.getPermissionsMode()); } completed.set(true); @@ -537,9 +541,9 @@ private void finalizeDownload(Context context, Path tmpPath, Path path) throws I parentDir.setWritable(true); } - // The permission of output file is changed to 0555 after action execution. We manually change + // The permission of output file is changed after action execution. We manually change // the permission here for the downloaded file to keep this behaviour consistent. - tmpPath.chmod(0555); + tmpPath.chmod(outputPermissions.getPermissionsMode()); FileSystemUtils.moveFile(tmpPath, path); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index c925b61d36d2dd..43b5ed1bc29570 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -61,6 +61,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", + "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis/platform:platform_utils", "//src/main/java/com/google/devtools/build/lib/authandtls", diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java index bcda6374686676..fb6f55c4916df0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code; +import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import io.reactivex.rxjava3.core.Completable; @@ -65,8 +66,9 @@ class RemoteActionInputFetcher extends AbstractActionInputPrefetcher { Path execRoot, TempPathGenerator tempPathGenerator, ImmutableList patternsToDownload, + OutputPermissions outputPermissions, boolean useNewExitCodeForLostInputs) { - super(reporter, execRoot, tempPathGenerator, patternsToDownload); + super(reporter, execRoot, tempPathGenerator, patternsToDownload, outputPermissions); this.buildRequestId = Preconditions.checkNotNull(buildRequestId); this.commandId = Preconditions.checkNotNull(commandId); this.remoteCache = Preconditions.checkNotNull(remoteCache); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 6f1bfbed7ba473..979df0fab81636 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.authandtls.CallCredentialsProvider; @@ -94,6 +95,7 @@ import com.google.devtools.build.lib.util.io.AsynchronousFileOutputStream; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.OutputService; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.common.options.OptionsBase; @@ -908,6 +910,11 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB RemoteOptions remoteOptions = Preconditions.checkNotNull( env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions"); + CoreOptions coreOptions = env.getOptions().getOptions(CoreOptions.class); + OutputPermissions outputPermissions = + coreOptions.experimentalWritableOutputs + ? OutputPermissions.WRITABLE + : OutputPermissions.READONLY; RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode; if (!remoteOutputsMode.downloadAllOutputs() && actionContextProvider.getRemoteCache() != null) { @@ -921,6 +928,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB env.getExecRoot(), tempPathGenerator, patternsToDownload, + outputPermissions, remoteOptions.useNewExitCodeForLostInputs); env.getEventBus().register(actionInputFetcher); builder.setActionInputPrefetcher(actionInputFetcher); 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 4a116c0fc2b482..874262d0ebc1cf 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 @@ -742,6 +742,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( state.inputArtifactData, action.discoversInputs(), skyframeActionExecutor.useArchivedTreeArtifacts(action), + skyframeActionExecutor.getOutputPermissions(), action.getOutputs(), skyframeActionExecutor.getXattrProvider(), tsgm.get(), 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 a9385f9f81260a..af848e30a71dfe 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 @@ -47,6 +47,7 @@ import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileStatusWithDigest; import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; +import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.RootedPath; @@ -93,6 +94,7 @@ static ActionMetadataHandler create( ActionInputMap inputArtifactData, boolean forInputDiscovery, boolean archivedTreeArtifactsEnabled, + OutputPermissions outputPermissions, ImmutableSet outputs, XattrProvider xattrProvider, TimestampGranularityMonitor tsgm, @@ -104,6 +106,7 @@ static ActionMetadataHandler create( inputArtifactData, forInputDiscovery, archivedTreeArtifactsEnabled, + outputPermissions, outputs, xattrProvider, tsgm, @@ -117,6 +120,7 @@ static ActionMetadataHandler create( private final ActionInputMap inputArtifactData; private final boolean forInputDiscovery; private final boolean archivedTreeArtifactsEnabled; + private final OutputPermissions outputPermissions; private final ImmutableMap filesetMapping; private final Set omittedOutputs = Sets.newConcurrentHashSet(); @@ -135,6 +139,7 @@ private ActionMetadataHandler( ActionInputMap inputArtifactData, boolean forInputDiscovery, boolean archivedTreeArtifactsEnabled, + OutputPermissions outputPermissions, ImmutableSet outputs, XattrProvider xattrProvider, TimestampGranularityMonitor tsgm, @@ -146,6 +151,7 @@ private ActionMetadataHandler( this.inputArtifactData = checkNotNull(inputArtifactData); this.forInputDiscovery = forInputDiscovery; this.archivedTreeArtifactsEnabled = archivedTreeArtifactsEnabled; + this.outputPermissions = outputPermissions; this.outputs = checkNotNull(outputs); this.xattrProvider = xattrProvider; this.tsgm = checkNotNull(tsgm); @@ -170,8 +176,9 @@ private ActionMetadataHandler( ActionMetadataHandler transformAfterInputDiscovery(OutputStore store) { return new ActionMetadataHandler( inputArtifactData, - /*forInputDiscovery=*/ false, + /* forInputDiscovery= */ false, archivedTreeArtifactsEnabled, + outputPermissions, outputs, xattrProvider, tsgm, @@ -287,7 +294,7 @@ public FileArtifactValue getMetadata(ActionInput actionInput) throws IOException // If necessary, we first call chmod the output file. The FileArtifactValue may use a // FileContentsProxy, which is based on ctime (affected by chmod). if (executionMode.get()) { - setPathReadOnlyAndExecutableIfFile(artifactPathResolver.toPath(artifact)); + setPathPermissionsIfFile(artifactPathResolver.toPath(artifact)); } value = constructFileArtifactValueFromFilesystem(artifact); @@ -330,13 +337,13 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa // initialized, so this should hold unless the action itself has deleted the root. if (!treeDir.isDirectory(Symlinks.FOLLOW)) { if (chmod) { - setPathReadOnlyAndExecutableIfFile(treeDir); + setPathPermissionsIfFile(treeDir); } return TreeArtifactValue.MISSING_TREE_ARTIFACT; } if (chmod) { - setPathReadOnlyAndExecutable(treeDir); + setPathPermissions(treeDir); } TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(parent); @@ -345,7 +352,7 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa treeDir, (parentRelativePath, type) -> { if (chmod && type != Dirent.Type.SYMLINK) { - setPathReadOnlyAndExecutable(treeDir.getRelative(parentRelativePath)); + setPathPermissions(treeDir.getRelative(parentRelativePath)); } if (type == Dirent.Type.DIRECTORY) { return; // The final TreeArtifactValue does not contain child directories. @@ -711,13 +718,13 @@ private static FileArtifactValue fileArtifactValueFromStat( fileStateValue.getDigest(), fileStateValue.getContentsProxy(), stat.getSize()); } - private static void setPathReadOnlyAndExecutableIfFile(Path path) throws IOException { + private void setPathPermissionsIfFile(Path path) throws IOException { if (path.isFile(Symlinks.NOFOLLOW)) { - setPathReadOnlyAndExecutable(path); + setPathPermissions(path); } } - private static void setPathReadOnlyAndExecutable(Path path) throws IOException { - path.chmod(0555); + private void setPathPermissions(Path path) throws IOException { + path.chmod(outputPermissions.getPermissionsMode()); } } 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 342d97715e8944..11a3d1dd6c86a3 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 @@ -99,6 +99,7 @@ import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystem.NotASymlinkException; +import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.OutputService; import com.google.devtools.build.lib.vfs.OutputService.ActionFileSystemType; import com.google.devtools.build.lib.vfs.Path; @@ -328,11 +329,19 @@ boolean publishTargetSummaries() { return options.getOptions(BuildEventProtocolOptions.class).publishTargetSummary; } + OutputPermissions getOutputPermissions() { + return options.getOptions(CoreOptions.class).experimentalWritableOutputs + ? OutputPermissions.WRITABLE + : OutputPermissions.READONLY; + } + XattrProvider getXattrProvider() { return syscallCache; } - /** REQUIRES: {@link #actionFileSystemType()} to be not {@code DISABLED}. */ + /** + * REQUIRES: {@link #actionFileSystemType()} to be not {@code DISABLED}. + */ FileSystem createActionFileSystem( String relativeOutputPath, ActionInputMap inputArtifactData, @@ -624,6 +633,7 @@ Token checkActionCache( action, resolvedCacheArtifacts, clientEnv, + getOutputPermissions(), handler, metadataHandler, artifactExpander, @@ -682,6 +692,7 @@ public T getContext(Class type) { action, resolvedCacheArtifacts, clientEnv, + getOutputPermissions(), handler, metadataHandler, artifactExpander, @@ -727,7 +738,13 @@ void updateActionCache( try { actionCacheChecker.updateActionCache( - action, token, metadataHandler, artifactExpander, clientEnv, remoteDefaultProperties); + action, + token, + metadataHandler, + artifactExpander, + clientEnv, + getOutputPermissions(), + remoteDefaultProperties); } catch (IOException e) { // Skyframe has already done all the filesystem access needed for outputs and swallows // IOExceptions for inputs. So an IOException is impossible here. diff --git a/src/main/java/com/google/devtools/build/lib/vfs/OutputPermissions.java b/src/main/java/com/google/devtools/build/lib/vfs/OutputPermissions.java new file mode 100644 index 00000000000000..0bc688dfb213dc --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/vfs/OutputPermissions.java @@ -0,0 +1,30 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.vfs; + +/** File permissions of output file(s). */ +public enum OutputPermissions { + READONLY(0555), + WRITABLE(0755); + + private final int permissionsMode; + + private OutputPermissions(int permissionsMode) { + this.permissionsMode = permissionsMode; + } + + public int getPermissionsMode() { + return permissionsMode; + } +} diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index b8543422e460ee..ee8f3d7c4685df 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -57,6 +57,7 @@ import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; @@ -179,13 +180,14 @@ private void runAction( Token token = cacheChecker.getTokenIfNeedToExecute( action, - /*resolvedCacheArtifacts=*/ null, + /* resolvedCacheArtifacts= */ null, clientEnv, - /*handler=*/ null, + OutputPermissions.READONLY, + /* handler= */ null, metadataHandler, - /*artifactExpander=*/ null, + /* artifactExpander= */ null, platform, - /*isRemoteCacheEnabled=*/ true); + /* loadCachedOutputMetadata= */ true); if (token != null) { // Real action execution would happen here. ActionExecutionContext context = mock(ActionExecutionContext.class); @@ -193,7 +195,13 @@ private void runAction( action.execute(context); cacheChecker.updateActionCache( - action, token, metadataHandler, /*artifactExpander=*/ null, clientEnv, platform); + action, + token, + metadataHandler, + /* artifactExpander= */ null, + clientEnv, + OutputPermissions.READONLY, + platform); } } @@ -437,13 +445,14 @@ public void testDeletedConstantMetadataOutputCausesReexecution() throws Exceptio assertThat( cacheChecker.getTokenIfNeedToExecute( action, - /*resolvedCacheArtifacts=*/ null, - /*clientEnv=*/ ImmutableMap.of(), - /*handler=*/ null, + /* resolvedCacheArtifacts= */ null, + /* clientEnv= */ ImmutableMap.of(), + OutputPermissions.READONLY, + /* handler= */ null, new FakeMetadataHandler(), - /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), - /*isRemoteCacheEnabled=*/ true)) + /* artifactExpander= */ null, + /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), + /* loadCachedOutputMetadata= */ true)) .isNotNull(); } @@ -563,13 +572,14 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { Token token = cacheChecker.getTokenIfNeedToExecute( action, - /*resolvedCacheArtifacts=*/ null, - /*clientEnv=*/ ImmutableMap.of(), - /*handler=*/ null, + /* resolvedCacheArtifacts= */ null, + /* clientEnv= */ ImmutableMap.of(), + OutputPermissions.READONLY, + /* handler= */ null, metadataHandler, - /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), - /*isRemoteCacheEnabled=*/ true); + /* artifactExpander= */ null, + /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), + /* loadCachedOutputMetadata= */ true); assertThat(output.getPath().exists()).isFalse(); assertThat(token).isNull(); @@ -592,13 +602,14 @@ public void saveOutputMetadata_remoteOutputUnavailable_remoteFileMetadataNotLoad Token token = cacheChecker.getTokenIfNeedToExecute( action, - /*resolvedCacheArtifacts=*/ null, - /*clientEnv=*/ ImmutableMap.of(), - /*handler=*/ null, + /* resolvedCacheArtifacts= */ null, + /* clientEnv= */ ImmutableMap.of(), + OutputPermissions.READONLY, + /* handler= */ null, metadataHandler, - /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), - /*isRemoteCacheEnabled=*/ false); + /* artifactExpander= */ null, + /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), + /* loadCachedOutputMetadata= */ false); assertThat(output.getPath().exists()).isFalse(); assertThat(token).isNotNull(); @@ -766,13 +777,14 @@ public void saveOutputMetadata_emptyTreeMetadata_notSaved() throws Exception { Token token = cacheChecker.getTokenIfNeedToExecute( action, - /*resolvedCacheArtifacts=*/ null, - /*clientEnv=*/ ImmutableMap.of(), - /*handler=*/ null, + /* resolvedCacheArtifacts= */ null, + /* clientEnv= */ ImmutableMap.of(), + OutputPermissions.READONLY, + /* handler= */ null, metadataHandler, - /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), - /*isRemoteCacheEnabled=*/ true); + /* artifactExpander= */ null, + /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), + /* loadCachedOutputMetadata= */ true); assertThat(token).isNull(); assertThat(output.getPath().exists()).isFalse(); @@ -864,13 +876,14 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Ex Token token = cacheChecker.getTokenIfNeedToExecute( action, - /*resolvedCacheArtifacts=*/ null, - /*clientEnv=*/ ImmutableMap.of(), - /*handler=*/ null, + /* resolvedCacheArtifacts= */ null, + /* clientEnv= */ ImmutableMap.of(), + OutputPermissions.READONLY, + /* handler= */ null, metadataHandler, - /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), - /*isRemoteCacheEnabled=*/ true); + /* artifactExpander= */ null, + /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), + /* loadCachedOutputMetadata= */ true); TreeArtifactValue expectedMetadata = createTreeMetadata( @@ -1025,13 +1038,14 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached( Token token = cacheChecker.getTokenIfNeedToExecute( action, - /*resolvedCacheArtifacts=*/ null, - /*clientEnv=*/ ImmutableMap.of(), - /*handler=*/ null, + /* resolvedCacheArtifacts= */ null, + /* clientEnv= */ ImmutableMap.of(), + OutputPermissions.READONLY, + /* handler= */ null, metadataHandler, - /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), - /*isRemoteCacheEnabled=*/ true); + /* artifactExpander= */ null, + /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), + /* loadCachedOutputMetadata= */ true); assertThat(token).isNull(); assertStatistics(1, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); diff --git a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java index b893c2aef538b7..f8f17739d72ce0 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; @@ -160,7 +161,8 @@ public void testIncrementalSave() throws IOException { @SuppressWarnings("ReturnValueIgnored") @Test public void testEntryToStringIsIdempotent() { - ActionCache.Entry entry = new ActionCache.Entry("actionKey", ImmutableMap.of(), false); + ActionCache.Entry entry = + new ActionCache.Entry("actionKey", ImmutableMap.of(), false, OutputPermissions.READONLY); entry.toString(); entry.addInputFile( PathFragment.create("foo/bar"), FileArtifactValue.createForDirectoryWithMtime(1234)); @@ -239,7 +241,8 @@ private TreeArtifactValue createTreeMetadata( @Test public void putAndGet_savesRemoteFileMetadata() { String key = "key"; - ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + ActionCache.Entry entry = + new ActionCache.Entry(key, ImmutableMap.of(), false, OutputPermissions.READONLY); Artifact artifact = ActionsTestUtil.DUMMY_ARTIFACT; RemoteFileArtifactValue metadata = createRemoteMetadata(artifact, "content"); entry.addOutputFile(artifact, metadata, /*saveFileMetadata=*/ true); @@ -253,7 +256,8 @@ public void putAndGet_savesRemoteFileMetadata() { @Test public void putAndGet_savesRemoteFileMetadata_withmaterializationExecPath() { String key = "key"; - ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + ActionCache.Entry entry = + new ActionCache.Entry(key, ImmutableMap.of(), false, OutputPermissions.READONLY); Artifact artifact = ActionsTestUtil.DUMMY_ARTIFACT; RemoteFileArtifactValue metadata = createRemoteMetadata(artifact, "content", PathFragment.create("/execroot/some/path")); @@ -268,7 +272,8 @@ public void putAndGet_savesRemoteFileMetadata_withmaterializationExecPath() { @Test public void putAndGet_ignoresLocalFileMetadata() throws IOException { String key = "key"; - ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + ActionCache.Entry entry = + new ActionCache.Entry(key, ImmutableMap.of(), false, OutputPermissions.READONLY); Artifact artifact = ActionsTestUtil.DUMMY_ARTIFACT; FileArtifactValue metadata = createLocalMetadata(artifact, "content"); entry.addOutputFile(artifact, metadata, /*saveFileMetadata=*/ true); @@ -282,7 +287,8 @@ public void putAndGet_ignoresLocalFileMetadata() throws IOException { @Test public void putAndGet_treeMetadata_onlySavesRemoteFileMetadata() throws IOException { String key = "key"; - ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + ActionCache.Entry entry = + new ActionCache.Entry(key, ImmutableMap.of(), false, OutputPermissions.READONLY); SpecialArtifact artifact = ActionsTestUtil.createTreeArtifactWithGeneratingAction( artifactRoot, PathFragment.create("bin/dummy")); @@ -323,7 +329,8 @@ public void putAndGet_treeMetadata_onlySavesRemoteFileMetadata() throws IOExcept @Test public void putAndGet_treeMetadata_savesRemoteArchivedArtifact() { String key = "key"; - ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + ActionCache.Entry entry = + new ActionCache.Entry(key, ImmutableMap.of(), false, OutputPermissions.READONLY); SpecialArtifact artifact = ActionsTestUtil.createTreeArtifactWithGeneratingAction( artifactRoot, PathFragment.create("bin/dummy")); @@ -349,7 +356,8 @@ public void putAndGet_treeMetadata_savesRemoteArchivedArtifact() { @Test public void putAndGet_treeMetadata_ignoresLocalArchivedArtifact() throws IOException { String key = "key"; - ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + ActionCache.Entry entry = + new ActionCache.Entry(key, ImmutableMap.of(), false, OutputPermissions.READONLY); SpecialArtifact artifact = ActionsTestUtil.createTreeArtifactWithGeneratingAction( artifactRoot, PathFragment.create("bin/dummy")); @@ -373,7 +381,8 @@ public void putAndGet_treeMetadata_ignoresLocalArchivedArtifact() throws IOExcep public void putAndGet_treeMetadata_savesMaterializationExecPath() { String key = "key"; PathFragment materializationExecPath = PathFragment.create("/execroot/some/path"); - ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + ActionCache.Entry entry = + new ActionCache.Entry(key, ImmutableMap.of(), false, OutputPermissions.READONLY); SpecialArtifact artifact = ActionsTestUtil.createTreeArtifactWithGeneratingAction( artifactRoot, PathFragment.create("bin/dummy")); @@ -424,7 +433,8 @@ private void putKey(String key, boolean discoversInputs) { private void putKey(String key, ActionCache ac, boolean discoversInputs) { ActionCache.Entry entry = - new ActionCache.Entry(key, ImmutableMap.of("k", "v"), discoversInputs); + new ActionCache.Entry( + key, ImmutableMap.of("k", "v"), discoversInputs, OutputPermissions.READONLY); entry.getFileDigest(); ac.put(key, entry); } diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/BUILD b/src/test/java/com/google/devtools/build/lib/exec/util/BUILD index 1190919374363b..afed03e2ed09c0 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/exec/util/BUILD @@ -30,6 +30,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:actions/template_expansion_action", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", + "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options", "//src/main/java/com/google/devtools/build/lib/analysis/platform", "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java b/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java index c58c403a8e97cd..be7cb4cbdc7897 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java @@ -22,6 +22,7 @@ import com.google.devtools.build.lib.analysis.actions.LocalTemplateExpansionStrategy; import com.google.devtools.build.lib.analysis.actions.SymlinkTreeActionContext; import com.google.devtools.build.lib.analysis.actions.TemplateExpansionContext; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.bugreport.BugReporter; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.events.Reporter; @@ -49,7 +50,7 @@ */ public class TestExecutorBuilder { public static final ImmutableList> DEFAULT_OPTIONS = - ImmutableList.of(ExecutionOptions.class, CommonCommandOptions.class); + ImmutableList.of(ExecutionOptions.class, CommonCommandOptions.class, CoreOptions.class); private final FileSystem fileSystem; private final Path execRoot; private Reporter reporter = new Reporter(new EventBus()); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java index e82fcfede54211..fca68db8b6f160 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java @@ -35,6 +35,7 @@ import com.google.devtools.build.lib.remote.util.InMemoryCacheClient; import com.google.devtools.build.lib.remote.util.StaticMetadataProvider; import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.common.options.Options; @@ -74,6 +75,7 @@ protected AbstractActionInputPrefetcher createPrefetcher(Map c execRoot, tempPathGenerator, ImmutableList.of(), + OutputPermissions.READONLY, /* useNewExitCodeForLostInputs= */ false); } @@ -91,6 +93,7 @@ public void testStagingVirtualActionInput() throws Exception { execRoot, tempPathGenerator, ImmutableList.of(), + OutputPermissions.READONLY, /* useNewExitCodeForLostInputs= */ false); VirtualActionInput a = ActionsTestUtil.createVirtualActionInput("file1", "hello world"); @@ -119,6 +122,7 @@ public void testStagingEmptyVirtualActionInput() throws Exception { execRoot, tempPathGenerator, ImmutableList.of(), + OutputPermissions.READONLY, /* useNewExitCodeForLostInputs= */ false); // act diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java index 8d58e33016e7b1..ebc04c3ba3149c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java @@ -20,7 +20,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; +import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionInputMap; @@ -42,6 +42,7 @@ import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.DigestUtils; +import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; @@ -49,7 +50,7 @@ import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.FileNotFoundException; import java.io.IOException; -import java.util.Set; +import java.util.Map; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -59,18 +60,19 @@ @RunWith(JUnit4.class) public final class ActionMetadataHandlerTest { - private final Set chmodCalls = Sets.newConcurrentHashSet(); + private final Map chmodCalls = Maps.newConcurrentMap(); private final Scratch scratch = new Scratch( new InMemoryFileSystem(DigestHashFunction.SHA256) { @Override - public void chmod(PathFragment path, int mode) throws IOException { - assertThat(mode).isEqualTo(0555); // Read only and executable. - if (!chmodCalls.add(getPath(path))) { + public void chmod(PathFragment pathFragment, int mode) throws IOException { + Path path = getPath(pathFragment); + if (chmodCalls.containsKey(path)) { fail("chmod called on " + path + " twice"); } - super.chmod(path, mode); + chmodCalls.put(path, mode); + super.chmod(pathFragment, mode); } }); @@ -95,14 +97,15 @@ private ActionMetadataHandler createHandler( return ActionMetadataHandler.create( inputMap, forInputDiscovery, - /*archivedTreeArtifactsEnabled=*/ false, + /* archivedTreeArtifactsEnabled= */ false, + OutputPermissions.READONLY, outputs, SyscallCache.NO_CACHE, tsgm, ArtifactPathResolver.IDENTITY, execRoot.asFragment(), derivedPathPrefix, - /*expandedFilesets=*/ ImmutableMap.of()); + /* expandedFilesets= */ ImmutableMap.of()); } @Test @@ -293,7 +296,7 @@ public void resettingOutputs() throws Exception { // The handler doesn't have any info. It'll stat the file and discover that it's 10 bytes long. assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(10); - assertThat(chmodCalls).containsExactly(outputPath); + assertThat(chmodCalls).containsExactly(outputPath, 0555); // Inject a remote file of size 42. handler.injectFile( @@ -304,7 +307,7 @@ public void resettingOutputs() throws Exception { handler.resetOutputs(ImmutableList.of(artifact)); chmodCalls.clear(); // Permit a second chmod call for the artifact. assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(10); - assertThat(chmodCalls).containsExactly(outputPath); + assertThat(chmodCalls).containsExactly(outputPath, 0555); } @Test @@ -438,9 +441,10 @@ public void getMetadataFromFilesetMapping() throws Exception { ActionMetadataHandler handler = ActionMetadataHandler.create( new ActionInputMap(0), - /*forInputDiscovery=*/ false, - /*archivedTreeArtifactsEnabled=*/ false, - /*outputs=*/ ImmutableSet.of(), + /* forInputDiscovery= */ false, + /* archivedTreeArtifactsEnabled= */ false, + OutputPermissions.READONLY, + /* outputs= */ ImmutableSet.of(), SyscallCache.NO_CACHE, tsgm, ArtifactPathResolver.IDENTITY, @@ -538,7 +542,38 @@ public void outputArtifactNotPreviouslyInjectedInExecutionMode() throws Exceptio assertThat(metadata.getDigest()).isEqualTo(outputPath.getDigest()); assertThat(handler.getOutputStore().getAllArtifactData()).containsExactly(output, metadata); assertThat(handler.getOutputStore().getAllTreeArtifactData()).isEmpty(); - assertThat(chmodCalls).containsExactly(outputPath); + assertThat(chmodCalls).containsExactly(outputPath, 0555); + } + + @Test + public void outputArtifactNotPreviouslyInjectedInExecutionMode_writablePermissions() + throws Exception { + Artifact output = + ActionsTestUtil.createArtifactWithRootRelativePath( + outputRoot, PathFragment.create("dir/file.out")); + Path outputPath = scratch.file(output.getPath().getPathString(), "contents"); + ActionMetadataHandler handler = + ActionMetadataHandler.create( + new ActionInputMap(0), + /* forInputDiscovery= */ false, + /* archivedTreeArtifactsEnabled= */ false, + OutputPermissions.WRITABLE, + /* outputs= */ ImmutableSet.of(output), + SyscallCache.NO_CACHE, + tsgm, + ArtifactPathResolver.IDENTITY, + execRoot.asFragment(), + derivedPathPrefix, + /* expandedFilesets= */ ImmutableMap.of()); + handler.prepareForActionExecution(); + + FileArtifactValue metadata = handler.getMetadata(output); + + assertThat(metadata.getDigest()).isEqualTo(outputPath.getDigest()); + assertThat(handler.getOutputStore().getAllArtifactData()).containsExactly(output, metadata); + assertThat(handler.getOutputStore().getAllTreeArtifactData()).isEmpty(); + // Permissions preserved in handler, so chmod calls should be empty. + assertThat(chmodCalls).containsExactly(outputPath, 0755); } @Test @@ -569,7 +604,14 @@ public void outputTreeArtifactNotPreviouslyInjectedInExecutionMode() throws Exce assertThat(handler.getOutputStore().getAllArtifactData()).isEmpty(); assertThat(chmodCalls) .containsExactly( - treeArtifact.getPath(), child1Path, child2Path, child2Path.getParentDirectory()); + treeArtifact.getPath(), + 0555, + child1Path, + 0555, + child2Path, + 0555, + child2Path.getParentDirectory(), + 0555); } @Test diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index 5d0af9dbfb6953..6fe78ba0dc38ed 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -597,6 +597,15 @@ sh_test( ], ) +sh_test( + name = "bazel_permissions_test", + size = "small", + srcs = ["bazel_permissions_test.sh"], + data = [":test-deps"], + tags = ["no_windows"], + deps = ["@bazel_tools//tools/bash/runfiles"], +) + sh_test( name = "bazel_execute_testlog", srcs = ["bazel_execute_testlog.sh"], diff --git a/src/test/shell/bazel/bazel_permissions_test.sh b/src/test/shell/bazel/bazel_permissions_test.sh new file mode 100755 index 00000000000000..43979b818bbde4 --- /dev/null +++ b/src/test/shell/bazel/bazel_permissions_test.sh @@ -0,0 +1,146 @@ +#!/bin/bash +# +# Copyright 2022 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -euo pipefail + +# --- begin runfiles.bash initialization --- +if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + if [[ -f "$0.runfiles_manifest" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest" + elif [[ -f "$0.runfiles/MANIFEST" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST" + elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + export RUNFILES_DIR="$0.runfiles" + fi +fi +if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash" +elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \ + "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)" +else + echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash" + exit 1 +fi +# --- end runfiles.bash initialization --- + +source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \ + || { echo "integration_test_setup.sh not found!" >&2; exit 1; } + +function test_output_readonly() { + # Test that permission of output files are 0555 if --experimental_writable_outputs is not set. + mkdir -p a + cat > a/BUILD <& $TEST_log || fail "Failed to build" + + ls -l bazel-bin/a/foo >& $TEST_log + expect_log "-r-xr-xr-x" + + bazel clean >& $TEST_log || fail "Failed to clean" + + # Verify that changing the value of --experimental_writable_outputs results + # in an update of the output permissions (invalidation of the action cache, etc) + bazel build \ + --experimental_writable_outputs \ + //a:foo >& $TEST_log || fail "Failed to build" + + ls -l bazel-bin/a/foo >& $TEST_log + expect_log "-rwxr-xr-x" + + bazel clean >& $TEST_log || fail "Failed to clean" +} + +function test_output_writable() { + # Test that permission of output files are 0755 if --experimental_writable_outputs is set. + mkdir -p a + cat > a/BUILD <& $TEST_log || fail "Failed to build" + + ls -l bazel-bin/a/foo >& $TEST_log + expect_log "-rwxr-xr-x" + + bazel clean >& $TEST_log || fail "Failed to clean" + + # Verify that changing the value of --experimental_writable_outputs results + # in an update of the output permissions (invalidation of the action cache, etc) + bazel build \ + //a:foo >& $TEST_log || fail "Failed to build" + + ls -l bazel-bin/a/foo >& $TEST_log + expect_log "-r-xr-xr-x" + + bazel clean >& $TEST_log || fail "Failed to clean" +} + +function test_create_tree_artifact_outputs_permissions() { + mkdir -p pkg + cat > pkg/def.bzl <<'EOF' +def _r(ctx): + d = ctx.actions.declare_directory("%s_dir" % ctx.label.name) + ctx.actions.run_shell( + outputs = [d], + command = "cd %s && touch x && touch y" % d.path, + ) + return [DefaultInfo(files = depset([d]))] + +r = rule(implementation = _r) +EOF + +cat > pkg/BUILD <<'EOF' +load(":def.bzl", "r") + +r(name = "a") +EOF + + bazel build pkg:a &>$TEST_log || fail "expected build to succeed" + + ls -l bazel-bin/pkg >& $TEST_log + expect_log "dr-xr-xr-x" + ls -l bazel-bin/pkg/a_dir/x >& $TEST_log + expect_log "-r-xr-xr-x" + ls -l bazel-bin/pkg/a_dir/y >& $TEST_log + expect_log "-r-xr-xr-x" + + bazel build pkg:a --experimental_writable_outputs &>$TEST_log || fail "expected build to succeed" + + ls -l bazel-bin/pkg >& $TEST_log + expect_log "drwxr-xr-x" + ls -l bazel-bin/pkg/a_dir/x >& $TEST_log + expect_log "-rwxr-xr-x" + ls -l bazel-bin/pkg/a_dir/y >& $TEST_log + expect_log "-rwxr-xr-x" +} + +run_suite "bazel file permissions tests" diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh index ff03785deb2d06..ef92bbb04b9a75 100755 --- a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh +++ b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh @@ -951,6 +951,97 @@ EOF expect_log "-r-xr-xr-x" } +function test_prefetcher_change_permission_writable_outputs() { + # test that prefetcher change permission for downloaded files and directories to 0755 if + # --experimental_writable_outputs is set. + touch WORKSPACE + + cat > rules.bzl <<'EOF' +def _gen_output_dir_impl(ctx): + output_dir = ctx.actions.declare_directory(ctx.attr.outdir) + ctx.actions.run_shell( + outputs = [output_dir], + inputs = [], + command = """ + mkdir -p $1/sub + echo "Shuffle, duffle, muzzle, muff" > $1/sub/bar + """, + arguments = [output_dir.path], + ) + return DefaultInfo(files = depset(direct = [output_dir])) + +gen_output_dir = rule( + implementation = _gen_output_dir_impl, + attrs = { + "outdir": attr.string(mandatory = True), + }, +) +EOF + + cat > BUILD <<'EOF' +load("rules.bzl", "gen_output_dir") + +genrule( + name = "input-file", + srcs = [], + outs = ["file"], + cmd = "echo 'input' > $@", +) + +gen_output_dir( + name = "input-tree", + outdir = "tree", +) + +genrule( + name = "test", + srcs = [":input-file", ":input-tree"], + outs = ["out"], + cmd = "touch $@", + tags = ["no-remote"], +) +EOF + + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + --experimental_writable_outputs \ + //:test >& $TEST_log \ + || fail "Failed to build" + + ls -l bazel-bin/file >& $TEST_log + expect_log "-rwxr-xr-x" + + ls -ld bazel-bin/tree >& $TEST_log + expect_log "drwxr-xr-x" + + ls -ld bazel-bin/tree/sub >& $TEST_log + expect_log "drwxr-xr-x" + + ls -l bazel-bin/tree/sub/bar >& $TEST_log + expect_log "-rwxr-xr-x" + + # Rebuild without the flag and verify that permissions in the + # outputs have changed. (Verifies that outputs aren't cached) + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + //:test >& $TEST_log \ + || fail "Failed to build" + + ls -l bazel-bin/file >& $TEST_log + expect_log "-r-xr-xr-x" + + ls -ld bazel-bin/tree >& $TEST_log + expect_log "dr-xr-xr-x" + + ls -ld bazel-bin/tree/sub >& $TEST_log + expect_log "dr-xr-xr-x" + + ls -l bazel-bin/tree/sub/bar >& $TEST_log + expect_log "-r-xr-xr-x" +} + function test_remote_cache_intermediate_outputs_toplevel() { # test that remote cache is hit when intermediate output is not executable in remote download toplevel mode touch WORKSPACE @@ -1192,6 +1283,77 @@ EOF expect_log "-r-xr-xr-x" } +function test_output_file_permission() { + # Test that permission of output files are always 0755 if --experimental_writable_outputs is set. + + mkdir -p a + cat > a/BUILD <& $TEST_log || fail "Failed to build" + + ls -l bazel-bin/a/bar >& $TEST_log + expect_log "-rwxr-xr-x" + + ls -l bazel-bin/a/foo >& $TEST_log + expect_log "-rwxr-xr-x" + + cat bazel-bin/a/bar >& $TEST_log + expect_log "-rwxr-xr-x" + + bazel clean >& $TEST_log || fail "Failed to clean" + + # normal remote execution + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_writable_outputs \ + //a:bar >& $TEST_log || fail "Failed to build" + + ls -l bazel-bin/a/bar >& $TEST_log + expect_log "-rwxr-xr-x" + + ls -l bazel-bin/a/foo >& $TEST_log + expect_log "-rwxr-xr-x" + + cat bazel-bin/a/bar >& $TEST_log + expect_log "-rwxr-xr-x" + + bazel clean >& $TEST_log || fail "Failed to clean" + + # build without bytes + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + --experimental_writable_outputs \ + //a:bar >& $TEST_log || fail "Failed to build" + + ls -l bazel-bin/a/bar >& $TEST_log + expect_log "-rwxr-xr-x" + + ls -l bazel-bin/a/foo >& $TEST_log + expect_log "-rwxr-xr-x" + + cat bazel-bin/a/bar >& $TEST_log + expect_log "-rwxr-xr-x" +} + function test_download_toplevel_when_turn_remote_cache_off() { # Test that BwtB doesn't cause build failure if remote cache is disabled in a following build. # See https://github.com/bazelbuild/bazel/issues/13882.