Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Watch arbitrary file in repo rules #21230

Closed
wants to merge 1 commit into from
Closed

Watch arbitrary file in repo rules #21230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion MODULE.bazel.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input",
"//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
Expand Down Expand Up @@ -217,6 +218,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_directory_value",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_failed_exception",
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
@GenerateTypeAdapter
public abstract class BazelLockFileValue implements SkyValue, Postable {

public static final int LOCK_FILE_VERSION = 4;
public static final int LOCK_FILE_VERSION = 5;

@SerializationConstant public static final SkyKey KEY = () -> SkyFunctions.BAZEL_LOCK_FILE;

Expand Down
16 changes: 16 additions & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
import com.google.devtools.build.lib.vfs.Path;
import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
Expand Down Expand Up @@ -444,6 +445,20 @@ public Location read(JsonReader jsonReader) throws IOException {
}
}

private static final TypeAdapter<RepoRecordedInput.File> REPO_RECORDED_INPUT_FILE_TYPE_ADAPTER =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same vague dissatisfaction with this as with time stamps in lockfiles: this encodes non-hermetic information in lockfiles, which are supposed to be checked in. Although in this case, one could say that that could be avoid by carefully not depending on absolute paths...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have two mechanisms for non-hermetic operations, repository rules and module extensions, we should think about where we want to draw the boundary between them. If module extensions are meant to be optimized for "reproducibility in the face of non-hermeticity", i.e., locking down the result of potentially non-hermetic operations in a way that is portable across machines via the lockfile, then we may want to purposefully limit their capabilities.

How about adding the watch capability to repo rules only for now? We can always gather some data from user feedback before we extend it to module extensions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good points, and I'm not totally sure what's the best way forward here. It's awkward to add watch only to repo rules because rctx.read(watch=True) is in StarlarkBaseExternalContext which is shared by both contexts. We could alternatively just filter out any "absolute file" watches when we write the lockfile. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very knowledgeable about the philosophical difference between module extensions and repositories, but at least as far as the implementation goes, it'd be pretty easy to add an "access to absolute paths is allowed" flag to StarlarkBaseExternalContext, which would be set to "true" for repositories and "false" for module extensions.

Between filtering out absolute file watches and erroring out when they are attempted, I'd rather we do the latter because if we ever decide that this is a useful thing, it's much more easy to turn an error into a non-error than to subtly change the behavior of code that Bazel already accepts.

Copy link
Member Author

@Wyverald Wyverald Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the situation is a bit more nuanced -- I'm adding a boolean parameter watch to rctx.read which defaults to True. That is, any file read by rctx.read will be watched, be it a label-addressed file or an absolute path file.

Now we can't really just make it an error to attempt to watch absolute path files in module extensions, because it is legal to read an absolute path file in module extensions today. And yes, we could make watch default to True for repo rules and False for module extensions, but I'm not sure that's better (as we would by default not watch even files inside the workspace).

Copy link
Member Author

@Wyverald Wyverald Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my newest idea: make rctx.read(watch) a tristate: true - always watch (same as today's rctx.read(...); rctx.watch(...);); false - never watch; auto - watch if appropriate ("appropriate" = it's a file inside the workspace OR we're in a repo rule context). Defaults to 'auto'. WDYT? (EDIT: implemented this, see code for concept.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about watch() defaulting to false everywhere, at least for the time being? That would mean that there is no behavior change and it would also resolve the module extension / repository conundrum.

re: your tristate approach, I'm neutral; I think it's a bit of DWIM, but only a bit so I could live with it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to enable watch by default because the only change in behavior is that we refetch a bit more often, which is really an improvement in correctness. It also reduces surprises -- people have been asking why label-addressed files are watched but others aren't.

new TypeAdapter<>() {
@Override
public void write(JsonWriter jsonWriter, RepoRecordedInput.File value) throws IOException {
jsonWriter.value(value.toStringInternal());
}

@Override
public RepoRecordedInput.File read(JsonReader jsonReader) throws IOException {
return (RepoRecordedInput.File)
RepoRecordedInput.File.PARSER.parse(jsonReader.nextString());
}
};

public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) {
return new GsonBuilder()
.setPrettyPrinting()
Expand All @@ -468,6 +483,7 @@ public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) {
.registerTypeAdapter(ModuleExtensionId.IsolationKey.class, ISOLATION_KEY_TYPE_ADAPTER)
.registerTypeAdapter(AttributeValues.class, new AttributeValuesAdapter())
.registerTypeAdapter(byte[].class, BYTE_ARRAY_TYPE_ADAPTER)
.registerTypeAdapter(RepoRecordedInput.File.class, REPO_RECORDED_INPUT_FILE_TYPE_ADAPTER)
.create();
}

