Track repo rule label attributes after the first non-existent one

Even with this commit, the fact that a particular label in a repository rule's label attributes does not resolve to a regular file is not tracked, which means that there is still a potential for incorrect incremental fetches.

Work towards #13441

Closes #18352.

PiperOrigin-RevId: 531150549
Change-Id: I086e4813ca88a3f49cde77d9be20adaaf954834b

Author: fmeum

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java

@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.packages.StructProvider;
 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.RepositoryFunction.RepositoryFunctionException;
 import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper;
 import com.google.devtools.build.lib.runtime.ProcessWrapper;
@@ -516,26 +517,42 @@ public String toString() {
    */
   // TODO(wyv): somehow migrate this to the base context too.
   public void enforceLabelAttributes() throws EvalException, InterruptedException {
+    // TODO: If a labels fails to resolve to an existing regular file, we do not add a dependency on
+    //  that fact - if the file is created later or changes its type, it will not trigger a rerun of
+    //  the repository function.
     StructImpl attr = getAttr();
     for (String name : attr.getFieldNames()) {
       Object value = attr.getValue(name);
       if (value instanceof Label) {
-        getPathFromLabel((Label) value);
+        dependOnLabelIgnoringErrors((Label) value);
       }
       if (value instanceof Sequence) {
         for (Object entry : (Sequence) value) {
           if (entry instanceof Label) {
-            getPathFromLabel((Label) entry);
+            dependOnLabelIgnoringErrors((Label) entry);
           }
         }
       }
       if (value instanceof Dict) {
         for (Object entry : ((Dict) value).keySet()) {
           if (entry instanceof Label) {
-            getPathFromLabel((Label) entry);
+            dependOnLabelIgnoringErrors((Label) entry);
           }
         }
       }
     }
   }
+
+  private void dependOnLabelIgnoringErrors(Label label)
+      throws InterruptedException, NeedsSkyframeRestartException {
+    try {
+      getPathFromLabel(label);
+    } catch (NeedsSkyframeRestartException e) {
+      throw e;
+    } catch (EvalException e) {
+      // EvalExceptions indicate labels not referring to existing files. This is fine,
+      // as long as they are never resolved to files in the execution of the rule; we allow
+      // non-strict rules.
+    }
+  }
 }

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java

@@ -198,11 +198,6 @@ public RepositoryDirectoryValue.Builder fetch(
       } catch (NeedsSkyframeRestartException e) {
         // Missing values are expected; just restart before we actually start the rule
         return null;
-      } catch (EvalException e) {
-        // EvalExceptions indicate labels not referring to existing files. This is fine,
-        // as long as they are never resolved to files in the execution of the rule; we allow
-        // non-strict rules. So now we have to start evaluating the actual rule, even if that
-        // means the rule might get restarted for legitimate reasons.
       }
 
       // This rule is mainly executed for its side effect. Nevertheless, the return value is

src/test/shell/bazel/starlark_prefetching_test.sh

@@ -227,4 +227,48 @@ EOF
   bazel build @ext//:foo || fail "expected success"
 }
 
+# Regression test for https://github.com/bazelbuild/bazel/issues/13441
+function test_files_tracked_with_non_existing_files() {
+  cat > rules.bzl <<'EOF'
+def _repo_impl(ctx):
+    ctx.symlink(ctx.path(Label("@//:WORKSPACE")).dirname, "link")
+    print("b.txt: " + ctx.read("link/b.txt"))
+    print("c.txt: " + ctx.read("link/c.txt"))
+
+    ctx.file("BUILD")
+    ctx.file("WORKSPACE")
+
+repo = repository_rule(
+    _repo_impl,
+    attrs = {"_files": attr.label_list(
+        default = [
+            Label("@//:a.txt"),
+            Label("@//:b.txt"),
+            Label("@//:c.txt"),
+        ],
+    )},
+)
+EOF
+
+  cat > WORKSPACE <<'EOF'
+load(":rules.bzl", "repo")
+repo(name = "ext")
+EOF
+  touch BUILD
+
+  # a.txt is intentionally not created
+  echo "bbbb" > b.txt
+  echo "cccc" > c.txt
+
+  # The missing file dependency is tolerated.
+  bazel build @ext//:all &> "$TEST_log" || fail "Expected repository rule to build"
+  expect_log "b.txt: bbbb"
+  expect_log "c.txt: cccc"
+
+  echo "not_cccc" > c.txt
+  bazel build @ext//:all &> "$TEST_log" || fail "Expected repository rule to build"
+  expect_log "b.txt: bbbb"
+  expect_log "c.txt: not_cccc"
+}
+
 run_suite "Starlark repo prefetching tests"