Skip to content

Commit

Permalink
Watch arbitrary file in repo rules
Browse files Browse the repository at this point in the history
* Starlark API changes
  * New method `rctx.watch()` that watches a path. As of right now, said path must exist and be a regular file, but that will change in follow-ups.
  * Existing method `rctx.read()` gets a new optional boolean parameter `watch` that defaults to True. This causes the file read to be watched as well by default, even if it's not addressed by a label.
  * Both changes apply to `mctx` as well.
* Marker file format changes
  * `FILE:` marker file entries can now be `FILE:` followed by either an absolute path (starts with a singular '/'), or a label-like ... thing (which is a label but has a slash in place of a colon). This is because we might be watching something inside a repo that is _not_ under a package.
  * Same change will happen to the `accumulatedFileDigests` sections of the module lockfile.
* Code changes
  * `RepoRecordedInput.File` subclasses are renamed to `FileOutsideWorkspace` (absolute paths) and `FileInsideWorkspace` (label-like things) instead to better reflect their meaning.
  * Unified the file watching/digest checking logic in SingleExtensionEvalFunction with the RepoRecordedInput stuff. Follow-ups will unify more.
  * Moved RepoRecordedInput to its own BUILD target as it's being used outside the repo fetching machinery (namely, module extensions)
  * Since `FileOutsideWorkspace` has to be an absolute path (never relative), we don't need a "base directory" for the RepoRecordedInput parser. What we really need is just the FileSystem object. Refactored code to reflect that.

Work towards #20952.
  • Loading branch information
