diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index 51c607af4352de..02832ad230bd9f 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -2741,7 +2741,7 @@ "general": { "bzlTransitiveDigest": "r8gQnSLwon27gWD77J8mb3DIe4v3gtn7J/rsic53Qyw=", "accumulatedFileDigests": { - "@@//src/test/tools/bzlmod:MODULE.bazel.lock": "e06d281fdc21f520f6c489fd021f6393bb5f94d6fb3dc6aeaa3214bd614f8266", + "@@//src/test/tools/bzlmod:MODULE.bazel.lock": "90bec989ef7d229ef67c24ad140987a20c64343695a944fc04c7266815f89ade", "@@//:MODULE.bazel": "c440045e7f8f6ff70a870646e1d3502afe2b63b4f51d794d0b6bfa6cc4b3c007" }, "envVariables": {}, 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 0855f0a1aa74e2..3c85644a59a996 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 = 5; + public static final int LOCK_FILE_VERSION = 6; @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 2a4d28d960cee4..a2b62f2789ccc4 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 @@ -459,6 +459,22 @@ public RepoRecordedInput.File read(JsonReader jsonReader) throws IOException { } }; + private static final TypeAdapter + 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() @@ -484,6 +500,8 @@ public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) { .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(); } 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 d526a79edacdd0..64e401ed6f224b 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 @@ -43,6 +43,8 @@ public static Builder builder() { public abstract ImmutableMap getRecordedFileInputs(); + public abstract ImmutableMap getRecordedDirentsInputs(); + public abstract ImmutableMap getEnvVariables(); public abstract ImmutableMap getGeneratedRepoSpecs(); @@ -68,6 +70,9 @@ public abstract static class Builder { public abstract Builder setRecordedFileInputs( ImmutableMap value); + public abstract Builder setRecordedDirentsInputs( + ImmutableMap value); + public abstract Builder setEnvVariables(ImmutableMap value); public abstract Builder setGeneratedRepoSpecs(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 52151259200462..1314114a770d86 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 @@ -72,7 +72,7 @@ protected ModuleExtensionContext( processWrapper, starlarkSemantics, remoteExecutor, - /* allowWatchingFilesOutsideWorkspace= */ false); + /* allowWatchingPathsOutsideWorkspace= */ 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 17506457a5b307..040723437a5b2d 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 @@ -218,6 +218,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) LockFileModuleExtension.builder() .setBzlTransitiveDigest(extension.getBzlTransitiveDigest()) .setRecordedFileInputs(moduleExtensionResult.getRecordedFileInputs()) + .setRecordedDirentsInputs(moduleExtensionResult.getRecordedDirentsInputs()) .setEnvVariables(extension.getEnvVars()) .setGeneratedRepoSpecs(generatedRepoSpecs) .setModuleExtensionMetadata(moduleExtensionMetadata) @@ -289,10 +290,16 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile( + extensionId + "' have changed"); } - if (didFilesChange(env, directories, lockedExtension.getRecordedFileInputs())) { + 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 } @@ -390,12 +397,12 @@ private static boolean didRepoMappingsChange( return false; } - private static boolean didFilesChange( + private static boolean didRecordedInputsChange( Environment env, BlazeDirectories directories, - ImmutableMap recordedFileInputs) + ImmutableMap recordedInputs) throws InterruptedException, NeedsSkyframeRestartException { - boolean upToDate = RepoRecordedInput.areAllValuesUpToDate(env, directories, recordedFileInputs); + boolean upToDate = RepoRecordedInput.areAllValuesUpToDate(env, directories, recordedInputs); if (env.valuesMissing()) { throw new NeedsSkyframeRestartException(); } @@ -762,6 +769,7 @@ public RunModuleExtensionResult run( generatedRepoSpecs.put(name, repoSpec); } return RunModuleExtensionResult.create( + ImmutableMap.of(), ImmutableMap.of(), generatedRepoSpecs.buildOrThrow(), Optional.of(ModuleExtensionMetadata.REPRODUCIBLE), @@ -931,6 +939,7 @@ public RunModuleExtensionResult run( } return RunModuleExtensionResult.create( moduleContext.getRecordedFileInputs(), + moduleContext.getRecordedDirentsInputs(), threadContext.getGeneratedRepoSpecs(), moduleExtensionMetadata, repoMappingRecorder.recordedEntries()); @@ -993,6 +1002,8 @@ abstract static class RunModuleExtensionResult { abstract ImmutableMap getRecordedFileInputs(); + abstract ImmutableMap getRecordedDirentsInputs(); + abstract ImmutableMap getGeneratedRepoSpecs(); abstract Optional getModuleExtensionMetadata(); @@ -1001,11 +1012,13 @@ abstract static class RunModuleExtensionResult { static RunModuleExtensionResult create( ImmutableMap recordedFileInputs, + ImmutableMap recordedDirentsInputs, ImmutableMap generatedRepoSpecs, Optional moduleExtensionMetadata, ImmutableTable recordedRepoMappingEntries) { return new AutoValue_SingleExtensionEvalFunction_RunModuleExtensionResult( recordedFileInputs, + recordedDirentsInputs, generatedRepoSpecs, moduleExtensionMetadata, recordedRepoMappingEntries); 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 3329e3a0354e67..b0f34b7bac8755 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 @@ -42,6 +42,7 @@ java_library( "//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: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", 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 9f332744d7d69e..aca498083d16ac 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 @@ -51,21 +51,20 @@ 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.RepoRecordedInput.Dirents; +import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.RepoCacheFriendlyPath; 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; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor.ExecutionResult; import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction; +import com.google.devtools.build.lib.skyframe.DirectoryListingValue; 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; @@ -149,10 +148,11 @@ private interface AsyncTask { @Nullable private final ProcessWrapper processWrapper; protected final StarlarkSemantics starlarkSemantics; private final HashMap recordedFileInputs = new HashMap<>(); + private final HashMap recordedDirentsInputs = new HashMap<>(); private final HashSet accumulatedEnvKeys = new HashSet<>(); private final RepositoryRemoteExecutor remoteExecutor; private final List asyncTasks; - private final boolean allowWatchingFilesOutsideWorkspace; + private final boolean allowWatchingPathsOutsideWorkspace; protected StarlarkBaseExternalContext( Path workingDirectory, @@ -164,7 +164,7 @@ protected StarlarkBaseExternalContext( @Nullable ProcessWrapper processWrapper, StarlarkSemantics starlarkSemantics, @Nullable RepositoryRemoteExecutor remoteExecutor, - boolean allowWatchingFilesOutsideWorkspace) { + boolean allowWatchingPathsOutsideWorkspace) { this.workingDirectory = workingDirectory; this.directories = directories; this.env = env; @@ -176,7 +176,7 @@ protected StarlarkBaseExternalContext( this.starlarkSemantics = starlarkSemantics; this.remoteExecutor = remoteExecutor; this.asyncTasks = new ArrayList<>(); - this.allowWatchingFilesOutsideWorkspace = allowWatchingFilesOutsideWorkspace; + this.allowWatchingPathsOutsideWorkspace = allowWatchingPathsOutsideWorkspace; } public boolean ensureNoPendingAsyncTasks(EventHandler eventHandler, boolean forSuccessfulFetch) { @@ -211,6 +211,10 @@ public ImmutableMap getRecordedFileInputs() { return ImmutableMap.copyOf(recordedFileInputs); } + public ImmutableMap getRecordedDirentsInputs() { + return ImmutableMap.copyOf(recordedDirentsInputs); + } + /** Returns set of environment variable keys encountered so far. */ public ImmutableSet getAccumulatedEnvKeys() { return ImmutableSet.copyOf(accumulatedEnvKeys); @@ -1144,7 +1148,7 @@ public StarlarkPath path(Object path) throws EvalException, InterruptedException protected StarlarkPath getPath(String method, Object path) throws EvalException, InterruptedException { if (path instanceof String) { - return new StarlarkPath(workingDirectory.getRelative(path.toString())); + return new StarlarkPath(this, workingDirectory.getRelative(path.toString())); } else if (path instanceof Label) { return getPathFromLabel((Label) path); } else if (path instanceof StarlarkPath) { @@ -1199,13 +1203,30 @@ public String readFile(Object path, String watch, StarlarkThread thread) } } - protected Pair toRootedPath(Path path) { + /** + * Converts a regular {@link Path} to a {@link RepoCacheFriendlyPath} based on {@link + * ShouldWatch}. If the path shouldn't be watched for whatever reason, returns null. If it's + * illegal to watch the path in the current context, but the user still requested a watch, throws + * an exception. + */ + @Nullable + protected RepoCacheFriendlyPath toRepoCacheFriendlyPath(Path path, ShouldWatch shouldWatch) + throws EvalException { + if (shouldWatch == ShouldWatch.NO) { + return null; + } + if (path.startsWith(workingDirectory)) { + // The path is under the working directory. Don't watch it, as it would cause a dependency + // cycle. + if (shouldWatch == ShouldWatch.AUTO) { + return null; + } + throw Starlark.errorf("attempted to watch path under working directory"); + } 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)); + return RepoCacheFriendlyPath.createInsideWorkspace(RepositoryName.MAIN, relPath); } Path outputBaseExternal = directories.getOutputBase().getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION); @@ -1216,16 +1237,19 @@ protected Pair toRootedPath(Path path) { 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)); + return RepoCacheFriendlyPath.createInsideWorkspace( + RepositoryName.createUnvalidated(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)); + if (!allowWatchingPathsOutsideWorkspace) { + if (shouldWatch == ShouldWatch.AUTO) { + return null; + } + throw Starlark.errorf( + "attempted to watch path outside workspace, but it's prohibited in the current context"); + } + return RepoCacheFriendlyPath.createOutsideWorkspace(path.asFragment()); } /** Whether to watch a path. See {@link #readFile} for semantics */ @@ -1251,34 +1275,47 @@ static ShouldWatch fromString(String s) throws EvalException { protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch) throws EvalException, RepositoryFunctionException, InterruptedException { - if (shouldWatch == ShouldWatch.NO) { + RepoCacheFriendlyPath repoCacheFriendlyPath = + toRepoCacheFriendlyPath(starlarkPath.getPath(), shouldWatch); + if (repoCacheFriendlyPath == null) { 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"); + RootedPath rootedPath = repoCacheFriendlyPath.getRootedPath(env, directories); + if (rootedPath == null) { + throw new NeedsSkyframeRestartException(); } try { FileValue fileValue = - (FileValue) env.getValueOrThrow(FileValue.key(pair.second), IOException.class); + (FileValue) env.getValueOrThrow(FileValue.key(rootedPath), IOException.class); if (fileValue == null) { throw new NeedsSkyframeRestartException(); } - recordedFileInputs.put(pair.first, RepoRecordedInput.File.fileValueToMarkerValue(fileValue)); + recordedFileInputs.put( + new RepoRecordedInput.File(repoCacheFriendlyPath), + RepoRecordedInput.File.fileValueToMarkerValue(fileValue)); + } catch (IOException e) { + throw new RepositoryFunctionException(e, Transience.TRANSIENT); + } + } + + protected void maybeWatchDirents(Path path, ShouldWatch shouldWatch) + throws EvalException, RepositoryFunctionException, InterruptedException { + RepoCacheFriendlyPath repoCacheFriendlyPath = toRepoCacheFriendlyPath(path, shouldWatch); + if (repoCacheFriendlyPath == null) { + return; + } + RootedPath rootedPath = repoCacheFriendlyPath.getRootedPath(env, directories); + if (env.valuesMissing()) { + throw new NeedsSkyframeRestartException(); + } + if (env.getValue(DirectoryListingValue.key(rootedPath)) == null) { + throw new NeedsSkyframeRestartException(); + } + try { + recordedDirentsInputs.put( + new RepoRecordedInput.Dirents(repoCacheFriendlyPath), + RepoRecordedInput.Dirents.getDirentsMarkerValue(path)); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } @@ -1294,12 +1331,11 @@ protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch) + "(if the path is a file); if the path was a file but is now a directory, or vice " + "versa; and if the path starts or stops existing. Notably, this does not " + "include changes to any files under the directory if the path is a directory. For " - // TODO: add `watch_dir()` - + "that, use watch_dir() instead.

Note " - + "that attempting to watch paths 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 path outside the current Bazel workspace " - + "will also result in an error.", + + "that, use path.readdir() " + + "instead.

Note that attempting to watch paths 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 path outside the " + + "current Bazel workspace will also result in an error.", parameters = { @Param( name = "path", @@ -1662,7 +1698,7 @@ private StarlarkPath findCommandOnPath(String program) throws IOException { // root?). Path path = workingDirectory.getFileSystem().getPath(fragment).getChild(program.trim()); if (path.exists() && path.isFile(Symlinks.FOLLOW) && path.isExecutable()) { - return new StarlarkPath(path); + return new StarlarkPath(this, path); } } } @@ -1672,7 +1708,7 @@ 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); - StarlarkPath starlarkPath = new StarlarkPath(rootedPath.asPath()); + StarlarkPath starlarkPath = new StarlarkPath(this, rootedPath.asPath()); try { maybeWatch(starlarkPath, ShouldWatch.AUTO); } catch (RepositoryFunctionException e) { 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 4823c294c1188a..3e676ecfdb8d78 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 @@ -16,9 +16,12 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.docgen.annot.DocCategory; +import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkBaseExternalContext.ShouldWatch; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; +import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import java.io.IOException; import javax.annotation.Nullable; import net.starlark.java.annot.Param; @@ -27,14 +30,15 @@ import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Printer; import net.starlark.java.eval.Sequence; +import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkValue; import net.starlark.java.eval.Tuple; /** - * A Path object to be used into Starlark remote repository. + * A Path object to be used in repo rules and module extensions. * *

This path object enable non-hermetic operations from Starlark and should not be returned by - * something other than a StarlarkRepositoryContext. + * something other than a StarlarkBaseExternalContext. */ @Immutable @StarlarkBuiltin( @@ -42,9 +46,11 @@ category = DocCategory.BUILTIN, doc = "A structure representing a file to be used inside a repository.") public final class StarlarkPath implements StarlarkValue { + private final StarlarkBaseExternalContext ctx; private final Path path; - StarlarkPath(Path path) { + StarlarkPath(StarlarkBaseExternalContext ctx, Path path) { + this.ctx = ctx; this.path = path; } @@ -77,13 +83,41 @@ public String getBasename() { @StarlarkMethod( name = "readdir", - doc = "The list of entries in the directory denoted by this path.") - public ImmutableList readdir() throws IOException { - ImmutableList.Builder builder = ImmutableList.builder(); - for (Path p : path.getDirectoryEntries()) { - builder.add(new StarlarkPath(p)); + doc = + "Returns the list of entries in the directory denoted by this path. Each entry is a " + + "path object itself.", + parameters = { + @Param( + name = "watch", + defaultValue = "'auto'", + positional = false, + named = true, + doc = + "whether Bazel should watch the list of entries in this directory and refetch the " + + "repository or re-evaluate the module extension next time when any changes " + + "are detected. Changes to detect include entry creation, deletion, and " + + "renaming. Note that this doesn't watch the contents of any entries " + + "in the directory.

Can be the string 'yes', 'no', or 'auto'. If set to " + + "'auto', Bazel will only watch this directory when it is legal to do so (see " + + "repository_ctx.watch() " + + "docs for more information)."), + }) + public ImmutableList readdir(String watch) + throws EvalException, RepositoryFunctionException, InterruptedException { + if (!isDir()) { + throw Starlark.errorf("can't readdir(), not a directory: %s", path); + } + ctx.maybeWatchDirents(path, ShouldWatch.fromString(watch)); + try { + ImmutableList.Builder builder = ImmutableList.builder(); + for (Path p : path.getDirectoryEntries()) { + builder.add(new StarlarkPath(ctx, p)); + } + return builder.build(); + } catch (IOException e) { + throw new RepositoryFunctionException(e, Transience.TRANSIENT); } - return builder.build(); } @StarlarkMethod( @@ -94,7 +128,7 @@ public ImmutableList readdir() throws IOException { @Nullable public StarlarkPath getDirname() { Path parentPath = path.getParentDirectory(); - return parentPath == null ? null : new StarlarkPath(parentPath); + return parentPath == null ? null : new StarlarkPath(ctx, parentPath); } @StarlarkMethod( @@ -108,6 +142,7 @@ public StarlarkPath getDirname() { + "added as needed.")) public StarlarkPath getChild(Tuple relativePaths) throws EvalException { return new StarlarkPath( + ctx, path.getRelative( String.join( Character.toString(PathFragment.SEPARATOR_CHAR), @@ -145,7 +180,7 @@ public boolean isDir() { "Returns the canonical path for this path by repeatedly replacing all symbolic links " + "with their referents.") public StarlarkPath realpath() throws IOException { - return new StarlarkPath(path.resolveSymbolicLinks()); + return new StarlarkPath(ctx, path.resolveSymbolicLinks()); } @Override 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 6d69087374669d..f6a32cdef62b5e 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 @@ -33,14 +33,19 @@ import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.repository.RepositoryFetchProgress; 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.Dirents; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException; import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper; import com.google.devtools.build.lib.runtime.ProcessWrapper; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; +import com.google.devtools.build.lib.skyframe.DirectoryListingValue; import com.google.devtools.build.lib.util.StringUtilities; 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.SyscallCache; import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; @@ -48,6 +53,7 @@ import java.io.OutputStream; import java.nio.charset.StandardCharsets; import java.nio.file.InvalidPathException; +import java.util.HashMap; import java.util.Map; import javax.annotation.Nullable; import net.starlark.java.annot.Param; @@ -108,7 +114,7 @@ public class StarlarkRepositoryContext extends StarlarkBaseExternalContext { processWrapper, starlarkSemantics, remoteExecutor, - /* allowWatchingFilesOutsideWorkspace= */ true); + /* allowWatchingPathsOutsideWorkspace= */ true); this.rule = rule; this.repoName = RepositoryName.createUnvalidated(rule.getName()); this.packageLocator = packageLocator; @@ -144,7 +150,7 @@ public String getName() { structField = true, doc = "The path to the root workspace of the bazel invocation.") public StarlarkPath getWorkspaceRoot() { - return new StarlarkPath(directories.getWorkspace()); + return new StarlarkPath(this, 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 1800444e73b385..d350edef1cff03 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 @@ -309,8 +309,9 @@ private RepositoryDirectoryValue.Builder fetchInternal( env.getListener().handle(Event.debug(defInfo)); } - // Modify marker data to include the files used by the rule's implementation function. + // Modify marker data to include the files/dirents used by the rule's implementation function. recordedInputValues.putAll(starlarkRepositoryContext.getRecordedFileInputs()); + recordedInputValues.putAll(starlarkRepositoryContext.getRecordedDirentsInputs()); // Ditto for environment variables accessed via `getenv`. for (String envKey : starlarkRepositoryContext.getAccumulatedEnvKeys()) { 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 46972d36aa2b00..c3c100c5ea4a68 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -373,12 +373,15 @@ java_library( "//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:directory_listing_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/util", "//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:auto_value", "//third_party:guava", "//third_party:jsr305", ], 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 4eee9852198cc8..aad74b69b07a8f 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 @@ -149,8 +149,9 @@ public void finishFile( try { Label label = getFileAttributeAsLabel(rule); recordedInputValues.put( - new RepoRecordedInput.FileInsideWorkspace( - label.getRepository(), label.toPathFragment()), + new RepoRecordedInput.File( + RepoRecordedInput.RepoCacheFriendlyPath.createInsideWorkspace( + 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 78ee43978d31de..9c8a6369109119 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,8 +14,10 @@ package com.google.devtools.build.lib.rules.repository; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; import com.google.common.io.BaseEncoding; @@ -25,8 +27,11 @@ 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.DirectoryListingValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.RepositoryMappingValue; +import com.google.devtools.build.lib.util.Fingerprint; +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; @@ -37,6 +42,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import javax.annotation.Nullable; /** @@ -85,7 +91,8 @@ public abstract static class Parser { @Nullable 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}) { + for (Parser parser : + new Parser[] {File.PARSER, Dirents.PARSER, EnvVar.PARSER, RecordedRepoMapping.PARSER}) { if (parts.get(0).equals(parser.getPrefix())) { return parser.parse(parts.get(1)); } @@ -164,8 +171,96 @@ 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 { + /** + * Represents a filesystem path stored in a way that is repo-cache-friendly. That is, if the path + * happens to point inside the current Bazel workspace (in either the main repo or an external + * repo), we store the appropriate repo name and the path fragment relative to the repo root, + * instead of the entire absolute path. + * + *

This is almost like storing 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. + * + *

Of course, when the path is outside the current Bazel workspace, we just store the absolute + * path. + */ + @AutoValue + public abstract static class RepoCacheFriendlyPath { + public abstract Optional repoName(); + + public abstract PathFragment path(); + + public static RepoCacheFriendlyPath createInsideWorkspace( + RepositoryName repoName, PathFragment path) { + Preconditions.checkArgument( + !path.isAbsolute(), "the provided path should be relative to the repo root"); + return new AutoValue_RepoRecordedInput_RepoCacheFriendlyPath(Optional.of(repoName), path); + } + + public static RepoCacheFriendlyPath createOutsideWorkspace(PathFragment path) { + Preconditions.checkArgument( + path.isAbsolute(), "the provided path should be absolute in the filesystem"); + return new AutoValue_RepoRecordedInput_RepoCacheFriendlyPath(Optional.empty(), path); + } + + @Override + public final String toString() { + // We store `@@foo//abc/def:ghi.bzl` as just `@@foo//abc/def/ghi.bzl`. See class javadoc for + // more context. + return repoName().map(repoName -> repoName + "//" + path()).orElse(path().toString()); + } + + public static RepoCacheFriendlyPath parse(String s) { + if (LabelValidator.isAbsolute(s)) { + int doubleSlash = s.indexOf("//"); + int skipAts = s.startsWith("@@") ? 2 : s.startsWith("@") ? 1 : 0; + return createInsideWorkspace( + RepositoryName.createUnvalidated(s.substring(skipAts, doubleSlash)), + PathFragment.create(s.substring(doubleSlash + 2))); + } + return createOutsideWorkspace(PathFragment.create(s)); + } + + /** + * If resolving this path requires getting a {@link RepositoryDirectoryValue}, this method + * returns the appropriate key; otherwise it returns null. + */ + @Nullable + public final RepositoryDirectoryValue.Key getRepoDirSkyKeyOrNull() { + if (repoName().isEmpty() || repoName().get().isMain()) { + return null; + } + return RepositoryDirectoryValue.key(repoName().get()); + } + + public final RootedPath getRootedPath(Environment env, BlazeDirectories directories) + throws InterruptedException { + Root root; + if (repoName().isEmpty()) { + root = Root.absoluteRoot(directories.getOutputBase().getFileSystem()); + } else if (repoName().get().isMain()) { + root = Root.fromPath(directories.getWorkspace()); + } else { + RepositoryDirectoryValue repoDirValue = + (RepositoryDirectoryValue) env.getValue(getRepoDirSkyKeyOrNull()); + if (repoDirValue == null || !repoDirValue.repositoryExists()) { + return null; + } + root = Root.fromPath(repoDirValue.getPath()); + } + return RootedPath.toRootedPath(root, path()); + } + } + + /** + * Represents a file input accessed during the repo fetch. Despite being named just "file", this + * can represent a file or a directory on the filesystem, and it does not need to exist. The value + * of the input contains whether this is a file or a directory or nonexistent, and if it's a file, + * the digest of its contents. + */ + public static final class File extends RepoRecordedInput { public static final Parser PARSER = new Parser() { @Override @@ -175,23 +270,43 @@ public String getPrefix() { @Override public RepoRecordedInput parse(String s) { - if (LabelValidator.isAbsolute(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(':', '/'))); - } - return new FileOutsideWorkspace(PathFragment.create(s)); + return new File(RepoCacheFriendlyPath.parse(s)); } }; + private final RepoCacheFriendlyPath path; + + public File(RepoCacheFriendlyPath path) { + this.path = path; + } + @Override public Parser getParser() { return PARSER; } + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof File)) { + return false; + } + File that = (File) o; + return Objects.equals(path, that.path); + } + + @Override + public int hashCode() { + return path.hashCode(); + } + + @Override + public String toStringInternal() { + return path.toString(); + } + /** * Convert to a {@link com.google.devtools.build.lib.actions.FileValue} to a String appropriate * for placing in a repository marker file. The file need not exist, and can be a file or a @@ -212,78 +327,25 @@ public static String fileValueToMarkerValue(FileValue fileValue) throws IOExcept } return BaseEncoding.base16().lowerCase().encode(digest); } - } - - /** - * 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; - } - - @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 - public int hashCode() { - return Objects.hash(repoName, pathFragment); - } - - @Override - 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 @Nullable public SkyKey getSkyKey(BlazeDirectories directories) { - return repoName.isMain() ? null : RepositoryDirectoryValue.key(repoName); + return path.getRepoDirSkyKeyOrNull(); } @Override public boolean isUpToDate( Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException { - 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 = path.getRootedPath(env, directories); + if (rootedPath == null) { + return false; } - 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()) { + if (fileValue == null) { return false; } return oldValue.equals(fileValueToMarkerValue(fileValue)); @@ -293,15 +355,24 @@ public boolean isUpToDate( } } - /** - * 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 FileOutsideWorkspace extends File { - private final PathFragment path; + /** Represents the list of entries under a directory accessed during the fetch. */ + public static final class Dirents extends RepoRecordedInput { + public static final Parser PARSER = + new Parser() { + @Override + public String getPrefix() { + return "DIRENTS"; + } + + @Override + public RepoRecordedInput parse(String s) { + return new Dirents(RepoCacheFriendlyPath.parse(s)); + } + }; + + private final RepoCacheFriendlyPath path; - public FileOutsideWorkspace(PathFragment path) { - Preconditions.checkArgument(path.isAbsolute()); + public Dirents(RepoCacheFriendlyPath path) { this.path = path; } @@ -310,10 +381,10 @@ public boolean equals(Object o) { if (this == o) { return true; } - if (!(o instanceof FileOutsideWorkspace)) { + if (!(o instanceof Dirents)) { return false; } - FileOutsideWorkspace that = (FileOutsideWorkspace) o; + Dirents that = (Dirents) o; return Objects.equals(path, that.path); } @@ -324,31 +395,47 @@ public int hashCode() { @Override public String toStringInternal() { - return path.getPathString(); + return path.toString(); } + @Override + public Parser getParser() { + return PARSER; + } + + @Nullable @Override public SkyKey getSkyKey(BlazeDirectories directories) { - return FileValue.key( - RootedPath.toRootedPath( - Root.absoluteRoot(directories.getOutputBase().getFileSystem()), path)); + return path.getRepoDirSkyKeyOrNull(); } @Override public boolean isUpToDate( Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException { + RootedPath rootedPath = path.getRootedPath(env, directories); + if (rootedPath == null) { + return false; + } + if (env.getValue(DirectoryListingValue.key(rootedPath)) == null) { + return false; + } try { - FileValue fileValue = - (FileValue) env.getValueOrThrow(getSkyKey(directories), IOException.class); - if (fileValue == null || !fileValue.isFile() || fileValue.isSpecialFile()) { - return false; - } - return oldValue.equals(fileValueToMarkerValue(fileValue)); + return oldValue.equals(getDirentsMarkerValue(rootedPath.asPath())); } catch (IOException e) { return false; } } + + public static String getDirentsMarkerValue(Path path) throws IOException { + Fingerprint fp = new Fingerprint(); + fp.addStrings( + path.getDirectoryEntries().stream() + .map(Path::getBaseName) + .sorted() + .collect(toImmutableList())); + return fp.hexDigestAndReset(); + } } /** Represents an environment variable accessed during the repo fetch. */ 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 05dd374c08a588..00d961ed838068 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 = 6; + private static final int MARKER_FILE_VERSION = 7; // Mapping of rule class name to RepositoryFunction. private final ImmutableMap handlers; diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPathTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPathTest.java index e6ebb596d7ebda..138c170ea9bb77 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPathTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPathTest.java @@ -35,16 +35,20 @@ public class StarlarkPathTest { private final FileSystem fs = new InMemoryFileSystem(DigestHashFunction.SHA256); private final Path wd = FileSystemUtils.getWorkingDirectory(fs); + private static StarlarkPath makePath(Path path) { + return new StarlarkPath(/* ctx= */ null, path); + } + @Before public void setup() throws Exception { - ev.update("wd", new StarlarkPath(wd)); + ev.update("wd", makePath(wd)); } @Test public void testStarlarkPathGetChild() throws Exception { - assertThat(ev.eval("wd.get_child()")).isEqualTo(new StarlarkPath(wd)); - assertThat(ev.eval("wd.get_child('foo')")).isEqualTo(new StarlarkPath(wd.getChild("foo"))); + assertThat(ev.eval("wd.get_child()")).isEqualTo(makePath(wd)); + assertThat(ev.eval("wd.get_child('foo')")).isEqualTo(makePath(wd.getChild("foo"))); assertThat(ev.eval("wd.get_child('a','b/c','/d/')")) - .isEqualTo(new StarlarkPath(wd.getRelative("a/b/c/d"))); + .isEqualTo(makePath(wd.getRelative("a/b/c/d"))); } } 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 d18e197cf0e4f2..10294a996430d8 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 @@ -479,7 +479,7 @@ public void testDirectoryListing() throws Exception { scratch.file("/my/folder/a"); scratch.file("/my/folder/b"); scratch.file("/my/folder/c"); - assertThat(context.path("/my/folder").readdir()).containsExactly( + assertThat(context.path("/my/folder").readdir("no")).containsExactly( context.path("/my/folder/a"), context.path("/my/folder/b"), context.path("/my/folder/c")); } diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py index 72110e2159f6ce..3689b53008fe0d 100644 --- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py +++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py @@ -19,6 +19,7 @@ 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 @@ -1960,11 +1961,12 @@ def testWatchingFileUnderWorkingDirectoryFails(self): _, _, stderr = self.RunBazel(['build', '@repo'], allow_failure=True) self.assertIn( - 'attempted to watch file under working directory', '\n'.join(stderr) + 'attempted to watch path under working directory', '\n'.join(stderr) ) def testWatchingFileOutsideWorkspaceFails(self): - outside_file = pathlib.Path(self._temp).joinpath('bleh') + outside_file = pathlib.Path(tempfile.mkdtemp(dir=self._temp)).joinpath( + 'hello') scratchFile(outside_file, ['repo']) self.ScratchFile( 'MODULE.bazel', @@ -1990,7 +1992,115 @@ def testWatchingFileOutsideWorkspaceFails(self): _, _, stderr = self.RunBazel(['build', '@repo'], allow_failure=True) self.assertIn( - 'attempted to watch file outside workspace', '\n'.join(stderr) + 'attempted to watch path outside workspace', '\n'.join(stderr) + ) + + def testPathReaddirWatchesDirents(self): + self.ScratchFile('some_dir/foo') + self.ScratchFile('some_dir/bar') + self.ScratchFile('some_dir/baz') + 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=\'repo\')")', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _ext_impl(ctx):', + ' print("I see: " + ",".join(sorted([e.basename for e in ctx.path(%s).readdir()])))' % repr( + self.Path('some_dir')), + ' repo_rule(name="repo")', + 'ext = module_extension(implementation=_ext_impl)', + ], + ) + + _, _, stderr = self.RunBazel(['build', '@repo']) + self.assertIn('I see: bar,baz,foo', '\n'.join(stderr)) + + # adding and removing entries should cause a reevaluation. + os.remove(self.Path('some_dir/baz')) + self.ScratchFile('some_dir/quux') + _, _, stderr = self.RunBazel(['build', '@repo']) + self.assertIn('I see: bar,foo,quux', '\n'.join(stderr)) + + # but changing file contents shouldn't. + self.ScratchFile('some_dir/bar', ['hulloooo']) + _, _, stderr = self.RunBazel(['build', '@repo']) + self.assertNotIn('I see:', '\n'.join(stderr)) + + def testPathReaddirOutsideWorkspaceDoesNotWatchDirents(self): + outside_dir = pathlib.Path(tempfile.mkdtemp(dir=self._temp)) + scratchFile(outside_dir.joinpath('whatever'), ['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=\'repo\')")', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _ext_impl(ctx):', + ' print("I see: " + ",".join(sorted([e.basename for e in ctx.path(%s).readdir()])))' % repr( + str(outside_dir)), + ' repo_rule(name="repo")', + 'ext = module_extension(implementation=_ext_impl)', + ], + ) + + _, _, stderr = self.RunBazel(['build', '@repo']) + self.assertIn('I see: whatever', '\n'.join(stderr)) + + # since the directory in question is outside the workspace, adding and + # removing entries shouldn't cause a reevaluation. + os.remove(outside_dir.joinpath('whatever')) + scratchFile(outside_dir.joinpath('anything'), ['kek']) + _, _, stderr = self.RunBazel(['build', '@repo']) + self.assertNotIn('I see:', '\n'.join(stderr)) + + def testForceWatchingDirentsOutsideWorkspaceFails(self): + outside_dir = pathlib.Path(tempfile.mkdtemp(dir=self._temp)) + scratchFile(outside_dir.joinpath('whatever'), ['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=\'repo\')")', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _ext_impl(ctx):', + ' print("I see: " + ",".join(sorted([e.basename for e in ctx.path(%s).readdir(watch="yes")])))' % repr( + str(outside_dir)), + ' repo_rule(name="repo")', + 'ext = module_extension(implementation=_ext_impl)', + ], + ) + + _, _, stderr = self.RunBazel(['build', '@repo'], allow_failure=True) + self.assertIn( + 'attempted to watch path outside workspace', '\n'.join(stderr) ) diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 7134697a91cbc6..06e70124d12120 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2750,7 +2750,7 @@ r=repository_rule(_r) EOF bazel build @r >& $TEST_log && fail "expected bazel to fail" - expect_log "attempted to watch file under working directory" + expect_log "attempted to watch path under working directory" } function test_file_watching_outside_workspace() { @@ -3000,4 +3000,54 @@ EOF expect_log "I see: bleh" } +function test_path_readdir_watches_dirents() { + local outside_dir=$(mktemp -d "${TEST_TMPDIR}/testXXXXXX") + touch ${outside_dir}/foo + touch ${outside_dir}/bar + touch ${outside_dir}/baz + + create_new_workspace + cat > MODULE.bazel < r.bzl <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: bar,baz,foo" + + # changing the contents of a file under there shouldn't trigger a refetch. + echo haha > ${outside_dir}/foo + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_not_log "I see:" + + # adding a file should trigger a refetch. + touch ${outside_dir}/quux + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: bar,baz,foo,quux" + + # removing a file should trigger a refetch. + rm ${outside_dir}/baz + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: bar,foo,quux" + + # changing a file to a directory shouldn't trigger a refetch. + rm ${outside_dir}/bar + mkdir ${outside_dir}/bar + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_not_log "I see:" + + # changing entries in subdirectories shouldn't trigger a refetch. + touch ${outside_dir}/bar/inner + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_not_log "I see:" +} + run_suite "local repository tests" diff --git a/src/test/tools/bzlmod/MODULE.bazel.lock b/src/test/tools/bzlmod/MODULE.bazel.lock index aa436d97185405..be7dcb5ce953f7 100644 --- a/src/test/tools/bzlmod/MODULE.bazel.lock +++ b/src/test/tools/bzlmod/MODULE.bazel.lock @@ -1,5 +1,5 @@ { - "lockFileVersion": 5, + "lockFileVersion": 6, "moduleFileHash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", "flags": { "cmdRegistries": [ @@ -1038,6 +1038,7 @@ "general": { "bzlTransitiveDigest": "pMLFCYaRPkgXPQ8vtuNkMfiHfPmRBy6QJfnid4sWfv0=", "recordedFileInputs": {}, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "local_config_apple_cc": { @@ -1064,6 +1065,7 @@ "general": { "bzlTransitiveDigest": "vsrPPBNf8OgywAYLMcIL1oNm2R8WtbCIL9wgQBUecpA=", "recordedFileInputs": {}, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "android_tools": { @@ -1090,6 +1092,7 @@ "general": { "bzlTransitiveDigest": "XWy8pzw7/6RclAFWd6/VfUdoXn2SdSpmHOrbfEFJ1ao=", "recordedFileInputs": {}, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "local_config_cc": { @@ -1116,6 +1119,7 @@ "general": { "bzlTransitiveDigest": "Qh2bWTU6QW6wkrd87qrU4YeY+SG37Nvw3A0PR4Y0L2Y=", "recordedFileInputs": {}, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "local_config_xcode": { @@ -1134,6 +1138,7 @@ "general": { "bzlTransitiveDigest": "hp4NgmNjEg5+xgvzfh6L83bt9/aiiWETuNpwNuF1MSU=", "recordedFileInputs": {}, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "local_config_sh": { @@ -1149,6 +1154,7 @@ "general": { "bzlTransitiveDigest": "AL+K5m+GCP3XRzLPqpKAq4GsjIVDXgUveWm8nih4ju0=", "recordedFileInputs": {}, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "remote_coverage_tools": { @@ -1169,6 +1175,7 @@ "general": { "bzlTransitiveDigest": "EleDU/FQ1+e/RgkW3aIDmdaxZEthvoWQhsqFTxiSgMI=", "recordedFileInputs": {}, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "buildozer_binary": { @@ -1193,6 +1200,7 @@ "general": { "bzlTransitiveDigest": "Vo8tBC4aLZTSWFbox69vCUJ2B0S7GYQDr5txjiyYfgQ=", "recordedFileInputs": {}, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "remotejdk21_linux_toolchain_config_repo": { @@ -1699,6 +1707,7 @@ "recordedFileInputs": { "@@rules_jvm_external~//rules_jvm_external_deps_install.json": "10442a5ae27d9ff4c2003e5ab71643bf0d8b48dcf968b4173fa274c3232a8c06" }, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "org_slf4j_slf4j_api_1_7_30": { @@ -2719,6 +2728,7 @@ "general": { "bzlTransitiveDigest": "Td87llNSs5GZ/kAxu6pAqfEXBZ3HdkSqRmUzvIfbFWg=", "recordedFileInputs": {}, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "io_bazel_rules_kotlin": { @@ -2745,6 +2755,7 @@ "general": { "bzlTransitiveDigest": "NGtTMUqs2EEJeXu6mXdpmYRrQGZiJV7S3mxeod3Hm7M=", "recordedFileInputs": {}, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "pythons_hub": { @@ -2773,6 +2784,7 @@ "general": { "bzlTransitiveDigest": "5c1tkdV6L67SQOZWc9MUoS5ZnsSxeDKsh9urs01jZSM=", "recordedFileInputs": {}, + "recordedDirentsInputs": {}, "envVariables": {}, "generatedRepoSpecs": { "pypi__coverage_cp39_aarch64-apple-darwin": {