Expand Down
7 changes: 4 additions & 3 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableTable;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
import com.ryanharter.auto.value.gson.GenerateTypeAdapter;
import java.util.Optional;

Expand All @@ -41,7 +41,7 @@ public static Builder builder() {
@SuppressWarnings("mutable")
public abstract byte[] getBzlTransitiveDigest();

public abstract ImmutableMap<Label, String> getAccumulatedFileDigests();
public abstract ImmutableMap<RepoRecordedInput.File, String> getRecordedFileInputs();

public abstract ImmutableMap<String, String> getEnvVariables();

Expand All @@ -65,7 +65,8 @@ public abstract static class Builder {

public abstract Builder setBzlTransitiveDigest(byte[] digest);

public abstract Builder setAccumulatedFileDigests(ImmutableMap<Label, String> value);
public abstract Builder setRecordedFileInputs(
ImmutableMap<RepoRecordedInput.File, String> value);

public abstract Builder setEnvVariables(ImmutableMap<String, String> value);

Expand Down
6 changes: 5 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.docgen.annot.DocCategory;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkBaseExternalContext;
import com.google.devtools.build.lib.runtime.ProcessWrapper;
Expand Down Expand Up @@ -50,6 +51,7 @@ public class ModuleExtensionContext extends StarlarkBaseExternalContext {

protected ModuleExtensionContext(
Path workingDirectory,
BlazeDirectories directories,
Environment env,
Map<String, String> envVariables,
DownloadManager downloadManager,
Expand All @@ -62,13 +64,15 @@ protected ModuleExtensionContext(
boolean rootModuleHasNonDevDependency) {
super(
workingDirectory,
directories,
env,
envVariables,
downloadManager,
timeoutScaling,
processWrapper,
starlarkSemantics,
remoteExecutor);
remoteExecutor,
/* allowWatchingFilesOutsideWorkspace= */ false);
this.extensionId = extensionId;
this.modules = modules;
this.rootModuleHasNonDevDependency = rootModuleHasNonDevDependency;
Expand Down
49 changes: 11 additions & 38 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import com.google.common.collect.Table;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode;
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
Expand All @@ -48,6 +47,7 @@
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException;
import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
import com.google.devtools.build.lib.runtime.ProcessWrapper;
import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor;
Expand All @@ -61,7 +61,6 @@
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
Expand All @@ -71,7 +70,6 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
Expand Down Expand Up @@ -219,7 +217,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
extension.getEvalFactors(),
LockFileModuleExtension.builder()
.setBzlTransitiveDigest(extension.getBzlTransitiveDigest())
.setAccumulatedFileDigests(moduleExtensionResult.getAccumulatedFileDigests())
.setRecordedFileInputs(moduleExtensionResult.getRecordedFileInputs())
.setEnvVariables(extension.getEnvVars())
.setGeneratedRepoSpecs(generatedRepoSpecs)
.setModuleExtensionMetadata(moduleExtensionMetadata)
Expand Down Expand Up @@ -291,7 +289,7 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile(
+ extensionId
+ "' have changed");
}
if (didFilesChange(env, lockedExtension.getAccumulatedFileDigests())) {
if (didFilesChange(env, directories, lockedExtension.getRecordedFileInputs())) {
diffRecorder.record(
"One or more files the extension '" + extensionId + "' is using have changed");
}
Expand Down Expand Up @@ -393,39 +391,13 @@ private static boolean didRepoMappingsChange(
}

private static boolean didFilesChange(
Environment env, ImmutableMap<Label, String> accumulatedFileDigests)
Environment env, BlazeDirectories directories, ImmutableMap<RepoRecordedInput.File, String> recordedFileInputs)
throws InterruptedException, NeedsSkyframeRestartException {
// Turn labels into FileValue keys & get those values
Map<Label, FileValue.Key> fileKeys = new HashMap<>();
for (Label label : accumulatedFileDigests.keySet()) {
try {
RootedPath rootedPath = RepositoryFunction.getRootedPathFromLabel(label, env);
fileKeys.put(label, FileValue.key(rootedPath));
} catch (NeedsSkyframeRestartException e) {
throw e;
} catch (EvalException e) {
// Consider those exception to be a cause for invalidation
return true;
}
}
SkyframeLookupResult result = env.getValuesAndExceptions(fileKeys.values());
boolean upToDate = RepoRecordedInput.areAllValuesUpToDate(env, directories, recordedFileInputs);
if (env.valuesMissing()) {
throw new NeedsSkyframeRestartException();
}

// Compare the collected file values with the hashes stored in the lockfile
for (Entry<Label, String> entry : accumulatedFileDigests.entrySet()) {
FileValue fileValue = (FileValue) result.get(fileKeys.get(entry.getKey()));
try {
if (!entry.getValue().equals(RepositoryFunction.fileValueToMarkerValue(fileValue))) {
return true;
}
} catch (IOException e) {
// Consider those exception to be a cause for invalidation
return true;
}
}
return false;
return !upToDate;
}

/**
Expand Down Expand Up @@ -956,7 +928,7 @@ public RunModuleExtensionResult run(
}
}
return RunModuleExtensionResult.create(
moduleContext.getAccumulatedFileDigests(),
moduleContext.getRecordedFileInputs(),
threadContext.getGeneratedRepoSpecs(),
moduleExtensionMetadata,
repoMappingRecorder.recordedEntries());
Expand Down Expand Up @@ -992,6 +964,7 @@ private ModuleExtensionContext createContext(
rootUsage != null && rootUsage.getHasNonDevUseExtension();
return new ModuleExtensionContext(
workingDirectory,
directories,
env,
clientEnvironmentSupplier.get(),
downloadManager,
Expand All @@ -1016,7 +989,7 @@ static final class SingleExtensionEvalFunctionException extends SkyFunctionExcep
@AutoValue
abstract static class RunModuleExtensionResult {

abstract ImmutableMap<Label, String> getAccumulatedFileDigests();
abstract ImmutableMap<RepoRecordedInput.File, String> getRecordedFileInputs();

abstract ImmutableMap<String, RepoSpec> getGeneratedRepoSpecs();

Expand All @@ -1025,12 +998,12 @@ abstract static class RunModuleExtensionResult {
abstract ImmutableTable<RepositoryName, String, RepositoryName> getRecordedRepoMappingEntries();

static RunModuleExtensionResult create(
ImmutableMap<Label, String> accumulatedFileDigests,
ImmutableMap<RepoRecordedInput.File, String> recordedFileInputs,
ImmutableMap<String, RepoSpec> generatedRepoSpecs,
Optional<ModuleExtensionMetadata> moduleExtensionMetadata,
ImmutableTable<RepositoryName, String, RepositoryName> recordedRepoMappingEntries) {
return new AutoValue_SingleExtensionEvalFunction_RunModuleExtensionResult(
accumulatedFileDigests,
recordedFileInputs,
generatedRepoSpecs,
moduleExtensionMetadata,
recordedRepoMappingEntries);
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/repository/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/repository:repository_events",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_directory_value",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function",
"//src/main/java/com/google/devtools/build/lib/rules:repository/resolved_file_value",
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/repository:repository_events",
"//src/main/java/com/google/devtools/build/lib/repository:request_repository_information_event",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_directory_value",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function",
"//src/main/java/com/google/devtools/build/lib/rules:repository/workspace_attribute_mapper",
Expand Down
Loading
Loading