Wyverald committed Feb 7, 2024
1 parent 9f0f232 commit 7c7a3a6
Show file tree
Hide file tree
Showing 32 changed files with 527 additions and 190 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ public void workspaceInit(
new ModuleFileFunction(registryFactory, directories.getWorkspace(), builtinModules))
.addSkyFunction(SkyFunctions.BAZEL_DEP_GRAPH, new BazelDepGraphFunction())
.addSkyFunction(
SkyFunctions.BAZEL_LOCK_FILE, new BazelLockFileFunction(directories.getWorkspace()))
SkyFunctions.BAZEL_LOCK_FILE, new BazelLockFileFunction(directories.getWorkspace(), runtime.getFileSystem()))
.addSkyFunction(SkyFunctions.BAZEL_FETCH_ALL, new BazelFetchAllFunction())
.addSkyFunction(SkyFunctions.BAZEL_MOD_TIDY, new BazelModTidyFunction())
.addSkyFunction(SkyFunctions.BAZEL_MODULE_INSPECTION, new BazelModuleInspectorFunction())
Expand Down
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
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code;
import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.Root;
Expand All @@ -52,6 +53,7 @@ public class BazelLockFileFunction implements SkyFunction {
Pattern.compile("\"lockFileVersion\":\\s*(\\d+)");

private final Path rootDirectory;
private final FileSystem runtimeFileSystem;

private static final BzlmodFlagsAndEnvVars EMPTY_FLAGS =
BzlmodFlagsAndEnvVars.create(
Expand All @@ -67,8 +69,9 @@ public class BazelLockFileFunction implements SkyFunction {
.setModuleExtensions(ImmutableMap.of())
.build();

public BazelLockFileFunction(Path rootDirectory) {
public BazelLockFileFunction(Path rootDirectory, FileSystem runtimeFileSystem) {
this.rootDirectory = rootDirectory;
this.runtimeFileSystem = runtimeFileSystem;
}

@Override
Expand All @@ -84,7 +87,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

try (SilentCloseable c = Profiler.instance().profile(ProfilerTask.BZLMOD, "parse lockfile")) {
return getLockfileValue(lockfilePath, rootDirectory);
return getLockfileValue(lockfilePath, rootDirectory, runtimeFileSystem);
} catch (IOException | JsonSyntaxException | NullPointerException e) {
throw new BazelLockfileFunctionException(
ExternalDepsException.withMessage(
Expand All @@ -96,7 +99,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}
}

public static BazelLockFileValue getLockfileValue(RootedPath lockfilePath, Path rootDirectory)
public static BazelLockFileValue getLockfileValue(
RootedPath lockfilePath, Path rootDirectory, FileSystem runtimeFileSystem)
throws IOException {
BazelLockFileValue bazelLockFileValue;
try {
Expand All @@ -110,7 +114,8 @@ public static BazelLockFileValue getLockfileValue(RootedPath lockfilePath, Path
.asPath()
.getParentDirectory()
.getRelative(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME),
rootDirectory)
rootDirectory,
runtimeFileSystem)
.fromJson(json, BazelLockFileValue.class);
} else {
// This is an old version, needs to be updated
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.devtools.build.lib.runtime.BlazeModule;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.Root;
Expand All @@ -47,6 +48,7 @@ public class BazelLockFileModule extends BlazeModule {
@Nullable private BazelModuleResolutionEvent moduleResolutionEvent;
private final Map<ModuleExtensionId, ModuleExtensionResolutionEvent>
extensionResolutionEventsMap = new HashMap<>();
private FileSystem runtimeFileSystem;

private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();

Expand All @@ -57,6 +59,7 @@ public void beforeCommand(CommandEnvironment env) {
if (options.lockfileMode.equals(LockfileMode.UPDATE)) {
env.getEventBus().register(this);
}
runtimeFileSystem = env.getRuntime().getFileSystem();
}

@Override
Expand Down Expand Up @@ -90,7 +93,7 @@ public void afterCommand() throws AbruptExitException {
// on the next build, which breaks commands such as `bazel config` that rely on
// com.google.devtools.build.skyframe.MemoizingEvaluator#getDoneValues.
if (!newLockfile.equals(oldLockfile)) {
updateLockfile(workspaceRoot, newLockfile);
updateLockfile(workspaceRoot, newLockfile, runtimeFileSystem);
}
this.moduleResolutionEvent = null;
this.extensionResolutionEventsMap.clear();
Expand Down Expand Up @@ -190,7 +193,8 @@ private boolean shouldKeepExtension(
* @param workspaceRoot Root of the workspace where the lockfile is located
* @param updatedLockfile The updated lockfile data to save
*/
public static void updateLockfile(Path workspaceRoot, BazelLockFileValue updatedLockfile) {
public static void updateLockfile(
Path workspaceRoot, BazelLockFileValue updatedLockfile, FileSystem runtimeFileSystem) {
RootedPath lockfilePath =
RootedPath.toRootedPath(Root.fromPath(workspaceRoot), LabelConstants.MODULE_LOCKFILE_NAME);
try {
Expand All @@ -202,7 +206,8 @@ public static void updateLockfile(Path workspaceRoot, BazelLockFileValue updated
.asPath()
.getParentDirectory()
.getRelative(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME),
workspaceRoot)
workspaceRoot,
runtimeFileSystem)
.toJson(updatedLockfile)
+ "\n");
} catch (IOException e) {
Expand Down
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 = 3;
public static final int LOCK_FILE_VERSION = 4;

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
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.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
Expand Down Expand Up @@ -444,7 +446,24 @@ public Location read(JsonReader jsonReader) throws IOException {
}
}

public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) {
private static TypeAdapter<RepoRecordedInput.File> makeRepoRecordedInputFileTypeAdapter(
FileSystem runtimeFileSystem) {
return 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(), runtimeFileSystem);
}
};
}

public static Gson createLockFileGson(
Path moduleFilePath, Path workspaceRoot, FileSystem runtimeFileSystem) {
return new GsonBuilder()
.setPrettyPrinting()
.disableHtmlEscaping()
Expand All @@ -468,6 +487,8 @@ 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, makeRepoRecordedInputFileTypeAdapter(runtimeFileSystem))
.create();
}

Expand Down
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 @@ -60,7 +60,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
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,6 +64,7 @@ protected ModuleExtensionContext(
boolean rootModuleHasNonDevDependency) {
super(
workingDirectory,
directories,
env,
envVariables,
downloadManager,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,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 @@ -49,6 +48,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 @@ -62,7 +62,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 @@ -72,7 +71,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 @@ -195,7 +193,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 @@ -284,7 +282,7 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile(
+ extensionId
+ "' have changed");
}
if (didFilesChange(env, lockedExtension.getAccumulatedFileDigests())) {
if (didFilesChange(env, lockedExtension.getRecordedFileInputs())) {
diffRecorder.record(
"One or more files the extension '" + extensionId + "' is using have changed");
}
Expand Down Expand Up @@ -386,39 +384,13 @@ private static boolean didRepoMappingsChange(
}

private static boolean didFilesChange(
Environment env, ImmutableMap<Label, String> accumulatedFileDigests)
Environment env, 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, 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 @@ -951,7 +923,7 @@ public RunModuleExtensionResult run(
}
}
return RunModuleExtensionResult.create(
moduleContext.getAccumulatedFileDigests(),
moduleContext.getRecordedFileInputs(),
threadContext.getGeneratedRepoSpecs(),
moduleExtensionMetadata,
repoMappingRecorder.recordedEntries());
Expand Down Expand Up @@ -987,6 +959,7 @@ private ModuleExtensionContext createContext(
rootUsage != null && rootUsage.getHasNonDevUseExtension();
return new ModuleExtensionContext(
workingDirectory,
directories,
env,
clientEnvironmentSupplier.get(),
downloadManager,
Expand All @@ -1011,7 +984,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 @@ -1020,12 +993,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
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
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

0 comments on commit 7c7a3a6

Please sign in to comment.