Skip to content

Commit 5afbce5

Browse files
tjgqc-parsonsShreeM01keertk
authored
[6.1.0] Flag for writable outputs (experimental) (bazelbuild#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 <[email protected]> Co-authored-by: kshyanashree <[email protected]> Co-authored-by: keertk <[email protected]>
1 parent 034a281 commit 5afbce5

21 files changed

+605
-109
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import com.google.devtools.build.lib.events.EventKind;
4242
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
4343
import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation;
44+
import com.google.devtools.build.lib.vfs.OutputPermissions;
4445
import com.google.devtools.build.lib.vfs.Path;
4546
import com.google.devtools.build.lib.vfs.PathFragment;
4647
import java.io.FileNotFoundException;
@@ -291,9 +292,10 @@ private static Map<String, String> computeUsedEnv(
291292
Map<String, String> usedClientEnv = computeUsedClientEnv(action, clientEnv);
292293
Map<String, String> usedExecProperties =
293294
computeUsedExecProperties(action, remoteDefaultPlatformProperties);
294-
// Combining the Client environment with the Remote Default Execution Properties, because
295-
// the Miss Reason is not used currently by Bazel, therefore there is no need to distinguish
296-
// between these two cases. This also saves memory used for the Action Cache.
295+
// Combining the Client environment with the Remote Default Execution Properties and Output
296+
// Permissions, because the Miss Reason is not used currently by Bazel, therefore there is no
297+
// need to distinguish between these property types. This also saves memory used for the Action
298+
// Cache.
297299
Map<String, String> usedEnvironment = new HashMap<>();
298300
usedEnvironment.putAll(usedClientEnv);
299301
usedEnvironment.putAll(usedExecProperties);
@@ -419,6 +421,7 @@ public Token getTokenIfNeedToExecute(
419421
Action action,
420422
List<Artifact> resolvedCacheArtifacts,
421423
Map<String, String> clientEnv,
424+
OutputPermissions outputPermissions,
422425
EventHandler handler,
423426
MetadataHandler metadataHandler,
424427
ArtifactExpander artifactExpander,
@@ -481,6 +484,7 @@ public Token getTokenIfNeedToExecute(
481484
artifactExpander,
482485
actionInputs,
483486
clientEnv,
487+
outputPermissions,
484488
remoteDefaultPlatformProperties,
485489
cachedOutputMetadata)) {
486490
if (entry != null) {
@@ -510,6 +514,7 @@ private boolean mustExecute(
510514
ArtifactExpander artifactExpander,
511515
NestedSet<Artifact> actionInputs,
512516
Map<String, String> clientEnv,
517+
OutputPermissions outputPermissions,
513518
Map<String, String> remoteDefaultPlatformProperties,
514519
@Nullable CachedOutputMetadata cachedOutputMetadata)
515520
throws InterruptedException {
@@ -542,7 +547,7 @@ private boolean mustExecute(
542547
}
543548
Map<String, String> usedEnvironment =
544549
computeUsedEnv(action, clientEnv, remoteDefaultPlatformProperties);
545-
if (!entry.usedSameClientEnv(usedEnvironment)) {
550+
if (!entry.sameActionProperties(usedEnvironment, outputPermissions)) {
546551
reportClientEnv(handler, action, usedEnvironment);
547552
actionCache.accountMiss(MissReason.DIFFERENT_ENVIRONMENT);
548553
return true;
@@ -580,6 +585,7 @@ public void updateActionCache(
580585
MetadataHandler metadataHandler,
581586
ArtifactExpander artifactExpander,
582587
Map<String, String> clientEnv,
588+
OutputPermissions outputPermissions,
583589
Map<String, String> remoteDefaultPlatformProperties)
584590
throws IOException, InterruptedException {
585591
checkState(cacheConfig.enabled(), "cache unexpectedly disabled, action: %s", action);
@@ -595,7 +601,8 @@ public void updateActionCache(
595601
new ActionCache.Entry(
596602
action.getKey(actionKeyContext, artifactExpander),
597603
usedEnvironment,
598-
action.discoversInputs());
604+
action.discoversInputs(),
605+
outputPermissions);
599606
for (Artifact output : action.getOutputs()) {
600607
// Remove old records from the cache if they used different key.
601608
String execPath = output.getExecPathString();
@@ -746,7 +753,7 @@ private void checkMiddlemanAction(
746753
// Compute the aggregated middleman digest.
747754
// Since we never validate action key for middlemen, we should not store
748755
// it in the cache entry and just use empty string instead.
749-
entry = new ActionCache.Entry("", ImmutableMap.of(), false);
756+
entry = new ActionCache.Entry("", ImmutableMap.of(), false, OutputPermissions.READONLY);
750757
for (Artifact input : action.getInputs().toList()) {
751758
entry.addInputFile(
752759
input.getExecPath(), getMetadataMaybe(metadataHandler, input), /*saveExecPath=*/ true);
@@ -768,6 +775,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
768775
Action action,
769776
List<Artifact> resolvedCacheArtifacts,
770777
Map<String, String> clientEnv,
778+
OutputPermissions outputPermissions,
771779
EventHandler handler,
772780
MetadataHandler metadataHandler,
773781
ArtifactExpander artifactExpander,
@@ -781,6 +789,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
781789
action,
782790
resolvedCacheArtifacts,
783791
clientEnv,
792+
outputPermissions,
784793
handler,
785794
metadataHandler,
786795
artifactExpander,

‎src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java‎

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
3434
import com.google.devtools.build.lib.util.Fingerprint;
3535
import com.google.devtools.build.lib.vfs.DigestUtils;
36+
import com.google.devtools.build.lib.vfs.OutputPermissions;
3637
import com.google.devtools.build.lib.vfs.PathFragment;
3738
import java.io.IOException;
3839
import java.io.PrintStream;
@@ -84,9 +85,11 @@ public interface ActionCache {
8485
* will continue to return same result regardless of internal data transformations).
8586
*/
8687
final class Entry {
88+
private static final byte[] EMPTY_CLIENT_ENV_DIGEST = new byte[0];
89+
8790
/** Unique instance to represent a corrupted cache entry. */
8891
public static final ActionCache.Entry CORRUPTED =
89-
new ActionCache.Entry(null, ImmutableMap.of(), false);
92+
new ActionCache.Entry(null, ImmutableMap.of(), false, OutputPermissions.READONLY);
9093

9194
private final String actionKey;
9295
@Nullable
@@ -95,7 +98,7 @@ final class Entry {
9598
// If null, digest is non-null and the entry is immutable.
9699
private Map<String, FileArtifactValue> mdMap;
97100
private byte[] digest;
98-
private final byte[] usedClientEnvDigest;
101+
private final byte[] actionPropertiesDigest;
99102
private final Map<String, RemoteFileArtifactValue> outputFileMetadata;
100103
private final Map<String, SerializableTreeArtifactValue> outputTreeMetadata;
101104

@@ -160,9 +163,13 @@ public static Optional<SerializableTreeArtifactValue> createSerializable(
160163
public abstract Optional<PathFragment> materializationExecPath();
161164
}
162165

163-
public Entry(String key, Map<String, String> usedClientEnv, boolean discoversInputs) {
166+
public Entry(
167+
String key,
168+
Map<String, String> usedClientEnv,
169+
boolean discoversInputs,
170+
OutputPermissions outputPermissions) {
164171
actionKey = key;
165-
this.usedClientEnvDigest = digestClientEnv(usedClientEnv);
172+
this.actionPropertiesDigest = digestActionProperties(usedClientEnv, outputPermissions);
166173
files = discoversInputs ? new ArrayList<>() : null;
167174
mdMap = new HashMap<>();
168175
outputFileMetadata = new HashMap<>();
@@ -171,39 +178,46 @@ public Entry(String key, Map<String, String> usedClientEnv, boolean discoversInp
171178

172179
public Entry(
173180
String key,
174-
byte[] usedClientEnvDigest,
181+
byte[] actionPropertiesDigest,
175182
@Nullable List<String> files,
176183
byte[] digest,
177184
Map<String, RemoteFileArtifactValue> outputFileMetadata,
178185
Map<String, SerializableTreeArtifactValue> outputTreeMetadata) {
179186
actionKey = key;
180-
this.usedClientEnvDigest = usedClientEnvDigest;
187+
this.actionPropertiesDigest = actionPropertiesDigest;
181188
this.files = files;
182189
this.digest = digest;
183190
mdMap = null;
184191
this.outputFileMetadata = outputFileMetadata;
185192
this.outputTreeMetadata = outputTreeMetadata;
186193
}
187194

188-
private static final byte[] EMPTY_CLIENT_ENV_DIGEST = new byte[0];
189-
190195
/**
191-
* Computes an order-independent digest of a map of environment variables.
196+
* Computes an order-independent digest of action properties. This includes a map of client
197+
* environment variables and the non-default permissions for output artifacts of the action.
192198
*
193199
* <p>Note that as discussed in https://github.com/bazelbuild/bazel/issues/15660, using {@link
194200
* DigestUtils#xor} to achieve order-independence is questionable in case it is possible that
195201
* multiple string keys map to the same bytes when passed through {@link Fingerprint#addString}
196202
* (due to lossy conversion from UTF-16 to UTF-8). We could instead use a sorted map, however
197203
* changing the digest function would cause action cache misses across bazel versions.
198204
*/
199-
private static byte[] digestClientEnv(Map<String, String> clientEnv) {
205+
private static byte[] digestActionProperties(
206+
Map<String, String> clientEnv, OutputPermissions outputPermissions) {
200207
byte[] result = EMPTY_CLIENT_ENV_DIGEST;
201208
Fingerprint fp = new Fingerprint();
202209
for (Map.Entry<String, String> entry : clientEnv.entrySet()) {
203210
fp.addString(entry.getKey());
204211
fp.addString(entry.getValue());
205212
result = DigestUtils.xor(result, fp.digestAndReset());
206213
}
214+
// Add the permissions mode to the digest if it differs from the default.
215+
// This is a bit of a hack to save memory on entries which have the default permissions mode
216+
// and no client env.
217+
if (outputPermissions != OutputPermissions.READONLY) {
218+
fp.addInt(outputPermissions.getPermissionsMode());
219+
result = DigestUtils.xor(result, fp.digestAndReset());
220+
}
207221
return result;
208222
}
209223

@@ -288,14 +302,16 @@ public String getActionKey() {
288302
return actionKey;
289303
}
290304

291-
/** @return the effectively used client environment */
292-
public byte[] getUsedClientEnvDigest() {
293-
return usedClientEnvDigest;
305+
/** Returns the effectively used client environment. */
306+
public byte[] getActionPropertiesDigest() {
307+
return actionPropertiesDigest;
294308
}
295309

296-
/** Determines whether this entry used the same client environment as the one given. */
297-
public boolean usedSameClientEnv(Map<String, String> clientEnv) {
298-
return Arrays.equals(digestClientEnv(clientEnv), usedClientEnvDigest);
310+
/** Determines whether this entry has the same action properties as the one given. */
311+
public boolean sameActionProperties(
312+
Map<String, String> clientEnv, OutputPermissions outputPermissions) {
313+
return Arrays.equals(
314+
digestActionProperties(clientEnv, outputPermissions), actionPropertiesDigest);
299315
}
300316

301317
/**
@@ -343,7 +359,7 @@ public String toString() {
343359
builder.append(" actionKey = ").append(actionKey).append("\n");
344360
builder
345361
.append(" usedClientEnvKey = ")
346-
.append(formatDigest(usedClientEnvDigest))
362+
.append(formatDigest(actionPropertiesDigest))
347363
.append("\n");
348364
builder.append(" digestKey = ");
349365
if (digest == null) {

‎src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,7 @@ private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) thr
571571
VarInt.putVarInt(indexer.getOrCreateIndex(file), sink);
572572
}
573573

574-
MetadataDigestUtils.write(entry.getUsedClientEnvDigest(), sink);
574+
MetadataDigestUtils.write(entry.getActionPropertiesDigest(), sink);
575575

576576
VarInt.putVarInt(entry.getOutputFiles().size(), sink);
577577
for (Map.Entry<String, RemoteFileArtifactValue> file : entry.getOutputFiles().entrySet()) {

‎src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,17 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
170170
+ "disabled.")
171171
public boolean strictFilesets;
172172

173+
// This option is only used during execution. However, it is a required input to the analysis
174+
// phase, as otherwise flipping this flag would not invalidate already-executed actions.
175+
@Option(
176+
name = "experimental_writable_outputs",
177+
defaultValue = "false",
178+
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
179+
effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
180+
metadataTags = {OptionMetadataTag.EXPERIMENTAL},
181+
help = "If true, the file permissions of action outputs are set to 0755 instead of 0555")
182+
public boolean experimentalWritableOutputs;
183+
173184
@Option(
174185
name = "incompatible_strict_conflict_checks",
175186
oldName = "experimental_strict_conflict_checks",
@@ -970,6 +981,7 @@ public FragmentOptions getHost() {
970981
host.includeRequiredConfigFragmentsProvider = includeRequiredConfigFragmentsProvider;
971982
host.debugSelectsAlwaysSucceed = debugSelectsAlwaysSucceed;
972983
host.checkTestonlyForOutputFiles = checkTestonlyForOutputFiles;
984+
host.experimentalWritableOutputs = experimentalWritableOutputs;
973985
host.strictConflictChecks = strictConflictChecks;
974986

975987
// === Runfiles ===

‎src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import com.google.devtools.build.lib.remote.util.RxUtils.TransferResult;
4949
import com.google.devtools.build.lib.remote.util.TempPathGenerator;
5050
import com.google.devtools.build.lib.vfs.FileSystemUtils;
51+
import com.google.devtools.build.lib.vfs.OutputPermissions;
5152
import com.google.devtools.build.lib.vfs.Path;
5253
import com.google.devtools.build.lib.vfs.PathFragment;
5354
import io.reactivex.rxjava3.core.Completable;
@@ -74,6 +75,7 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet
7475
private final Reporter reporter;
7576
private final AsyncTaskCache.NoResult<Path> downloadCache = AsyncTaskCache.NoResult.create();
7677
private final TempPathGenerator tempPathGenerator;
78+
private final OutputPermissions outputPermissions;
7779
protected final Set<Artifact> outputsAreInputs = Sets.newConcurrentHashSet();
7880

7981
protected final Path execRoot;
@@ -123,11 +125,13 @@ protected AbstractActionInputPrefetcher(
123125
Reporter reporter,
124126
Path execRoot,
125127
TempPathGenerator tempPathGenerator,
126-
ImmutableList<Pattern> patternsToDownload) {
128+
ImmutableList<Pattern> patternsToDownload,
129+
OutputPermissions outputPermissions) {
127130
this.reporter = reporter;
128131
this.execRoot = execRoot;
129132
this.tempPathGenerator = tempPathGenerator;
130133
this.patternsToDownload = patternsToDownload;
134+
this.outputPermissions = outputPermissions;
131135
}
132136

133137
private boolean shouldDownloadFile(Path path, FileArtifactValue metadata) {
@@ -352,12 +356,12 @@ private Completable prefetchInputTree(
352356
}
353357

354358
for (Path dir : dirs) {
355-
// Change permission of all directories of a tree artifact to 0555 (files are
359+
// Change permission of all directories of a tree artifact (files are
356360
// changed inside {@code finalizeDownload}) in order to match the behaviour when
357361
// the tree artifact is generated locally. In that case, permission of all files
358-
// and directories inside a tree artifact is changed to 0555 within {@code
362+
// and directories inside a tree artifact is changed within {@code
359363
// checkOutputs()}.
360-
dir.chmod(0555);
364+
dir.chmod(outputPermissions.getPermissionsMode());
361365
}
362366

363367
completed.set(true);
@@ -537,9 +541,9 @@ private void finalizeDownload(Context context, Path tmpPath, Path path) throws I
537541
parentDir.setWritable(true);
538542
}
539543

540-
// The permission of output file is changed to 0555 after action execution. We manually change
544+
// The permission of output file is changed after action execution. We manually change
541545
// the permission here for the downloaded file to keep this behaviour consistent.
542-
tmpPath.chmod(0555);
546+
tmpPath.chmod(outputPermissions.getPermissionsMode());
543547
FileSystemUtils.moveFile(tmpPath, path);
544548
}
545549

‎src/main/java/com/google/devtools/build/lib/remote/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ java_library(
6161
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
6262
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
6363
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
64+
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
6465
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
6566
"//src/main/java/com/google/devtools/build/lib/analysis/platform:platform_utils",
6667
"//src/main/java/com/google/devtools/build/lib/authandtls",

‎src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import com.google.devtools.build.lib.server.FailureDetails;
3939
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
4040
import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code;
41+
import com.google.devtools.build.lib.vfs.OutputPermissions;
4142
import com.google.devtools.build.lib.vfs.Path;
4243
import com.google.devtools.build.lib.vfs.PathFragment;
4344
import io.reactivex.rxjava3.core.Completable;
@@ -65,8 +66,9 @@ class RemoteActionInputFetcher extends AbstractActionInputPrefetcher {
6566
Path execRoot,
6667
TempPathGenerator tempPathGenerator,
6768
ImmutableList<Pattern> patternsToDownload,
69+
OutputPermissions outputPermissions,
6870
boolean useNewExitCodeForLostInputs) {
69-
super(reporter, execRoot, tempPathGenerator, patternsToDownload);
71+
super(reporter, execRoot, tempPathGenerator, patternsToDownload, outputPermissions);
7072
this.buildRequestId = Preconditions.checkNotNull(buildRequestId);
7173
this.commandId = Preconditions.checkNotNull(commandId);
7274
this.remoteCache = Preconditions.checkNotNull(remoteCache);

‎src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import com.google.devtools.build.lib.analysis.BlazeDirectories;
4242
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
4343
import com.google.devtools.build.lib.analysis.config.BuildOptions;
44+
import com.google.devtools.build.lib.analysis.config.CoreOptions;
4445
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
4546
import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
4647
import com.google.devtools.build.lib.authandtls.CallCredentialsProvider;
@@ -94,6 +95,7 @@
9495
import com.google.devtools.build.lib.util.io.AsynchronousFileOutputStream;
9596
import com.google.devtools.build.lib.vfs.DigestHashFunction;
9697
import com.google.devtools.build.lib.vfs.FileSystem;
98+
import com.google.devtools.build.lib.vfs.OutputPermissions;
9799
import com.google.devtools.build.lib.vfs.OutputService;
98100
import com.google.devtools.build.lib.vfs.Path;
99101
import com.google.devtools.common.options.OptionsBase;
@@ -908,6 +910,11 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
908910
RemoteOptions remoteOptions =
909911
Preconditions.checkNotNull(
910912
env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions");
913+
CoreOptions coreOptions = env.getOptions().getOptions(CoreOptions.class);
914+
OutputPermissions outputPermissions =
915+
coreOptions.experimentalWritableOutputs
916+
? OutputPermissions.WRITABLE
917+
: OutputPermissions.READONLY;
911918
RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
912919

913920
if (!remoteOutputsMode.downloadAllOutputs() && actionContextProvider.getRemoteCache() != null) {
@@ -921,6 +928,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
921928
env.getExecRoot(),
922929
tempPathGenerator,
923930
patternsToDownload,
931+
outputPermissions,
924932
remoteOptions.useNewExitCodeForLostInputs);
925933
env.getEventBus().register(actionInputFetcher);
926934
builder.setActionInputPrefetcher(actionInputFetcher);

‎src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -742,6 +742,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
742742
state.inputArtifactData,
743743
action.discoversInputs(),
744744
skyframeActionExecutor.useArchivedTreeArtifacts(action),
745+
skyframeActionExecutor.getOutputPermissions(),
745746
action.getOutputs(),
746747
skyframeActionExecutor.getXattrProvider(),
747748
tsgm.get(),

0 commit comments

Comments
 (0)