From 4a4b181c2bac726d1862952d110670676c926346 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Tue, 6 Feb 2024 21:11:17 -0500 Subject: [PATCH] Watch arbitrary file in repo rules * 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 string parameter `watch` that defaults to `auto`. This causes the file read to be watched as well by default, even if it's not addressed by a label. See `rctx.read()` docs for the semantics of `watch`. * Both changes apply to `mctx` as well, except that no files outside the current Bazel workspace may be watched for `mctx`. * 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, which we can get from the PathPackageLocator available in Skyframe. Refactored code to reflect that. RELNOTES: Added a new method, `repository_ctx.watch()`, which asks Bazel to watch for changes to an arbitrary file. When said file changes, Bazel will refetch the repo. Work towards #20952. --- MODULE.bazel.lock | 2 +- compile.sh | 1 + .../devtools/build/lib/bazel/bzlmod/BUILD | 2 + .../lib/bazel/bzlmod/BazelLockFileValue.java | 2 +- .../lib/bazel/bzlmod/GsonTypeAdapterUtil.java | 16 ++ .../bazel/bzlmod/LockFileModuleExtension.java | 7 +- .../bazel/bzlmod/ModuleExtensionContext.java | 6 +- .../bzlmod/SingleExtensionEvalFunction.java | 49 +--- .../devtools/build/lib/bazel/repository/BUILD | 1 + .../build/lib/bazel/repository/starlark/BUILD | 1 + .../starlark/StarlarkBaseExternalContext.java | 197 ++++++++++--- .../repository/starlark/StarlarkPath.java | 2 +- .../starlark/StarlarkRepositoryContext.java | 11 +- .../starlark/StarlarkRepositoryFunction.java | 7 +- .../android/AndroidNdkRepositoryFunction.java | 7 +- .../android/AndroidSdkRepositoryFunction.java | 7 +- .../build/lib/bazel/rules/android/BUILD | 1 + .../com/google/devtools/build/lib/rules/BUILD | 26 +- .../repository/NewRepositoryFileHandler.java | 6 +- .../rules/repository/RepoRecordedInput.java | 264 ++++++++++++++---- .../RepositoryDelegatorFunction.java | 14 +- .../rules/repository/RepositoryFunction.java | 40 +-- .../build/lib/bazel/repository/starlark/BUILD | 2 + .../StarlarkRepositoryContextTest.java | 17 +- .../devtools/build/lib/rules/repository/BUILD | 1 + .../repository/RepositoryFunctionTest.java | 4 +- src/test/py/bazel/bzlmod/bazel_fetch_test.py | 3 +- .../py/bazel/bzlmod/bazel_lockfile_test.py | 58 ++++ .../shell/bazel/starlark_repository_test.sh | 150 ++++++++++ src/test/tools/bzlmod/MODULE.bazel.lock | 28 +- 30 files changed, 708 insertions(+), 224 deletions(-) diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index ac7b39f567a5cc..f1ac3e85d1bbd4 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -2741,7 +2741,7 @@ "general": { "bzlTransitiveDigest": "qF4MUyoxY7fCY0kaWtM87qFtK+boKcqviSosJqqpaDI=", "accumulatedFileDigests": { - "@@//src/test/tools/bzlmod:MODULE.bazel.lock": "e591609d4999da0cac2aad19df3ff8a1e42f3f032fb16308037d0d9e555f369f", + "@@//src/test/tools/bzlmod:MODULE.bazel.lock": "2ac47b971c5b12765dc679ce4137864f67f02f867f43ec22f86ecc203b905a1c", "@@//:MODULE.bazel": "f291782aef1d2989f49c0e884b32ec8d7814ae48c598b6f3060870ccb3f5c0b6" }, "envVariables": {}, diff --git a/compile.sh b/compile.sh index d91583d277cba8..3e95db7f75f902 100755 --- a/compile.sh +++ b/compile.sh @@ -67,6 +67,7 @@ bazel_build "src:bazel_nojdk${EXE_EXT}" \ --action_env=PATH \ --host_platform=@local_config_platform//:host \ --platforms=@local_config_platform//:host \ + --lockfile_mode=error \ || fail "Could not build Bazel" bazel_bin_path="$(get_bazel_bin_path)/src/bazel_nojdk${EXE_EXT}" [ -e "$bazel_bin_path" ] \ diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD index 6ecf0be408019c..a6300dede5c6f6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD @@ -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", @@ -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", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java index 3ae71c57c66135..0855f0a1aa74e2 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java @@ -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; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java index 808d83821d6291..2a4d28d960cee4 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java @@ -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; @@ -444,6 +445,20 @@ public Location read(JsonReader jsonReader) throws IOException { } } + private static final TypeAdapter 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()); + } + }; + public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) { return new GsonBuilder() .setPrettyPrinting() @@ -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(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java index 1443189e389a42..d526a79edacdd0 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java @@ -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; @@ -41,7 +41,7 @@ public static Builder builder() { @SuppressWarnings("mutable") public abstract byte[] getBzlTransitiveDigest(); - public abstract ImmutableMap getAccumulatedFileDigests(); + public abstract ImmutableMap getRecordedFileInputs(); public abstract ImmutableMap getEnvVariables(); @@ -65,7 +65,8 @@ public abstract static class Builder { public abstract Builder setBzlTransitiveDigest(byte[] digest); - public abstract Builder setAccumulatedFileDigests(ImmutableMap value); + public abstract Builder setRecordedFileInputs( + ImmutableMap value); public abstract Builder setEnvVariables(ImmutableMap value); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java index cdddef1c3a1479..52151259200462 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java @@ -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; @@ -50,6 +51,7 @@ public class ModuleExtensionContext extends StarlarkBaseExternalContext { protected ModuleExtensionContext( Path workingDirectory, + BlazeDirectories directories, Environment env, Map envVariables, DownloadManager downloadManager, @@ -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; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index 38ca1ba625b32d..10a39168ba4b88 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -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; @@ -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; @@ -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; @@ -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; @@ -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) @@ -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"); } @@ -393,39 +391,13 @@ private static boolean didRepoMappingsChange( } private static boolean didFilesChange( - Environment env, ImmutableMap accumulatedFileDigests) + Environment env, BlazeDirectories directories, ImmutableMap recordedFileInputs) throws InterruptedException, NeedsSkyframeRestartException { - // Turn labels into FileValue keys & get those values - Map 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 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; } /** @@ -956,7 +928,7 @@ public RunModuleExtensionResult run( } } return RunModuleExtensionResult.create( - moduleContext.getAccumulatedFileDigests(), + moduleContext.getRecordedFileInputs(), threadContext.getGeneratedRepoSpecs(), moduleExtensionMetadata, repoMappingRecorder.recordedEntries()); @@ -992,6 +964,7 @@ private ModuleExtensionContext createContext( rootUsage != null && rootUsage.getHasNonDevUseExtension(); return new ModuleExtensionContext( workingDirectory, + directories, env, clientEnvironmentSupplier.get(), downloadManager, @@ -1016,7 +989,7 @@ static final class SingleExtensionEvalFunctionException extends SkyFunctionExcep @AutoValue abstract static class RunModuleExtensionResult { - abstract ImmutableMap getAccumulatedFileDigests(); + abstract ImmutableMap getRecordedFileInputs(); abstract ImmutableMap getGeneratedRepoSpecs(); @@ -1025,12 +998,12 @@ abstract static class RunModuleExtensionResult { abstract ImmutableTable getRecordedRepoMappingEntries(); static RunModuleExtensionResult create( - ImmutableMap accumulatedFileDigests, + ImmutableMap recordedFileInputs, ImmutableMap generatedRepoSpecs, Optional moduleExtensionMetadata, ImmutableTable recordedRepoMappingEntries) { return new AutoValue_SingleExtensionEvalFunction_RunModuleExtensionResult( - accumulatedFileDigests, + recordedFileInputs, generatedRepoSpecs, moduleExtensionMetadata, recordedRepoMappingEntries); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/repository/BUILD index b624864b06e4b6..2f6a957d8f9cd6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/BUILD @@ -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", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD index 4ce0fabc14a265..5811960f84619b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD @@ -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", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index bdbdb08a2e5ffb..aaedebc5d51452 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -27,6 +27,7 @@ import com.google.common.collect.Maps; import com.google.common.util.concurrent.Futures; import com.google.devtools.build.lib.actions.FileValue; +import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.debug.WorkspaceRuleEvent; import com.google.devtools.build.lib.bazel.repository.DecompressorDescriptor; import com.google.devtools.build.lib.bazel.repository.DecompressorValue; @@ -36,6 +37,8 @@ import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager; import com.google.devtools.build.lib.bazel.repository.downloader.HttpUtils; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.LabelConstants; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.ExtendedEventHandler.FetchProgress; @@ -47,6 +50,9 @@ 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.RepoRecordedInput.FileInsideWorkspace; +import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.FileOutsideWorkspace; import com.google.devtools.build.lib.rules.repository.RepositoryFunction; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException; import com.google.devtools.build.lib.runtime.ProcessWrapper; @@ -54,15 +60,16 @@ import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor.ExecutionResult; import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction; import com.google.devtools.build.lib.util.OsUtils; +import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; -import com.google.devtools.build.skyframe.SkyKey; import java.io.File; import java.io.IOException; import java.io.OutputStream; @@ -133,6 +140,7 @@ private interface AsyncTask { private static final int MAX_PROFILE_ARGS_LEN = 512; protected final Path workingDirectory; + protected final BlazeDirectories directories; protected final Environment env; protected final ImmutableMap envVariables; private final StarlarkOS osObject; @@ -140,21 +148,25 @@ private interface AsyncTask { protected final double timeoutScaling; @Nullable private final ProcessWrapper processWrapper; protected final StarlarkSemantics starlarkSemantics; - private final HashMap accumulatedFileDigests = new HashMap<>(); + private final HashMap recordedFileInputs = new HashMap<>(); private final HashSet accumulatedEnvKeys = new HashSet<>(); private final RepositoryRemoteExecutor remoteExecutor; private final List asyncTasks; + private final boolean allowWatchingFilesOutsideWorkspace; protected StarlarkBaseExternalContext( Path workingDirectory, + BlazeDirectories directories, Environment env, Map envVariables, DownloadManager downloadManager, double timeoutScaling, @Nullable ProcessWrapper processWrapper, StarlarkSemantics starlarkSemantics, - @Nullable RepositoryRemoteExecutor remoteExecutor) { + @Nullable RepositoryRemoteExecutor remoteExecutor, + boolean allowWatchingFilesOutsideWorkspace) { this.workingDirectory = workingDirectory; + this.directories = directories; this.env = env; this.envVariables = ImmutableMap.copyOf(envVariables); this.osObject = new StarlarkOS(this.envVariables); @@ -164,6 +176,7 @@ protected StarlarkBaseExternalContext( this.starlarkSemantics = starlarkSemantics; this.remoteExecutor = remoteExecutor; this.asyncTasks = new ArrayList<>(); + this.allowWatchingFilesOutsideWorkspace = allowWatchingFilesOutsideWorkspace; } public boolean ensureNoPendingAsyncTasks(EventHandler eventHandler, boolean forSuccessfulFetch) { @@ -194,8 +207,8 @@ protected void registerAsyncTask(AsyncTask task) { protected abstract String getIdentifyingStringForLogging(); /** Returns the file digests used by this context object so far. */ - public ImmutableMap getAccumulatedFileDigests() { - return ImmutableMap.copyOf(accumulatedFileDigests); + public ImmutableMap getRecordedFileInputs() { + return ImmutableMap.copyOf(recordedFileInputs); } /** Returns set of environment variable keys encountered so far. */ @@ -1131,17 +1144,14 @@ public StarlarkPath path(Object path) throws EvalException, InterruptedException protected StarlarkPath getPath(String method, Object path) throws EvalException, InterruptedException { if (path instanceof String) { - PathFragment pathFragment = PathFragment.create(path.toString()); - return new StarlarkPath( - pathFragment.isAbsolute() - ? workingDirectory.getFileSystem().getPath(pathFragment) - : workingDirectory.getRelative(pathFragment)); + return new StarlarkPath(workingDirectory.getRelative(path.toString())); } else if (path instanceof Label) { return getPathFromLabel((Label) path); } else if (path instanceof StarlarkPath) { return (StarlarkPath) path; } else { - throw Starlark.errorf("%s can only take a string or a label.", method); + // This can never happen because we check it in the Starlark interpreter. + throw new IllegalArgumentException("expected string or label for path"); } } @@ -1150,22 +1160,35 @@ protected StarlarkPath getPath(String method, Object path) doc = "Reads the content of a file on the filesystem.", useStarlarkThread = true, parameters = { - @Param( - name = "path", - allowedTypes = { - @ParamType(type = String.class), - @ParamType(type = Label.class), - @ParamType(type = StarlarkPath.class) - }, - doc = "path of the file to read from."), + @Param( + name = "path", + allowedTypes = { + @ParamType(type = String.class), + @ParamType(type = Label.class), + @ParamType(type = StarlarkPath.class) + }, + doc = "path of the file to read from."), + @Param( + name = "watch", + defaultValue = "'auto'", + positional = false, + named = true, + doc = + "whether to watch the file. Can be the string 'yes', 'no', " + + "or 'auto'. Passing 'yes' is equivalent to immediately invoking the " + + "watch() method; passing 'no' does not " + + "attempt to watch the file; passing 'auto' will only attempt to watch the " + + "file when it is legal to do so (see watch() docs for more " + + "information.") }) - public String readFile(Object path, StarlarkThread thread) + public String readFile(Object path, String watch, StarlarkThread thread) throws RepositoryFunctionException, EvalException, InterruptedException { StarlarkPath p = getPath("read()", path); WorkspaceRuleEvent w = WorkspaceRuleEvent.newReadEvent( p.toString(), getIdentifyingStringForLogging(), thread.getCallerLocation()); env.getListener().post(w); + maybeWatch(p, ShouldWatch.fromString(watch)); try { return FileSystemUtils.readContent(p.getPath(), ISO_8859_1); } catch (IOException e) { @@ -1173,6 +1196,116 @@ public String readFile(Object path, StarlarkThread thread) } } + protected Pair toRootedPath(Path path) { + if (path.startsWith(directories.getWorkspace())) { + // The file is under the workspace root. + PathFragment relPath = path.relativeTo(directories.getWorkspace()); + return Pair.of( + new FileInsideWorkspace(RepositoryName.MAIN, relPath), + RootedPath.toRootedPath(Root.fromPath(directories.getWorkspace()), relPath)); + } + Path outputBaseExternal = + directories.getOutputBase().getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION); + if (path.startsWith(outputBaseExternal)) { + PathFragment relPath = path.relativeTo(outputBaseExternal); + if (!relPath.isEmpty()) { + // The file is under a repo root. + String repoName = relPath.getSegment(0); + PathFragment repoRelPath = + relPath.relativeTo(PathFragment.createAlreadyNormalized(repoName)); + return Pair.of( + new FileInsideWorkspace(RepositoryName.createUnvalidated(repoName), repoRelPath), + RootedPath.toRootedPath( + Root.fromPath(outputBaseExternal.getChild(repoName)), repoRelPath)); + } + } + // The file is just under a random absolute path. + return Pair.of( + new FileOutsideWorkspace(path.asFragment()), + RootedPath.toRootedPath(Root.absoluteRoot(path.getFileSystem()), path)); + } + + protected enum ShouldWatch { + YES, + NO, + AUTO; + + static ShouldWatch fromString(String s) throws EvalException { + switch (s) { + case "yes": + return YES; + case "no": + return NO; + case "auto": + return AUTO; + } + throw Starlark.errorf( + "bad value for 'watch' parameter; want 'yes', 'no', or 'auto', got %s", s); + } + } + + protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch) + throws EvalException, RepositoryFunctionException, InterruptedException { + if (shouldWatch == ShouldWatch.NO) { + return; + } + Path path = starlarkPath.getPath(); + Pair pair = toRootedPath(path); + if (path.startsWith(workingDirectory)) { + // The file is under the working directory. Don't watch it, as it would cause a dependency + // cycle. + if (shouldWatch == ShouldWatch.AUTO) { + return; + } + throw Starlark.errorf("attempted to watch file under working directory"); + } + if (!allowWatchingFilesOutsideWorkspace && pair.first instanceof FileOutsideWorkspace) { + if (shouldWatch == ShouldWatch.AUTO) { + return; + } + throw Starlark.errorf( + "attempted to watch file outside workspace, but it's prohibited in the current context"); + } + try { + FileValue fileValue = + (FileValue) env.getValueOrThrow(FileValue.key(pair.second), IOException.class); + if (fileValue == null) { + throw new NeedsSkyframeRestartException(); + } + if (!fileValue.isFile() || fileValue.isSpecialFile()) { + throw Starlark.errorf("Not a regular file: %s", pair.second.asPath().getPathString()); + } + + recordedFileInputs.put(pair.first, RepoRecordedInput.File.fileValueToMarkerValue(fileValue)); + } catch (IOException e) { + throw new RepositoryFunctionException(e, Transience.TRANSIENT); + } + } + + @StarlarkMethod( + name = "watch", + doc = + "Tells Bazel to watch for changes to the given file. Any changes to the file will " + + "invalidate this repository or module extension, and cause it to be refetched or " + + "re-evaluated next time.

