[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 <cparsons@google.com>
Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com>
Co-authored-by: keertk <110264242+keertk@users.noreply.github.com>
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 0612e51..a053d76 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 @@
Map<String, String> usedClientEnv = computeUsedClientEnv(action, clientEnv);
Map<String, String> 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<String, String> usedEnvironment = new HashMap<>();
usedEnvironment.putAll(usedClientEnv);
usedEnvironment.putAll(usedExecProperties);
@@ -419,6 +421,7 @@
Action action,
List<Artifact> resolvedCacheArtifacts,
Map<String, String> clientEnv,
+ OutputPermissions outputPermissions,
EventHandler handler,
MetadataHandler metadataHandler,
ArtifactExpander artifactExpander,
@@ -481,6 +484,7 @@
artifactExpander,
actionInputs,
clientEnv,
+ outputPermissions,
remoteDefaultPlatformProperties,
cachedOutputMetadata)) {
if (entry != null) {
@@ -510,6 +514,7 @@
ArtifactExpander artifactExpander,
NestedSet<Artifact> actionInputs,
Map<String, String> clientEnv,
+ OutputPermissions outputPermissions,
Map<String, String> remoteDefaultPlatformProperties,
@Nullable CachedOutputMetadata cachedOutputMetadata)
throws InterruptedException {
@@ -542,7 +547,7 @@
}
Map<String, String> 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 @@
MetadataHandler metadataHandler,
ArtifactExpander artifactExpander,
Map<String, String> clientEnv,
+ OutputPermissions outputPermissions,
Map<String, String> remoteDefaultPlatformProperties)
throws IOException, InterruptedException {
checkState(cacheConfig.enabled(), "cache unexpectedly disabled, action: %s", action);
@@ -595,7 +601,8 @@
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 @@
// 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 @@
Action action,
List<Artifact> resolvedCacheArtifacts,
Map<String, String> clientEnv,
+ OutputPermissions outputPermissions,
EventHandler handler,
MetadataHandler metadataHandler,
ArtifactExpander artifactExpander,
@@ -781,6 +789,7 @@
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 6204e6f..8f91a18 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 @@
* 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 @@
// If null, digest is non-null and the entry is immutable.
private Map<String, FileArtifactValue> mdMap;
private byte[] digest;
- private final byte[] usedClientEnvDigest;
+ private final byte[] actionPropertiesDigest;
private final Map<String, RemoteFileArtifactValue> outputFileMetadata;
private final Map<String, SerializableTreeArtifactValue> outputTreeMetadata;
@@ -160,9 +163,13 @@
public abstract Optional<PathFragment> materializationExecPath();
}
- public Entry(String key, Map<String, String> usedClientEnv, boolean discoversInputs) {
+ public Entry(
+ String key,
+ Map<String, String> 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,
- byte[] usedClientEnvDigest,
+ byte[] actionPropertiesDigest,
@Nullable List<String> files,
byte[] digest,
Map<String, RemoteFileArtifactValue> outputFileMetadata,
Map<String, SerializableTreeArtifactValue> outputTreeMetadata) {
actionKey = key;
- this.usedClientEnvDigest = usedClientEnvDigest;
+ this.actionPropertiesDigest = actionPropertiesDigest;
this.files = files;
this.digest = digest;
mdMap = null;
@@ -185,10 +192,9 @@
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.
*
* <p>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 @@
* (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<String, String> clientEnv) {
+ private static byte[] digestActionProperties(
+ Map<String, String> clientEnv, OutputPermissions outputPermissions) {
byte[] result = EMPTY_CLIENT_ENV_DIGEST;
Fingerprint fp = new Fingerprint();
for (Map.Entry<String, String> entry : clientEnv.entrySet()) {
@@ -204,6 +211,13 @@
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 @@
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<String, String> clientEnv) {
- return Arrays.equals(digestClientEnv(clientEnv), usedClientEnvDigest);
+ /** Determines whether this entry has the same action properties as the one given. */
+ public boolean sameActionProperties(
+ Map<String, String> clientEnv, OutputPermissions outputPermissions) {
+ return Arrays.equals(
+ digestActionProperties(clientEnv, outputPermissions), actionPropertiesDigest);
}
/**
@@ -343,7 +359,7 @@
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 b2d44b8..937b353 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 @@
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<String, RemoteFileArtifactValue> 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 d028ad8..e683417 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 @@
+ "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 @@
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 bcdf604..3c32568 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 @@
private final Reporter reporter;
private final AsyncTaskCache.NoResult<Path> downloadCache = AsyncTaskCache.NoResult.create();
private final TempPathGenerator tempPathGenerator;
+ private final OutputPermissions outputPermissions;
protected final Set<Artifact> outputsAreInputs = Sets.newConcurrentHashSet();
protected final Path execRoot;
@@ -123,11 +125,13 @@
Reporter reporter,
Path execRoot,
TempPathGenerator tempPathGenerator,
- ImmutableList<Pattern> patternsToDownload) {
+ ImmutableList<Pattern> 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 @@
}
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 @@
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 c925b61..43b5ed1 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 @@
"//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 bcda637..fb6f55c 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 @@
Path execRoot,
TempPathGenerator tempPathGenerator,
ImmutableList<Pattern> 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 6f1bfbe..979df0f 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 @@
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 @@
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 4a116c0..874262d 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 @@
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 a9385f9..af848e3 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 @@
ActionInputMap inputArtifactData,
boolean forInputDiscovery,
boolean archivedTreeArtifactsEnabled,
+ OutputPermissions outputPermissions,
ImmutableSet<Artifact> outputs,
XattrProvider xattrProvider,
TimestampGranularityMonitor tsgm,
@@ -104,6 +106,7 @@
inputArtifactData,
forInputDiscovery,
archivedTreeArtifactsEnabled,
+ outputPermissions,
outputs,
xattrProvider,
tsgm,
@@ -117,6 +120,7 @@
private final ActionInputMap inputArtifactData;
private final boolean forInputDiscovery;
private final boolean archivedTreeArtifactsEnabled;
+ private final OutputPermissions outputPermissions;
private final ImmutableMap<PathFragment, FileArtifactValue> filesetMapping;
private final Set<Artifact> omittedOutputs = Sets.newConcurrentHashSet();
@@ -135,6 +139,7 @@
ActionInputMap inputArtifactData,
boolean forInputDiscovery,
boolean archivedTreeArtifactsEnabled,
+ OutputPermissions outputPermissions,
ImmutableSet<Artifact> outputs,
XattrProvider xattrProvider,
TimestampGranularityMonitor tsgm,
@@ -146,6 +151,7 @@
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 @@
ActionMetadataHandler transformAfterInputDiscovery(OutputStore store) {
return new ActionMetadataHandler(
inputArtifactData,
- /*forInputDiscovery=*/ false,
+ /* forInputDiscovery= */ false,
archivedTreeArtifactsEnabled,
+ outputPermissions,
outputs,
xattrProvider,
tsgm,
@@ -287,7 +294,7 @@
// 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 @@
// 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 @@
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 @@
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 342d977..11a3d1d 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 @@
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 @@
action,
resolvedCacheArtifacts,
clientEnv,
+ getOutputPermissions(),
handler,
metadataHandler,
artifactExpander,
@@ -682,6 +692,7 @@
action,
resolvedCacheArtifacts,
clientEnv,
+ getOutputPermissions(),
handler,
metadataHandler,
artifactExpander,
@@ -727,7 +738,13 @@
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 0000000..0bc688d
--- /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 b854342..ee8f3d7 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 b893c2a..f8f1773 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 @@
@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 @@
@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 @@
@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 @@
@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 @@
@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 @@
@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 @@
@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_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, 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 1190919..afed03e 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 @@
"//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 c58c403..be7cb4c 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<Class<? extends OptionsBase>> 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 e82fcfe..fca68db 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 @@
execRoot,
tempPathGenerator,
ImmutableList.of(),
+ OutputPermissions.READONLY,
/* useNewExitCodeForLostInputs= */ false);
}
@@ -91,6 +93,7 @@
execRoot,
tempPathGenerator,
ImmutableList.of(),
+ OutputPermissions.READONLY,
/* useNewExitCodeForLostInputs= */ false);
VirtualActionInput a = ActionsTestUtil.createVirtualActionInput("file1", "hello world");
@@ -119,6 +122,7 @@
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 8d58e33..ebc04c3 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<Path> chmodCalls = Sets.newConcurrentHashSet();
+ private final Map<Path, Integer> 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 @@
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 @@
// 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 @@
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 @@
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 @@
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 @@
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 5d0af9d..6fe78ba 100644
--- a/src/test/shell/bazel/BUILD
+++ b/src/test/shell/bazel/BUILD
@@ -598,6 +598,15 @@
)
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"],
data = [":test-deps"],
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 0000000..43979b8
--- /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 <<EOF
+genrule(
+ name = "foo",
+ srcs = [],
+ outs = ["foo"],
+ cmd = "echo 'foo' > \$@",
+)
+EOF
+
+ 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"
+
+ # 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 <<EOF
+genrule(
+ name = "foo",
+ srcs = [],
+ outs = ["foo"],
+ cmd = "echo 'foo' > \$@",
+)
+EOF
+
+ 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"
+
+ # 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 ff03785..ef92bbb 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 @@
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 @@
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 <<EOF
+genrule(
+ name = "foo",
+ srcs = [],
+ outs = ["foo"],
+ cmd = "echo 'foo' > \$@",
+)
+
+genrule(
+ name = "bar",
+ srcs = [":foo"],
+ outs = ["bar"],
+ cmd = "ls -lL \$(SRCS) > \$@",
+ tags = ["no-remote"],
+)
+EOF
+
+ # no remote execution
+ bazel build \
+ --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"
+
+ # 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.
