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.

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

[7.1.0] Repo file/dir watching API #21435

Merged
merged 9 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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

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

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/autocodec",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
Expand Down Expand Up @@ -216,6 +217,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_value",
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 = 4;
public static final int LOCK_FILE_VERSION = 6;

@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,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,36 @@ public Location read(JsonReader jsonReader) throws IOException {
}
}

private static final TypeAdapter<RepoRecordedInput.File> REPO_RECORDED_INPUT_FILE_TYPE_ADAPTER =
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());
}
};

private static final TypeAdapter<RepoRecordedInput.Dirents>
REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER =
new TypeAdapter<>() {
@Override
public void write(JsonWriter jsonWriter, RepoRecordedInput.Dirents value)
throws IOException {
jsonWriter.value(value.toStringInternal());
}

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

public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) {
return new GsonBuilder()
.setPrettyPrinting()
Expand All @@ -468,6 +499,9 @@ 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)
.registerTypeAdapter(
RepoRecordedInput.Dirents.class, REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER)
.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,9 @@ 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<RepoRecordedInput.Dirents, String> getRecordedDirentsInputs();

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

Expand All @@ -65,7 +67,11 @@ 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 setRecordedDirentsInputs(
ImmutableMap<RepoRecordedInput.Dirents, 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,13 +64,15 @@ protected ModuleExtensionContext(
boolean rootModuleHasNonDevDependency) {
super(
workingDirectory,
directories,
env,
envVariables,
downloadManager,
timeoutScaling,
processWrapper,
starlarkSemantics,
remoteExecutor);
remoteExecutor,
/* allowWatchingPathsOutsideWorkspace= */ false);
this.extensionId = extensionId;
this.modules = modules;
this.rootModuleHasNonDevDependency = rootModuleHasNonDevDependency;
Expand Down
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,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
extension.getEvalFactors(),
LockFileModuleExtension.builder()
.setBzlTransitiveDigest(extension.getBzlTransitiveDigest())
.setAccumulatedFileDigests(moduleExtensionResult.getAccumulatedFileDigests())
.setRecordedFileInputs(moduleExtensionResult.getRecordedFileInputs())
.setRecordedDirentsInputs(moduleExtensionResult.getRecordedDirentsInputs())
.setEnvVariables(extension.getEnvVars())
.setGeneratedRepoSpecs(generatedRepoSpecs)
.setModuleExtensionMetadata(moduleExtensionMetadata)
Expand Down Expand Up @@ -291,10 +290,16 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile(
+ extensionId
+ "' have changed");
}
if (didFilesChange(env, lockedExtension.getAccumulatedFileDigests())) {
if (didRecordedInputsChange(env, directories, lockedExtension.getRecordedFileInputs())) {
diffRecorder.record(
"One or more files the extension '" + extensionId + "' is using have changed");
}
if (didRecordedInputsChange(env, directories, lockedExtension.getRecordedDirentsInputs())) {
diffRecorder.record(
"One or more directory listings watched by the extension '"
+ extensionId
+ "' have changed");
}
} catch (DiffFoundEarlyExitException ignored) {
// ignored
}
Expand Down Expand Up @@ -392,40 +397,16 @@ private static boolean didRepoMappingsChange(
return false;
}

private static boolean didFilesChange(
Environment env, ImmutableMap<Label, String> accumulatedFileDigests)
private static boolean didRecordedInputsChange(
Environment env,
BlazeDirectories directories,
ImmutableMap<? extends RepoRecordedInput, String> recordedInputs)
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, recordedInputs);
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 @@ -788,6 +769,7 @@ public RunModuleExtensionResult run(
generatedRepoSpecs.put(name, repoSpec);
}
return RunModuleExtensionResult.create(
ImmutableMap.of(),
ImmutableMap.of(),
generatedRepoSpecs.buildOrThrow(),
Optional.of(ModuleExtensionMetadata.REPRODUCIBLE),
Expand Down Expand Up @@ -956,7 +938,8 @@ public RunModuleExtensionResult run(
}
}
return RunModuleExtensionResult.create(
moduleContext.getAccumulatedFileDigests(),
moduleContext.getRecordedFileInputs(),
moduleContext.getRecordedDirentsInputs(),
threadContext.getGeneratedRepoSpecs(),
moduleExtensionMetadata,
repoMappingRecorder.recordedEntries());
Expand Down Expand Up @@ -992,6 +975,7 @@ private ModuleExtensionContext createContext(
rootUsage != null && rootUsage.getHasNonDevUseExtension();
return new ModuleExtensionContext(
workingDirectory,
directories,
env,
clientEnvironmentSupplier.get(),
downloadManager,
Expand All @@ -1016,7 +1000,9 @@ static final class SingleExtensionEvalFunctionException extends SkyFunctionExcep
@AutoValue
abstract static class RunModuleExtensionResult {

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

abstract ImmutableMap<RepoRecordedInput.Dirents, String> getRecordedDirentsInputs();

abstract ImmutableMap<String, RepoSpec> getGeneratedRepoSpecs();

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

static RunModuleExtensionResult create(
ImmutableMap<Label, String> accumulatedFileDigests,
ImmutableMap<RepoRecordedInput.File, String> recordedFileInputs,
ImmutableMap<RepoRecordedInput.Dirents, String> recordedDirentsInputs,
ImmutableMap<String, RepoSpec> generatedRepoSpecs,
Optional<ModuleExtensionMetadata> moduleExtensionMetadata,
ImmutableTable<RepositoryName, String, RepositoryName> recordedRepoMappingEntries) {
return new AutoValue_SingleExtensionEvalFunction_RunModuleExtensionResult(
accumulatedFileDigests,
recordedFileInputs,
recordedDirentsInputs,
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 @@ -20,6 +20,7 @@
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.bazel.ResolvedEvent;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
import com.google.devtools.build.lib.rules.repository.ResolvedFileValue;
Expand Down Expand Up @@ -56,7 +57,7 @@ public RepositoryDirectoryValue.Builder fetch(
Path outputDirectory,
BlazeDirectories directories,
Environment env,
Map<String, String> markerData,
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws RepositoryFunctionException {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,15 @@ 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",
"//src/main/java/com/google/devtools/build/lib/rules:repository/workspace_file_helper",
"//src/main/java/com/google/devtools/build/lib/shell",
"//src/main/java/com/google/devtools/build/lib/skyframe:action_environment_function",
"//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:directory_tree_digest_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:ignored_package_prefixes_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.bazel.repository.starlark;

import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState;
Expand Down Expand Up @@ -71,12 +72,12 @@ enum Signal {
@Nullable volatile Future<RepositoryDirectoryValue.Builder> workerFuture = null;

/**
* This is where the {@code markerData} for the whole invocation is collected.
* This is where the recorded inputs & values for the whole invocation is collected.
*
* <p>{@link com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction} creates a
* new map on each restart, so we can't simply plumb that in.
*/
final Map<String, String> markerData = new TreeMap<>();
final Map<RepoRecordedInput, String> recordedInputValues = new TreeMap<>();

SkyFunction.Environment signalForFreshEnv() throws InterruptedException {
signalQueue.put(Signal.RESTART);
Expand Down
Loading
Loading