No longer eagerly fetch labels in repo rule attributes

Eager fetching of all labels listed in repo rule attributes was introduced as a performance optimization to avoid costly restarts.

Now that restarts are gone by default, this is no longer a benefit as it can cause unnecessary fetches and also create cycles where there wouldn't be any without this behavior (e.g. when two repos write each others labels into a file without resolving them).

Work towards bazelbuild#19055

Closes bazelbuild#23371.

PiperOrigin-RevId: 665744319
Change-Id: Ia27f207793a2da3fb8e37743b328483f9d45192c

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

@@ -58,7 +58,6 @@
 import net.starlark.java.annot.StarlarkMethod;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
-import net.starlark.java.eval.Sequence;
 import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkInt;
 import net.starlark.java.eval.StarlarkSemantics;
@@ -516,66 +515,4 @@ public void watchTree(Object path)
   public String toString() {
     return "repository_ctx[" + rule.getLabel() + "]";
   }
-
-  /**
-   * Try to compute the paths of all attributes that are labels, including labels in list and dict
-   * arguments.
-   *
-   * <p>The value is ignored, but any missing information from the environment is detected (and an
-   * exception thrown). In this way, we can enforce that all arguments are evaluated before we start
-   * potentially more expensive operations.
-   */
-  // TODO(wyv): somehow migrate this to the base context too.
-  public void enforceLabelAttributes()
-      throws EvalException, InterruptedException, NeedsSkyframeRestartException {
-    // 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();
-    // Batch restarts as they are expensive
-    boolean needsRestart = false;
-    for (String name : attr.getFieldNames()) {
-      Object value = attr.getValue(name);
-      if (value instanceof Label) {
-        if (dependOnLabelIgnoringErrors((Label) value)) {
-          needsRestart = true;
-        }
-      }
-      if (value instanceof Sequence) {
-        for (Object entry : (Sequence) value) {
-          if (entry instanceof Label) {
-            if (dependOnLabelIgnoringErrors((Label) entry)) {
-              needsRestart = true;
-            }
-          }
-        }
-      }
-      if (value instanceof Dict) {
-        for (Object entry : ((Dict) value).keySet()) {
-          if (entry instanceof Label) {
-            if (dependOnLabelIgnoringErrors((Label) entry)) {
-              needsRestart = true;
-            }
-          }
-        }
-      }
-    }
-
-    if (needsRestart) {
-      throw new NeedsSkyframeRestartException();
-    }
-  }
-
-  private boolean dependOnLabelIgnoringErrors(Label label) throws InterruptedException {
-    try {
-      getPathFromLabel(label);
-    } catch (NeedsSkyframeRestartException e) {
-      return true;
-    } 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.
-    }
-    return false;
-  }
 }

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

@@ -302,15 +302,6 @@ private RepositoryDirectoryValue.Builder fetchInternal(
         PrecomputedValue.REMOTE_EXECUTION_ENABLED.get(env);
       }
 
-      // Since restarting a repository function can be really expensive, we first ensure that
-      // all label-arguments can be resolved to paths.
-      try {
-        starlarkRepositoryContext.enforceLabelAttributes();
-      } catch (NeedsSkyframeRestartException e) {
-        // Missing values are expected; just restart before we actually start the rule
-        return null;
-      }
-
       // This rule is mainly executed for its side effect. Nevertheless, the return value is
       // of importance, as it provides information on how the call has to be modified to be a
       // reproducible rule.

src/test/shell/bazel/BUILD

@@ -844,13 +844,6 @@ sh_test(
     tags = ["no_windows"],
 )
 
-sh_test(
-    name = "starlark_prefetching_test",
-    srcs = ["starlark_prefetching_test.sh"],
-    data = [":test-deps"],
-    tags = ["no_windows"],
-)
-
 sh_test(
     name = "external_starlark_load_test",
     size = "large",

src/test/shell/bazel/external_integration_test.sh

@@ -2665,7 +2665,7 @@ EOF
 
   mkdir main
   cd main
-  cat > foo.bzl <<'EOF'
+  cat > foo.bzl <<EOF
 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
 
 def foo():
@@ -2675,7 +2675,7 @@ def foo():
     build_file = "@b//:a.BUILD",
   )
 EOF
-  cat > bar.bzl <<'EOF'
+  cat > bar.bzl <<EOF
 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
 
 def bar():