Fix watching paths in undefined repos in repo rules

Wyverald · copybara-github · commit aba1186aa25e · 2024-03-05T10:53:52.000-08:00
Watching a path in an undefined repo (eg. `$output_base/external/i_am_undefined/blah`) currently causes an NPE because we expect it to need a skyframe restart when it actually doesn't. The underlying reason is rather convoluted, but the gist is that we request `RepositoryDirectoryValue.Key(@@i_am_undefined)` explicitly, when we don't actually have to -- we can simply use `FileValue.Key($output_base/external/i_am_undefined/blah` directly, and rely on the fact that `FileFunction` knows to request the corresponding `RepositoryDirectoryValue` and behave appropriately when the repo is undefined. (This is due to logic in `ExternalFilesHelper.maybeHandleExternalFile`.) Additionally, this PR removes the `rctx.symlink(watch_target=)` parameter. The symlink target's existence and its contents should not affect the creation of the symlink at all, so there's no reason to ever watch it. Fixes #21483. Closes #21562. PiperOrigin-RevId: 612898526 Change-Id: I30deb9d9b2d19e0869517f4b6b126078446745b4
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
@@ -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
@@ -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 <a href=\"#watch\">watch</a> the symlink target. Can be the string "
-                    + "'yes', 'no', or 'auto'. Passing 'yes' is equivalent to immediately invoking "
-                    + "the <a href=\"#watch\"><code>watch()</code></a> 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 <code>watch()</code> 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
@@ -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
@@ -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
