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 aca498083d16ac..441b34abcda0da 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 @@ -59,7 +59,6 @@ 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.io.OutErr; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -1280,20 +1279,16 @@ protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch) if (repoCacheFriendlyPath == null) { return; } - RootedPath rootedPath = repoCacheFriendlyPath.getRootedPath(env, directories); - if (rootedPath == null) { - throw new NeedsSkyframeRestartException(); - } try { + var recordedInput = new RepoRecordedInput.File(repoCacheFriendlyPath); FileValue fileValue = - (FileValue) env.getValueOrThrow(FileValue.key(rootedPath), IOException.class); + (FileValue) env.getValueOrThrow(recordedInput.getSkyKey(directories), IOException.class); if (fileValue == null) { throw new NeedsSkyframeRestartException(); } recordedFileInputs.put( - new RepoRecordedInput.File(repoCacheFriendlyPath), - RepoRecordedInput.File.fileValueToMarkerValue(fileValue)); + recordedInput, RepoRecordedInput.File.fileValueToMarkerValue(fileValue)); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } @@ -1305,17 +1300,13 @@ protected void maybeWatchDirents(Path path, ShouldWatch shouldWatch) if (repoCacheFriendlyPath == null) { return; } - RootedPath rootedPath = repoCacheFriendlyPath.getRootedPath(env, directories); - if (env.valuesMissing()) { - throw new NeedsSkyframeRestartException(); - } - if (env.getValue(DirectoryListingValue.key(rootedPath)) == null) { + var recordedInput = new RepoRecordedInput.Dirents(repoCacheFriendlyPath); + if (env.getValue(recordedInput.getSkyKey(directories)) == null) { throw new NeedsSkyframeRestartException(); } try { recordedDirentsInputs.put( - new RepoRecordedInput.Dirents(repoCacheFriendlyPath), - RepoRecordedInput.Dirents.getDirentsMarkerValue(path)); + recordedInput, RepoRecordedInput.Dirents.getDirentsMarkerValue(path)); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } 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 764c35e14435d7..ad28d3119926d3 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 @@ -44,7 +44,6 @@ 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.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; @@ -209,20 +208,8 @@ private StarlarkPath externalPath(String method, Object pathObject) @ParamType(type = StarlarkPath.class) }, doc = "The path of the symlink to create."), - @Param( - name = "watch_target", - defaultValue = "'auto'", - positional = false, - named = true, - doc = - "whether to watch the symlink target. 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 path; passing 'auto' will only attempt to watch " - + "the file when it is legal to do so (see watch() docs for more " - + "information."), }) - public void symlink(Object target, Object linkName, String watchTarget, StarlarkThread thread) + public void symlink(Object target, Object linkName, StarlarkThread thread) throws RepositoryFunctionException, EvalException, InterruptedException { StarlarkPath targetPath = getPath("symlink()", target); StarlarkPath linkPath = getPath("symlink()", linkName); @@ -233,7 +220,6 @@ public void symlink(Object target, Object linkName, String watchTarget, Starlark getIdentifyingStringForLogging(), thread.getCallerLocation()); env.getListener().post(w); - maybeWatch(targetPath, ShouldWatch.fromString(watchTarget)); try { checkInOutputDirectory("write", linkPath); makeDirectories(linkPath.getPath()); @@ -605,20 +591,16 @@ public void watchTree(Object path) if (repoCacheFriendlyPath == null) { return; } - RootedPath rootedPath = repoCacheFriendlyPath.getRootedPath(env, directories); - if (rootedPath == null) { - throw new NeedsSkyframeRestartException(); - } try { + var recordedInput = new RepoRecordedInput.DirTree(repoCacheFriendlyPath); DirectoryTreeDigestValue digestValue = (DirectoryTreeDigestValue) - env.getValueOrThrow(DirectoryTreeDigestValue.key(rootedPath), IOException.class); + env.getValueOrThrow(recordedInput.getSkyKey(directories), IOException.class); if (digestValue == null) { throw new NeedsSkyframeRestartException(); } - recordedDirTreeInputs.put( - new RepoRecordedInput.DirTree(repoCacheFriendlyPath), digestValue.hexDigest()); + recordedDirTreeInputs.put(recordedInput, digestValue.hexDigest()); } 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 4bef44e2efed2b..b5b9a4d99b4b72 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 @@ -23,6 +23,7 @@ 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.cmdline.LabelConstants; import com.google.devtools.build.lib.cmdline.LabelValidator; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction; @@ -115,8 +116,7 @@ public static boolean areAllValuesUpToDate( throws InterruptedException { env.getValuesAndExceptions( recordedInputValues.keySet().stream() - .map(RepoRecordedInput::getSkyKey) - .filter(Objects::nonNull) + .map(rri -> rri.getSkyKey(directories)) .collect(toImmutableSet())); if (env.valuesMissing()) { return false; @@ -157,18 +157,14 @@ public int compareTo(RepoRecordedInput o) { /** 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}. Can be null if - * no SkyKey is needed. - */ - @Nullable - public abstract SkyKey getSkyKey(); + /** Returns the {@link SkyKey} that is necessary to determine {@link #isUpToDate}. */ + 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, BlazeDirectories directories, @Nullable String oldValue) @@ -226,32 +222,25 @@ public static RepoCacheFriendlyPath parse(String s) { 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 { + /** Returns the rooted path corresponding to this "repo-friendly path". */ + public final RootedPath getRootedPath(BlazeDirectories directories) { 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()); + // This path is from an external repo. We just directly fabricate the path here instead of + // requesting the appropriate RepositoryDirectoryValue, since we can rely on the various + // other SkyFunctions (such as FileStateFunction and DirectoryListingStateFunction) to do + // that for us instead. This also sidesteps an awkward situation when the external repo in + // question is not defined. + root = + Root.fromPath( + directories + .getOutputBase() + .getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION) + .getRelative(repoName().get().getName())); } return RootedPath.toRootedPath(root, path()); } @@ -331,23 +320,18 @@ public static String fileValueToMarkerValue(FileValue fileValue) throws IOExcept return BaseEncoding.base16().lowerCase().encode(digest); } - @Override @Nullable - public SkyKey getSkyKey() { - return path.getRepoDirSkyKeyOrNull(); + public SkyKey getSkyKey(BlazeDirectories directories) { + return FileValue.key(path.getRootedPath(directories)); } @Override public boolean isUpToDate( Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException { - RootedPath rootedPath = path.getRootedPath(env, directories); - if (rootedPath == null) { - return false; - } - SkyKey fileKey = FileValue.key(rootedPath); try { - FileValue fileValue = (FileValue) env.getValueOrThrow(fileKey, IOException.class); + FileValue fileValue = + (FileValue) env.getValueOrThrow(getSkyKey(directories), IOException.class); if (fileValue == null) { return false; } @@ -406,25 +390,22 @@ public Parser getParser() { return PARSER; } - @Nullable @Override - public SkyKey getSkyKey() { - return path.getRepoDirSkyKeyOrNull(); + public SkyKey getSkyKey(BlazeDirectories directories) { + return DirectoryListingValue.key(path.getRootedPath(directories)); } @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) { + SkyKey skyKey = getSkyKey(directories); + if (env.getValue(skyKey) == null) { return false; } try { - return oldValue.equals(getDirentsMarkerValue(rootedPath.asPath())); + return oldValue.equals( + getDirentsMarkerValue(((DirectoryListingValue.Key) skyKey).argument().asPath())); } catch (IOException e) { return false; } @@ -493,22 +474,17 @@ public Parser getParser() { return PARSER; } - @Nullable @Override - public SkyKey getSkyKey() { - return path.getRepoDirSkyKeyOrNull(); + public SkyKey getSkyKey(BlazeDirectories directories) { + return DirectoryTreeDigestValue.key(path.getRootedPath(directories)); } @Override public boolean isUpToDate( Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException { - RootedPath rootedPath = path.getRootedPath(env, directories); - if (rootedPath == null) { - return false; - } DirectoryTreeDigestValue value = - (DirectoryTreeDigestValue) env.getValue(DirectoryTreeDigestValue.key(rootedPath)); + (DirectoryTreeDigestValue) env.getValue(getSkyKey(directories)); if (value == null) { return false; } @@ -565,7 +541,7 @@ public String toStringInternal() { } @Override - public SkyKey getSkyKey() { + public SkyKey getSkyKey(BlazeDirectories directories) { return ActionEnvironmentFunction.key(name); } @@ -575,7 +551,7 @@ public boolean isUpToDate( 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); @@ -636,7 +612,7 @@ public String toStringInternal() { } @Override - public SkyKey getSkyKey() { + public SkyKey getSkyKey(BlazeDirectories directories) { // Since we only record repo mapping entries for repos defined in Bzlmod, we can request the // WORKSPACE-less version of the main repo mapping (as no repos defined in Bzlmod can see // stuff from WORKSPACE). @@ -649,7 +625,8 @@ public SkyKey getSkyKey() { 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/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 3509a0105488f4..b679a29dee5505 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 @@ -459,7 +459,7 @@ public void testSymlink() throws Exception { setUpContextForRule("test"); context.createFile(context.path("foo"), "foobar", true, true, thread); - context.symlink(context.path("foo"), context.path("bar"), "auto", thread); + context.symlink(context.path("foo"), context.path("bar"), thread); testOutputFile(outputDirectory.getChild("bar"), "foobar"); assertThat(context.path("bar").realpath()).isEqualTo(context.path("foo")); diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index b3feef46fec2c1..d05261dc69565f 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -501,7 +501,7 @@ function test_starlark_repository_environ() { def _impl(repository_ctx): print(repository_ctx.os.environ["FOO"]) # Symlink so a repository is created - repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path(""), watch_target="no") + repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path("")) repo = repository_rule(implementation=_impl, local=False) EOF @@ -544,7 +544,7 @@ EOF def _impl(repository_ctx): print(repository_ctx.os.environ["BAR"]) # Symlink so a repository is created - repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path(""), watch_target="no") + repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path("")) repo = repository_rule(implementation=_impl, local=True) EOF BAR=BEZ bazel build @foo//:bar >& $TEST_log || fail "Failed to build" @@ -556,7 +556,7 @@ EOF def _impl(repository_ctx): print(repository_ctx.os.environ["BAZ"]) # Symlink so a repository is created - repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path(""), watch_target="no") + repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path("")) repo = repository_rule(implementation=_impl, local=True) EOF BAZ=BOZ bazel build @foo//:bar >& $TEST_log || fail "Failed to build" @@ -2867,6 +2867,46 @@ EOF expect_log "I see: something" } +function test_file_watching_in_undefined_repo() { + create_new_workspace + cat > MODULE.bazel < foo.bzl <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see something!" + + # Defining @@_main~_repo_rules~bar should now cause @foo to refetch. + cat >> MODULE.bazel < bar.bzl <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see something!" + + # However, just adding a random other repo shouldn't alert @foo. + cat >> MODULE.bazel <& $TEST_log || fail "expected bazel to succeed" + expect_not_log "I see something!" +} + function test_file_watching_in_other_repo_cycle() { create_new_workspace cat > MODULE.bazel <