Note that attempting to watch files inside the repo " + + "currently being fetched, or inside the working directory of the current module " + + "extension, will result in an error. A module extension attempting to watch a file " + + "outside the current Bazel workspace will also result in an error.", + parameters = { + @Param( + name = "path", + allowedTypes = { + @ParamType(type = String.class), + @ParamType(type = Label.class), + @ParamType(type = StarlarkPath.class) + }, + doc = "path of the file to watch."), + }) + public void watchForStarlark(Object path) + throws RepositoryFunctionException, EvalException, InterruptedException { + maybeWatch(getPath("watch()", path), ShouldWatch.YES); + } + // Create parent directories for the given path protected static void makeDirectories(Path path) throws IOException { Path parent = path.getParentDirectory(); @@ -1530,26 +1663,12 @@ private StarlarkPath findCommandOnPath(String program) throws IOException { // Resolve the label given by value into a file path. protected StarlarkPath getPathFromLabel(Label label) throws EvalException, InterruptedException { RootedPath rootedPath = RepositoryFunction.getRootedPathFromLabel(label, env); - SkyKey fileSkyKey = FileValue.key(rootedPath); - FileValue fileValue; + StarlarkPath starlarkPath = new StarlarkPath(rootedPath.asPath()); try { - fileValue = (FileValue) env.getValueOrThrow(fileSkyKey, IOException.class); - } catch (IOException e) { - throw Starlark.errorf("%s", e.getMessage()); - } - - if (fileValue == null) { - throw new NeedsSkyframeRestartException(); - } - if (!fileValue.isFile() || fileValue.isSpecialFile()) { - throw Starlark.errorf("Not a regular file: %s", rootedPath.asPath().getPathString()); - } - - try { - accumulatedFileDigests.put(label, RepositoryFunction.fileValueToMarkerValue(fileValue)); - } catch (IOException e) { - throw Starlark.errorf("%s", e.getMessage()); + maybeWatch(starlarkPath, ShouldWatch.AUTO); + } catch (RepositoryFunctionException e) { + throw Starlark.errorf("%s", e.getCause().getMessage()); } - return new StarlarkPath(rootedPath.asPath()); + return starlarkPath; } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPath.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPath.java index dc70f674cd0a27..8f0193410bbb4d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPath.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPath.java @@ -41,7 +41,7 @@ name = "path", category = DocCategory.BUILTIN, doc = "A structure representing a file to be used inside a repository.") -final class StarlarkPath implements StarlarkValue { +public final class StarlarkPath implements StarlarkValue { private final Path path; StarlarkPath(Path path) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index 246228875f8788..8e051c7981f0dd 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.docgen.annot.DocCategory; +import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.debug.WorkspaceRuleEvent; import com.google.devtools.build.lib.bazel.repository.DecompressorDescriptor; import com.google.devtools.build.lib.bazel.repository.DecompressorValue; @@ -74,7 +75,6 @@ public class StarlarkRepositoryContext extends StarlarkBaseExternalContext { private final Rule rule; private final RepositoryName repoName; private final PathPackageLocator packageLocator; - private final Path workspaceRoot; private final StructImpl attrObject; private final ImmutableSet ignoredPatterns; private final SyscallCache syscallCache; @@ -96,23 +96,24 @@ public class StarlarkRepositoryContext extends StarlarkBaseExternalContext { StarlarkSemantics starlarkSemantics, @Nullable RepositoryRemoteExecutor remoteExecutor, SyscallCache syscallCache, - Path workspaceRoot) + BlazeDirectories directories) throws EvalException { super( outputDirectory, + directories, environment, env, downloadManager, timeoutScaling, processWrapper, starlarkSemantics, - remoteExecutor); + remoteExecutor, + /* allowWatchingFilesOutsideWorkspace= */ true); this.rule = rule; this.repoName = RepositoryName.createUnvalidated(rule.getName()); this.packageLocator = packageLocator; this.ignoredPatterns = ignoredPatterns; this.syscallCache = syscallCache; - this.workspaceRoot = workspaceRoot; WorkspaceAttributeMapper attrs = WorkspaceAttributeMapper.of(rule); ImmutableMap.Builder attrBuilder = new ImmutableMap.Builder<>(); for (String name : attrs.getAttributeNames()) { @@ -143,7 +144,7 @@ public String getName() { structField = true, doc = "The path to the root workspace of the bazel invocation.") public StarlarkPath getWorkspaceRoot() { - return new StarlarkPath(workspaceRoot); + return new StarlarkPath(directories.getWorkspace()); } @StarlarkMethod( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java index 70dcb78cfa75b2..1800444e73b385 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java @@ -253,7 +253,7 @@ private RepositoryDirectoryValue.Builder fetchInternal( starlarkSemantics, repositoryRemoteExecutor, syscallCache, - directories.getWorkspace()); + directories); if (starlarkRepositoryContext.isRemotable()) { // If a rule is declared remotable then invalidate it if remote execution gets @@ -310,10 +310,7 @@ private RepositoryDirectoryValue.Builder fetchInternal( } // Modify marker data to include the files used by the rule's implementation function. - for (Map.Entry entry : - starlarkRepositoryContext.getAccumulatedFileDigests().entrySet()) { - recordedInputValues.put(new RepoRecordedInput.LabelFile(entry.getKey()), entry.getValue()); - } + recordedInputValues.putAll(starlarkRepositoryContext.getRecordedFileInputs()); // Ditto for environment variables accessed via `getenv`. for (String envKey : starlarkRepositoryContext.getAccumulatedEnvKeys()) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java index f0d939bee3939f..8526baeb6bbea9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java @@ -250,13 +250,16 @@ public boolean isLocal(Rule rule) { @Override public boolean verifyRecordedInputs( - Rule rule, Map recordedInputValues, Environment env) + Rule rule, + BlazeDirectories directories, + Map recordedInputValues, + Environment env) throws InterruptedException { WorkspaceAttributeMapper attributes = WorkspaceAttributeMapper.of(rule); if (attributes.isAttributeValueExplicitlySpecified("path")) { return true; } - return super.verifyRecordedInputs(rule, recordedInputValues, env); + return super.verifyRecordedInputs(rule, directories, recordedInputValues, env); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java index 7862cc453c76e5..09d9654b79660e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java @@ -179,13 +179,16 @@ public boolean isLocal(Rule rule) { @Override public boolean verifyRecordedInputs( - Rule rule, Map recordedInputValues, Environment env) + Rule rule, + BlazeDirectories directories, + Map recordedInputValues, + Environment env) throws InterruptedException { WorkspaceAttributeMapper attributes = WorkspaceAttributeMapper.of(rule); if (attributes.isAttributeValueExplicitlySpecified("path")) { return true; } - return super.verifyRecordedInputs(rule, recordedInputValues, env); + return super.verifyRecordedInputs(rule, directories, recordedInputValues, env); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BUILD index cab8c2ca49eb8b..0071154ae013a1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BUILD @@ -46,6 +46,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages/semantics", + "//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", diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index d82dc2c7e4878a..9008af56c56a3e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -307,6 +307,7 @@ java_library( deps = [ ":repository/new_local_repository_rule", ":repository/new_repository_file_handler", + ":repository/repo_recorded_input", ":repository/repository_directory_value", ":repository/repository_function", ":repository/resolved_file_value", @@ -345,6 +346,7 @@ java_library( name = "repository/new_repository_file_handler", srcs = ["repository/NewRepositoryFileHandler.java"], deps = [ + ":repository/repo_recorded_input", ":repository/repository_function", ":repository/workspace_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", @@ -360,6 +362,28 @@ java_library( ], ) +java_library( + name = "repository/repo_recorded_input", + srcs = ["repository/RepoRecordedInput.java"], + deps = [ + ":repository/repository_directory_value", + "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", + "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator", + "//src/main/java/com/google/devtools/build/lib/skyframe:action_environment_function", + "//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//third_party:guava", + "//third_party:jsr305", + ], +) + java_library( name = "repository/repository_directory_value", srcs = ["repository/RepositoryDirectoryValue.java"], @@ -382,7 +406,6 @@ java_library( srcs = [ "repository/LocalRepositoryFunction.java", "repository/NeedsSkyframeRestartException.java", - "repository/RepoRecordedInput.java", "repository/RepositoryDelegatorFunction.java", "repository/RepositoryDirectoryDirtinessChecker.java", "repository/RepositoryFunction.java", @@ -391,6 +414,7 @@ java_library( ], deps = [ ":repository/local_repository_rule", + ":repository/repo_recorded_input", ":repository/repository_directory_value", ":repository/resolved_file_value", ":repository/workspace_attribute_mapper", diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java index 8f2100ae4a6a73..4eee9852198cc8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java @@ -147,9 +147,11 @@ public void finishFile( // Link x/FILENAME to /x.FILENAME. symlinkFile(fileValue, filename, outputDirectory); try { + Label label = getFileAttributeAsLabel(rule); recordedInputValues.put( - new RepoRecordedInput.LabelFile(getFileAttributeAsLabel(rule)), - RepositoryFunction.fileValueToMarkerValue(fileValue)); + new RepoRecordedInput.FileInsideWorkspace( + label.getRepository(), label.toPathFragment()), + RepoRecordedInput.File.fileValueToMarkerValue(fileValue)); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java index 512335744419e0..4a404b36e16f01 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java @@ -14,17 +14,19 @@ package com.google.devtools.build.lib.rules.repository; +import static com.google.common.collect.ImmutableSet.toImmutableSet; + +import com.google.common.base.Preconditions; import com.google.common.base.Splitter; +import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.actions.FileValue; -import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.cmdline.LabelValidator; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction; import com.google.devtools.build.lib.skyframe.ClientEnvironmentValue; -import com.google.devtools.build.lib.skyframe.PackageLookupValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.RepositoryMappingValue; -import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; @@ -33,6 +35,7 @@ import java.io.IOException; import java.util.Comparator; import java.util.List; +import java.util.Map; import java.util.Objects; import javax.annotation.Nullable; @@ -65,7 +68,7 @@ public abstract static class Parser { * Parses a recorded input from the post-colon substring that identifies the specific input: for * example, the {@code MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. */ - public abstract RepoRecordedInput parse(String s, Path baseDirectory); + public abstract RepoRecordedInput parse(String s); } private static final Comparator COMPARATOR = @@ -76,22 +79,55 @@ public abstract static class Parser { * Parses a recorded input from its string representation. * * @param s the string representation - * @param baseDirectory the path to a base directory that any filesystem paths should be resolved - * relative to * @return The parsed recorded input object, or {@code null} if the string representation is * invalid */ @Nullable - public static RepoRecordedInput parse(String s, Path baseDirectory) { + public static RepoRecordedInput parse(String s) { List parts = Splitter.on(':').limit(2).splitToList(s); for (Parser parser : new Parser[] {File.PARSER, EnvVar.PARSER, RecordedRepoMapping.PARSER}) { if (parts.get(0).equals(parser.getPrefix())) { - return parser.parse(parts.get(1), baseDirectory); + return parser.parse(parts.get(1)); } } return null; } + /** + * Returns whether all values are still up-to-date for each recorded input. If Skyframe values are + * missing, the return value should be ignored; callers are responsible for checking {@code + * env.valuesMissing()} and triggering a Skyframe restart if needed. + */ + public static boolean areAllValuesUpToDate( + Environment env, + BlazeDirectories directories, + Map recordedInputValues) + throws InterruptedException { + env.getValuesAndExceptions( + recordedInputValues.keySet().stream() + .map(rri -> rri.getSkyKey(directories)) + .filter(Objects::nonNull) + .collect(toImmutableSet())); + if (env.valuesMissing()) { + return false; + } + for (Map.Entry recordedInputValue : + recordedInputValues.entrySet()) { + if (!recordedInputValue + .getKey() + .isUpToDate(env, directories, recordedInputValue.getValue())) { + return false; + } + } + return true; + } + + @Override + public abstract boolean equals(Object obj); + + @Override + public abstract int hashCode(); + @Override public final String toString() { return getParser().getPrefix() + ":" + toStringInternal(); @@ -106,26 +142,31 @@ public int compareTo(RepoRecordedInput o) { * Returns the post-colon substring that identifies the specific input: for example, the {@code * MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. */ - abstract String toStringInternal(); + public abstract String toStringInternal(); /** Returns the parser object for this type of recorded inputs. */ public abstract Parser getParser(); - /** Returns the {@link SkyKey} that is necessary to determine {@link #isUpToDate}. */ - public abstract SkyKey getSkyKey(); + /** + * Returns the {@link SkyKey} that is necessary to determine {@link #isUpToDate}. Can be null if + * no SkyKey is needed. + */ + @Nullable + public abstract SkyKey getSkyKey(BlazeDirectories directories); /** * Returns whether the given {@code oldValue} is still up-to-date for this recorded input. This - * method can assume that {@link #getSkyKey()} is already evaluated; it can request further - * Skyframe evaluations, and if any values are missing, this method can return any value (doesn't - * matter what) and will be reinvoked after a Skyframe restart. + * method can assume that {@link #getSkyKey(BlazeDirectories)} is already evaluated; it can + * request further Skyframe evaluations, and if any values are missing, this method can return any + * value (doesn't matter what) and will be reinvoked after a Skyframe restart. */ - public abstract boolean isUpToDate(Environment env, @Nullable String oldValue) + public abstract boolean isUpToDate( + Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException; /** Represents a file input accessed during the repo fetch. */ public abstract static class File extends RepoRecordedInput { - static final Parser PARSER = + public static final Parser PARSER = new Parser() { @Override public String getPrefix() { @@ -133,15 +174,16 @@ public String getPrefix() { } @Override - public RepoRecordedInput parse(String s, Path baseDirectory) { + public RepoRecordedInput parse(String s) { if (LabelValidator.isAbsolute(s)) { - return new LabelFile(Label.parseCanonicalUnchecked(s)); + int doubleSlash = s.indexOf("//"); + int skipAts = s.startsWith("@@") ? 2 : s.startsWith("@") ? 1 : 0; + return new FileInsideWorkspace( + RepositoryName.createUnvalidated(s.substring(skipAts, doubleSlash)), + // For backwards compatibility, treat colons as slashes. + PathFragment.create(s.substring(doubleSlash + 2).replace(':', '/'))); } - Path path = baseDirectory.getRelative(s); - return new AbsolutePathFile( - RootedPath.toRootedPath( - Root.fromPath(path.getParentDirectory()), - PathFragment.create(path.getBaseName()))); + return new FileOutsideWorkspace(PathFragment.create(s)); } }; @@ -149,42 +191,94 @@ public RepoRecordedInput parse(String s, Path baseDirectory) { public Parser getParser() { return PARSER; } + + /** + * Convert to a {@link com.google.devtools.build.lib.actions.FileValue} to a String appropriate + * for placing in a repository marker file. + * + * @param fileValue The value to convert. It must correspond to a regular file. + */ + public static String fileValueToMarkerValue(FileValue fileValue) throws IOException { + Preconditions.checkArgument(fileValue.isFile() && !fileValue.isSpecialFile()); + // Return the file content digest in hex. fileValue may or may not have the digest available. + byte[] digest = fileValue.realFileStateValue().getDigest(); + if (digest == null) { + // Fast digest not available, or it would have been in the FileValue. + digest = fileValue.realRootedPath().asPath().getDigest(); + } + return BaseEncoding.base16().lowerCase().encode(digest); + } } - /** Represents a file input accessed during the repo fetch that is addressable by a label. */ - public static final class LabelFile extends File { - final Label label; + /** + * Represents a file input accessed during the repo fetch that is within the current Bazel + * workspace. + * + *

This is almost like being addressable by a label, but includes the extra corner + * case of files inside a repo but not within any package due to missing BUILD files. For example, + * the file {@code @@foo//:abc.bzl} is addressable by a label if the file {@code @@foo//:BUILD} + * exists. But if the BUILD file doesn't exist, the {@code abc.bzl} file should still be + * "watchable"; it's just that {@code @@foo//:abc.bzl} is technically not a valid label. + */ + public static final class FileInsideWorkspace extends File { + private final RepositoryName repoName; + private final PathFragment pathFragment; + + public FileInsideWorkspace(RepositoryName repoName, PathFragment pathFragment) { + this.repoName = repoName; + this.pathFragment = pathFragment; + } - public LabelFile(Label label) { - this.label = label; + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof FileInsideWorkspace)) return false; + FileInsideWorkspace that = (FileInsideWorkspace) o; + return Objects.equals(repoName, that.repoName) + && Objects.equals(pathFragment, that.pathFragment); } @Override - String toStringInternal() { - return label.toString(); + public int hashCode() { + return Objects.hash(repoName, pathFragment); } @Override - public SkyKey getSkyKey() { - return PackageLookupValue.key(label.getPackageIdentifier()); + public String toStringInternal() { + // We store `@@foo//abc/def:ghi.bzl` as just `@@foo//abc/def/ghi.bzl`. See class javadoc for + // more context. + return repoName + "//" + pathFragment; } @Override - public boolean isUpToDate(Environment env, @Nullable String oldValue) + @Nullable + public SkyKey getSkyKey(BlazeDirectories directories) { + return repoName.isMain() ? null : RepositoryDirectoryValue.key(repoName); + } + + @Override + public boolean isUpToDate( + Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException { - PackageLookupValue pkgLookupValue = (PackageLookupValue) env.getValue(getSkyKey()); - if (pkgLookupValue == null || !pkgLookupValue.packageExists()) { - return false; + Root root; + if (repoName.isMain()) { + root = Root.fromPath(directories.getWorkspace()); + } else { + RepositoryDirectoryValue repoDirValue = + (RepositoryDirectoryValue) env.getValue(getSkyKey(directories)); + if (repoDirValue == null || !repoDirValue.repositoryExists()) { + return false; + } + root = Root.fromPath(repoDirValue.getPath()); } - RootedPath rootedPath = - RootedPath.toRootedPath(pkgLookupValue.getRoot(), label.toPathFragment()); + RootedPath rootedPath = RootedPath.toRootedPath(root, pathFragment); SkyKey fileKey = FileValue.key(rootedPath); try { FileValue fileValue = (FileValue) env.getValueOrThrow(fileKey, IOException.class); if (fileValue == null || !fileValue.isFile() || fileValue.isSpecialFile()) { return false; } - return oldValue.equals(RepositoryFunction.fileValueToMarkerValue(fileValue)); + return oldValue.equals(fileValueToMarkerValue(fileValue)); } catch (IOException e) { return false; } @@ -192,35 +286,53 @@ public boolean isUpToDate(Environment env, @Nullable String oldValue) } /** - * Represents a file input accessed during the repo fetch that is not addressable by a - * label. This most likely means that it's outside any known Bazel workspace. + * Represents a file input accessed during the repo fetch that is outside the current Bazel + * workspace. This file is addressed by its absolute path. */ - public static final class AbsolutePathFile extends File { - final RootedPath path; + public static final class FileOutsideWorkspace extends File { + private final PathFragment path; - public AbsolutePathFile(RootedPath path) { + public FileOutsideWorkspace(PathFragment path) { + Preconditions.checkArgument(path.isAbsolute()); this.path = path; } @Override - String toStringInternal() { - return path.asPath().getPathString(); + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof FileOutsideWorkspace)) return false; + FileOutsideWorkspace that = (FileOutsideWorkspace) o; + return Objects.equals(path, that.path); + } + + @Override + public int hashCode() { + return Objects.hash(path); + } + + @Override + public String toStringInternal() { + return path.getPathString(); } @Override - public SkyKey getSkyKey() { - return FileValue.key(path); + public SkyKey getSkyKey(BlazeDirectories directories) { + return FileValue.key( + RootedPath.toRootedPath( + Root.absoluteRoot(directories.getOutputBase().getFileSystem()), path)); } @Override - public boolean isUpToDate(Environment env, @Nullable String oldValue) + public boolean isUpToDate( + Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException { try { - FileValue fileValue = (FileValue) env.getValueOrThrow(getSkyKey(), IOException.class); + FileValue fileValue = + (FileValue) env.getValueOrThrow(getSkyKey(directories), IOException.class); if (fileValue == null || !fileValue.isFile() || fileValue.isSpecialFile()) { return false; } - return oldValue.equals(RepositoryFunction.fileValueToMarkerValue(fileValue)); + return oldValue.equals(fileValueToMarkerValue(fileValue)); } catch (IOException e) { return false; } @@ -237,7 +349,7 @@ public String getPrefix() { } @Override - public RepoRecordedInput parse(String s, Path baseDirectory) { + public RepoRecordedInput parse(String s) { return new EnvVar(s); } }; @@ -248,27 +360,41 @@ public EnvVar(String name) { this.name = name; } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof EnvVar)) return false; + EnvVar envVar = (EnvVar) o; + return Objects.equals(name, envVar.name); + } + + @Override + public int hashCode() { + return Objects.hash(name); + } + @Override public Parser getParser() { return PARSER; } @Override - String toStringInternal() { + public String toStringInternal() { return name; } @Override - public SkyKey getSkyKey() { + public SkyKey getSkyKey(BlazeDirectories directories) { return ActionEnvironmentFunction.key(name); } @Override - public boolean isUpToDate(Environment env, @Nullable String oldValue) + public boolean isUpToDate( + Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException { String v = PrecomputedValue.REPO_ENV.get(env).get(name); if (v == null) { - v = ((ClientEnvironmentValue) env.getValue(getSkyKey())).getValue(); + v = ((ClientEnvironmentValue) env.getValue(getSkyKey(directories))).getValue(); } // Note that `oldValue` can be null if the env var was not set. return Objects.equals(oldValue, v); @@ -285,7 +411,7 @@ public String getPrefix() { } @Override - public RepoRecordedInput parse(String s, Path baseDirectory) { + public RepoRecordedInput parse(String s) { List parts = Splitter.on(',').limit(2).splitToList(s); return new RecordedRepoMapping( RepositoryName.createUnvalidated(parts.get(0)), parts.get(1)); @@ -300,25 +426,41 @@ public RecordedRepoMapping(RepositoryName sourceRepo, String apparentName) { this.apparentName = apparentName; } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof RecordedRepoMapping)) return false; + RecordedRepoMapping that = (RecordedRepoMapping) o; + return Objects.equals(sourceRepo, that.sourceRepo) + && Objects.equals(apparentName, that.apparentName); + } + + @Override + public int hashCode() { + return Objects.hash(sourceRepo, apparentName); + } + @Override public Parser getParser() { return PARSER; } @Override - String toStringInternal() { + public String toStringInternal() { return sourceRepo.getName() + ',' + apparentName; } @Override - public SkyKey getSkyKey() { + public SkyKey getSkyKey(BlazeDirectories directories) { return RepositoryMappingValue.key(sourceRepo); } @Override - public boolean isUpToDate(Environment env, @Nullable String oldValue) + public boolean isUpToDate( + Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException { - RepositoryMappingValue repoMappingValue = (RepositoryMappingValue) env.getValue(getSkyKey()); + RepositoryMappingValue repoMappingValue = + (RepositoryMappingValue) env.getValue(getSkyKey(directories)); return repoMappingValue != RepositoryMappingValue.NOT_FOUND_VALUE && RepositoryName.createUnvalidated(oldValue) .equals(repoMappingValue.getRepositoryMapping().get(apparentName)); diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index 3839956b709e64..979cbd9df3b7e6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java @@ -93,7 +93,7 @@ public final class RepositoryDelegatorFunction implements SkyFunction { // The marker file version is inject in the rule key digest so the rule key is always different // when we decide to update the format. - private static final int MARKER_FILE_VERSION = 4; + private static final int MARKER_FILE_VERSION = 5; // Mapping of rule class name to RepositoryFunction. private final ImmutableMap handlers; @@ -589,6 +589,7 @@ static String unescape(String str) { } private static class DigestWriter { + private final BlazeDirectories directories; private final Path markerPath; private final Rule rule; // not just Map<> to signal that iteration order must be deterministic @@ -600,6 +601,7 @@ private static class DigestWriter { RepositoryName repositoryName, Rule rule, StarlarkSemantics starlarkSemantics) { + this.directories = directories; ruleKey = computeRuleKey(rule, starlarkSemantics); markerPath = getMarkerPath(directories, repositoryName.getName()); this.rule = rule; @@ -648,15 +650,14 @@ byte[] areRepositoryAndMarkerFileConsistent( return null; } - Path baseDirectory = rule.getPackage().getPackageDirectory(); Map recordedInputValues = new TreeMap<>(); String content; try { content = FileSystemUtils.readContent(markerPath, UTF_8); - String markerRuleKey = readMarkerFile(content, baseDirectory, recordedInputValues); + String markerRuleKey = readMarkerFile(content, recordedInputValues); boolean verified = false; if (Preconditions.checkNotNull(ruleKey).equals(markerRuleKey)) { - verified = handler.verifyRecordedInputs(rule, recordedInputValues, env); + verified = handler.verifyRecordedInputs(rule, directories, recordedInputValues, env); if (env.valuesMissing()) { return null; } @@ -678,7 +679,7 @@ Map getRecordedInputValues() { @Nullable private static String readMarkerFile( - String content, Path baseDirectory, Map recordedInputValues) { + String content, Map recordedInputValues) { String markerRuleKey = null; Iterable lines = Splitter.on('\n').split(content); @@ -690,8 +691,7 @@ private static String readMarkerFile( } else { int sChar = line.indexOf(' '); if (sChar > 0) { - RepoRecordedInput input = - RepoRecordedInput.parse(unescape(line.substring(0, sChar)), baseDirectory); + RepoRecordedInput input = RepoRecordedInput.parse(unescape(line.substring(0, sChar))); if (input == null) { // ignore invalid entries. continue; diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java index 9aa7f860412b9e..1533eaca58c46e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java @@ -15,14 +15,10 @@ package com.google.devtools.build.lib.rules.repository; import static com.google.common.base.Preconditions.checkState; -import static com.google.common.collect.ImmutableSet.toImmutableSet; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; -import com.google.common.io.BaseEncoding; -import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.cmdline.Label; @@ -191,38 +187,12 @@ public abstract RepositoryDirectoryValue.Builder fetch( * is needed. */ public boolean verifyRecordedInputs( - Rule rule, Map recordedInputValues, Environment env) + Rule rule, + BlazeDirectories directories, + Map recordedInputValues, + Environment env) throws InterruptedException { - ImmutableSet skyKeys = - recordedInputValues.keySet().stream() - .map(RepoRecordedInput::getSkyKey) - .collect(toImmutableSet()); - env.getValuesAndExceptions(skyKeys); - if (env.valuesMissing()) { - return false; - } - for (Map.Entry recordedInputValue : recordedInputValues.entrySet()) { - if (!recordedInputValue.getKey().isUpToDate(env, recordedInputValue.getValue())) { - return false; - } - } - return true; - } - /** - * Convert to a {@link com.google.devtools.build.lib.actions.FileValue} to a String appropriate - * for placing in a repository marker file. - * - * @param fileValue The value to convert. It must correspond to a regular file. - */ - public static String fileValueToMarkerValue(FileValue fileValue) throws IOException { - Preconditions.checkArgument(fileValue.isFile() && !fileValue.isSpecialFile()); - // Return the file content digest in hex. fileValue may or may not have the digest available. - byte[] digest = fileValue.realFileStateValue().getDigest(); - if (digest == null) { - // Fast digest not available, or it would have been in the FileValue. - digest = fileValue.realRootedPath().asPath().getDigest(); - } - return BaseEncoding.base16().lowerCase().encode(digest); + return RepoRecordedInput.areAllValuesUpToDate(env, directories, recordedInputValues); } public static RootedPath getRootedPathFromLabel(Label label, Environment env) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD index ab616727be0ce1..5a4dae694ace81 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD @@ -21,6 +21,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib:runtime", "//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:server_directories", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/bazel/repository/starlark", "//src/main/java/com/google/devtools/build/lib/cmdline", @@ -69,6 +70,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib:runtime", "//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:server_directories", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/bazel/repository/starlark", "//src/main/java/com/google/devtools/build/lib/cmdline", diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java index 655e21734d7c40..81bcbf43735667 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java @@ -25,6 +25,9 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import com.google.common.io.CharStreams; +import com.google.devtools.build.lib.analysis.BlazeDirectories; +import com.google.devtools.build.lib.analysis.ServerDirectories; +import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager; import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.events.ExtendedEventHandler; @@ -82,6 +85,7 @@ public final class StarlarkRepositoryContextTest { private Scratch scratch; + private Path outputBase; private Path outputDirectory; private Root root; private Path workspaceFile; @@ -94,6 +98,7 @@ public final class StarlarkRepositoryContextTest { @Before public void setUp() throws Exception { scratch = new Scratch("/"); + outputBase = scratch.dir("/outputBase"); outputDirectory = scratch.dir("/outputDir"); root = Root.fromPath(scratch.dir("/wsRoot")); workspaceFile = scratch.file("/wsRoot/WORKSPACE"); @@ -159,6 +164,12 @@ private void setUpContextForRule( outputDirectory, ImmutableList.of(root), BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY); + BlazeDirectories directories = + new BlazeDirectories( + new ServerDirectories(root.asPath(), outputBase, root.asPath()), + root.asPath(), + /* defaultSystemJavabase= */ null, + AnalysisMock.get().getProductName()); context = new StarlarkRepositoryContext( rule, @@ -169,11 +180,11 @@ private void setUpContextForRule( envVariables, downloader, 1.0, - /*processWrapper=*/ null, + /* processWrapper= */ null, starlarkSemantics, repoRemoteExecutor, SyscallCache.NO_CACHE, - root.asPath()); + directories); } private void setUpContextForRule(String name) throws Exception { @@ -304,7 +315,7 @@ public void testRead() throws Exception { setUpContextForRule("test"); context.createFile(context.path("foo/bar"), "foobar", true, true, thread); - String content = context.readFile(context.path("foo/bar"), thread); + String content = context.readFile(context.path("foo/bar"), true, thread); assertThat(content).isEqualTo("foobar"); } diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/BUILD b/src/test/java/com/google/devtools/build/lib/rules/repository/BUILD index bea028bcb01688..ba165c318ed011 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/BUILD @@ -34,6 +34,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/rules:repository/local_repository_rule", + "//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_compile", diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java index 9a7f175d633e10..84bb5478ce15ee 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java @@ -127,7 +127,7 @@ public void testFileValueToMarkerValue() throws Exception { // Digest should be returned if the FileStateValue has it. FileStateValue fsv = new RegularFileStateValueWithDigest(3, new byte[] {1, 2, 3, 4}); FileValue fv = new RegularFileValue(path, fsv); - assertThat(RepositoryFunction.fileValueToMarkerValue(fv)).isEqualTo("01020304"); + assertThat(RepoRecordedInput.File.fileValueToMarkerValue(fv)).isEqualTo("01020304"); // Digest should also be returned if the FileStateValue doesn't have it. FileStatus status = Mockito.mock(FileStatus.class); @@ -136,6 +136,6 @@ public void testFileValueToMarkerValue() throws Exception { fsv = new RegularFileStateValueWithContentsProxy(3, FileContentsProxy.create(status)); fv = new RegularFileValue(path, fsv); String expectedDigest = BaseEncoding.base16().lowerCase().encode(path.asPath().getDigest()); - assertThat(RepositoryFunction.fileValueToMarkerValue(fv)).isEqualTo(expectedDigest); + assertThat(RepoRecordedInput.File.fileValueToMarkerValue(fv)).isEqualTo(expectedDigest); } } diff --git a/src/test/py/bazel/bzlmod/bazel_fetch_test.py b/src/test/py/bazel/bzlmod/bazel_fetch_test.py index 2981549af4bdad..5bf97be567499a 100644 --- a/src/test/py/bazel/bzlmod/bazel_fetch_test.py +++ b/src/test/py/bazel/bzlmod/bazel_fetch_test.py @@ -17,6 +17,7 @@ import os import tempfile from absl.testing import absltest + from src.test.py.bazel import test_base from src.test.py.bazel.bzlmod.test_utils import BazelRegistry @@ -234,7 +235,7 @@ def testForceFetch(self): 'extension.bzl', [ 'def _repo_rule_impl(ctx):', - ' file_content = ctx.read("' + file_path + '").strip()', + ' file_content = ctx.read("' + file_path + '", watch=False)', ' print(file_content)', ' ctx.file("BUILD")', 'repo_rule = repository_rule(implementation=_repo_rule_impl)', diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py index d94bd55675a8cc..850c26d75b8516 100644 --- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py +++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py @@ -16,8 +16,10 @@ import json import os +import pathlib import tempfile from absl.testing import absltest + from src.test.py.bazel import test_base from src.test.py.bazel.bzlmod.test_utils import BazelRegistry from src.test.py.bazel.bzlmod.test_utils import scratchFile @@ -1934,6 +1936,62 @@ def testReproducibleExtensionInLockfileErrorMode(self): stderr, ) + def testWatchingFileUnderWorkingDirectoryFails(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'ext = use_extension("extension.bzl", "ext")', + 'use_repo(ext, "repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _ext_impl(ctx):', + ' ctx.file("hello", "repo")', + ' repo_rule(name=ctx.read("hello", watch="yes"))', + 'ext = module_extension(implementation=_ext_impl)', + ], + ) + + _, _, stderr = self.RunBazel(['build', '@repo'], allow_failure=True) + self.assertIn('attempted to watch file under working directory', + '\n'.join(stderr)) + + def testWatchingFileOutsideWorkspaceFails(self): + outside_file = pathlib.Path(self._temp).joinpath('bleh') + scratchFile(outside_file, ['repo']) + self.ScratchFile( + 'MODULE.bazel', + [ + 'ext = use_extension("extension.bzl", "ext")', + 'use_repo(ext, "repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _ext_impl(ctx):', + ' repo_rule(name=ctx.read(%s, watch="yes"))' % repr(str( + outside_file)), + 'ext = module_extension(implementation=_ext_impl)', + ], + ) + + _, _, stderr = self.RunBazel(['build', '@repo'], allow_failure=True) + self.assertIn('attempted to watch file outside workspace', + '\n'.join(stderr)) + if __name__ == '__main__': absltest.main() diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index b7df798d38d5d6..f5826d94f75f86 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2704,4 +2704,154 @@ EOF expect_log "I see: @@bar~//:data" } +function test_file_watching_inside_working_dir() { + # when reading a file inside the working directory (where the repo + # is to be fetched), we shouldn't watch it. + create_new_workspace + cat > MODULE.bazel < r.bzl <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: nothing" + + # Running Bazel again shouldn't cause a refetch, despite "data.txt" + # having been written to after the read. + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_not_log "I see:" +} + +function test_file_watching_inside_working_dir_forcing_error() { + # when reading a file inside the working directory (where the repo + # is to be fetched), we shouldn't watch it. Forcing the watch should + # result in an error. + create_new_workspace + cat > MODULE.bazel < r.bzl <& $TEST_log && fail "expected bazel to fail" + expect_log "attempted to watch file under working directory" +} + +function test_file_watching_outside_workspace() { + # when reading a file outside the Bazel workspace, we should watch it. + local outside_dir="${TEST_TMPDIR}/outside_dir" + mkdir -p "${outside_dir}" + echo nothing > ${outside_dir}/data.txt + + create_new_workspace + cat > MODULE.bazel < r.bzl <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: nothing" + + # Running Bazel again shouldn't cause a refetch. + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_not_log "I see:" + + # But changing the outside file should cause a refetch. + echo something > ${outside_dir}/data.txt + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: something" +} + +function test_file_watching_in_other_repo() { + # when reading a file in another repo, we should watch it. + local outside_dir="${TEST_TMPDIR}/outside_dir" + mkdir -p "${outside_dir}" + echo nothing > ${outside_dir}/data.txt + + create_new_workspace + cat > MODULE.bazel < r.bzl <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: nothing" + + # Running Bazel again shouldn't cause a refetch. + bazel build @foo >& $TEST_log || fail "expected bazel to succeed" + expect_not_log "I see:" + + # But changing the file inside @bar should cause @foo to refetch. + cat > MODULE.bazel <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: something" +} + +function test_file_watching_in_other_repo_cycle() { + create_new_workspace + cat > MODULE.bazel < r.bzl <& $TEST_log && fail "expected bazel to fail!" + expect_log "Circular definition of repositories" +} + run_suite "local repository tests" diff --git a/src/test/tools/bzlmod/MODULE.bazel.lock b/src/test/tools/bzlmod/MODULE.bazel.lock index b8c8061982c520..25946507c9318e 100644 --- a/src/test/tools/bzlmod/MODULE.bazel.lock +++ b/src/test/tools/bzlmod/MODULE.bazel.lock @@ -1,5 +1,5 @@ { - "lockFileVersion": 4, + "lockFileVersion": 5, "moduleFileHash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", "flags": { "cmdRegistries": [ @@ -1037,7 +1037,7 @@ "@@apple_support~//crosstool:setup.bzl%apple_cc_configure_extension": { "general": { "bzlTransitiveDigest": "pMLFCYaRPkgXPQ8vtuNkMfiHfPmRBy6QJfnid4sWfv0=", - "accumulatedFileDigests": {}, + "recordedFileInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "local_config_apple_cc": { @@ -1063,7 +1063,7 @@ "@@bazel_tools//tools/android:android_extensions.bzl%remote_android_tools_extensions": { "general": { "bzlTransitiveDigest": "vsrPPBNf8OgywAYLMcIL1oNm2R8WtbCIL9wgQBUecpA=", - "accumulatedFileDigests": {}, + "recordedFileInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "android_tools": { @@ -1089,7 +1089,7 @@ "@@bazel_tools//tools/cpp:cc_configure.bzl%cc_configure_extension": { "general": { "bzlTransitiveDigest": "XWy8pzw7/6RclAFWd6/VfUdoXn2SdSpmHOrbfEFJ1ao=", - "accumulatedFileDigests": {}, + "recordedFileInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "local_config_cc": { @@ -1115,7 +1115,7 @@ "@@bazel_tools//tools/osx:xcode_configure.bzl%xcode_configure_extension": { "general": { "bzlTransitiveDigest": "Qh2bWTU6QW6wkrd87qrU4YeY+SG37Nvw3A0PR4Y0L2Y=", - "accumulatedFileDigests": {}, + "recordedFileInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "local_config_xcode": { @@ -1133,7 +1133,7 @@ "@@bazel_tools//tools/sh:sh_configure.bzl%sh_configure_extension": { "general": { "bzlTransitiveDigest": "hp4NgmNjEg5+xgvzfh6L83bt9/aiiWETuNpwNuF1MSU=", - "accumulatedFileDigests": {}, + "recordedFileInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "local_config_sh": { @@ -1148,7 +1148,7 @@ "@@bazel_tools//tools/test:extensions.bzl%remote_coverage_tools_extension": { "general": { "bzlTransitiveDigest": "AL+K5m+GCP3XRzLPqpKAq4GsjIVDXgUveWm8nih4ju0=", - "accumulatedFileDigests": {}, + "recordedFileInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "remote_coverage_tools": { @@ -1168,7 +1168,7 @@ "@@buildozer~//:buildozer_binary.bzl%buildozer_binary": { "general": { "bzlTransitiveDigest": "EleDU/FQ1+e/RgkW3aIDmdaxZEthvoWQhsqFTxiSgMI=", - "accumulatedFileDigests": {}, + "recordedFileInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "buildozer_binary": { @@ -1192,7 +1192,7 @@ "@@rules_java~//java:extensions.bzl%toolchains": { "general": { "bzlTransitiveDigest": "Bm4ulErUcJjZtKeAt2etkB6sGO8evCgHQxbw4Px8q60=", - "accumulatedFileDigests": {}, + "recordedFileInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "remotejdk21_linux_toolchain_config_repo": { @@ -1696,8 +1696,8 @@ "@@rules_jvm_external~//:extensions.bzl%maven": { "general": { "bzlTransitiveDigest": "yXprMX4LqzJwuZlbtT9WNeu7p2iEYw7j4R1NP9pc4Ow=", - "accumulatedFileDigests": { - "@@rules_jvm_external~//:rules_jvm_external_deps_install.json": "10442a5ae27d9ff4c2003e5ab71643bf0d8b48dcf968b4173fa274c3232a8c06" + "recordedFileInputs": { + "@@rules_jvm_external~//rules_jvm_external_deps_install.json": "10442a5ae27d9ff4c2003e5ab71643bf0d8b48dcf968b4173fa274c3232a8c06" }, "envVariables": {}, "generatedRepoSpecs": { @@ -2718,7 +2718,7 @@ "@@rules_jvm_external~//:non-module-deps.bzl%non_module_deps": { "general": { "bzlTransitiveDigest": "Td87llNSs5GZ/kAxu6pAqfEXBZ3HdkSqRmUzvIfbFWg=", - "accumulatedFileDigests": {}, + "recordedFileInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "io_bazel_rules_kotlin": { @@ -2744,7 +2744,7 @@ "@@rules_python~//python/extensions:python.bzl%python": { "general": { "bzlTransitiveDigest": "NGtTMUqs2EEJeXu6mXdpmYRrQGZiJV7S3mxeod3Hm7M=", - "accumulatedFileDigests": {}, + "recordedFileInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "pythons_hub": { @@ -2772,7 +2772,7 @@ "@@rules_python~//python/extensions/private:internal_deps.bzl%internal_deps": { "general": { "bzlTransitiveDigest": "5c1tkdV6L67SQOZWc9MUoS5ZnsSxeDKsh9urs01jZSM=", - "accumulatedFileDigests": {}, + "recordedFileInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "pypi__coverage_cp39_aarch64-apple-darwin": {