Skip to content

Commit

Permalink
[6.1.0] Flag for writable outputs (experimental) (bazelbuild#17617)
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
4 people authored Feb 28, 2023
1 parent 034a281 commit 5afbce5
Show file tree
Hide file tree
Showing 21 changed files with 605 additions and 109 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -291,9 +292,10 @@ private static Map<String, String> computeUsedEnv(
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);
Expand Down Expand Up @@ -419,6 +421,7 @@ public Token getTokenIfNeedToExecute(
Action action,
List<Artifact> resolvedCacheArtifacts,
Map<String, String> clientEnv,
OutputPermissions outputPermissions,
EventHandler handler,
MetadataHandler metadataHandler,
ArtifactExpander artifactExpander,
Expand Down Expand Up @@ -481,6 +484,7 @@ public Token getTokenIfNeedToExecute(
artifactExpander,
actionInputs,
clientEnv,
outputPermissions,
remoteDefaultPlatformProperties,
cachedOutputMetadata)) {
if (entry != null) {
Expand Down Expand Up @@ -510,6 +514,7 @@ private boolean mustExecute(
ArtifactExpander artifactExpander,
NestedSet<Artifact> actionInputs,
Map<String, String> clientEnv,
OutputPermissions outputPermissions,
Map<String, String> remoteDefaultPlatformProperties,
@Nullable CachedOutputMetadata cachedOutputMetadata)
throws InterruptedException {
Expand Down Expand Up @@ -542,7 +547,7 @@ private boolean mustExecute(
}
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;
Expand Down Expand Up @@ -580,6 +585,7 @@ public void updateActionCache(
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);
Expand All @@ -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();
Expand Down Expand Up @@ -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);
Expand All @@ -768,6 +775,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
Action action,
List<Artifact> resolvedCacheArtifacts,
Map<String, String> clientEnv,
OutputPermissions outputPermissions,
EventHandler handler,
MetadataHandler metadataHandler,
ArtifactExpander artifactExpander,
Expand All @@ -781,6 +789,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
action,
resolvedCacheArtifacts,
clientEnv,
outputPermissions,
handler,
metadataHandler,
artifactExpander,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -95,7 +98,7 @@ final class Entry {
// 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;

Expand Down Expand Up @@ -160,9 +163,13 @@ public static Optional<SerializableTreeArtifactValue> createSerializable(
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<>();
Expand All @@ -171,39 +178,46 @@ public Entry(String key, Map<String, String> usedClientEnv, boolean discoversInp

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;
this.outputFileMetadata = outputFileMetadata;
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
* multiple string keys map to the same bytes when passed through {@link Fingerprint#addString}
* (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()) {
fp.addString(entry.getKey());
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;
}

Expand Down Expand Up @@ -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<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);
}

/**
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, RemoteFileArtifactValue> file : entry.getOutputFiles().entrySet()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -970,6 +981,7 @@ public FragmentOptions getHost() {
host.includeRequiredConfigFragmentsProvider = includeRequiredConfigFragmentsProvider;
host.debugSelectsAlwaysSucceed = debugSelectsAlwaysSucceed;
host.checkTestonlyForOutputFiles = checkTestonlyForOutputFiles;
host.experimentalWritableOutputs = experimentalWritableOutputs;
host.strictConflictChecks = strictConflictChecks;

// === Runfiles ===
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -74,6 +75,7 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet
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;
Expand Down Expand Up @@ -123,11 +125,13 @@ protected AbstractActionInputPrefetcher(
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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/remote/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -65,8 +66,9 @@ class RemoteActionInputFetcher extends AbstractActionInputPrefetcher {
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
state.inputArtifactData,
action.discoversInputs(),
skyframeActionExecutor.useArchivedTreeArtifacts(action),
skyframeActionExecutor.getOutputPermissions(),
action.getOutputs(),
skyframeActionExecutor.getXattrProvider(),
tsgm.get(),
Expand Down
Loading

0 comments on commit 5afbce5

Please sign in to comment.