From 2ed72d79160115c192fd11e41461de296ed0ca77 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 11 May 2023 03:09:24 -0700 Subject: [PATCH] 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 --- .../starlark/StarlarkRepositoryContext.java | 23 ++++++++-- .../starlark/StarlarkRepositoryFunction.java | 5 --- .../shell/bazel/starlark_prefetching_test.sh | 44 +++++++++++++++++++ 3 files changed, 64 insertions(+), 8 deletions(-) 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 2a8b59d80e5d25..dd8ad14c9b9fc8 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 @@ -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. + } + } } 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 83772d1fd28194..140697445739bb 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 @@ -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 diff --git a/src/test/shell/bazel/starlark_prefetching_test.sh b/src/test/shell/bazel/starlark_prefetching_test.sh index f12c7426c617bd..69e5bfda740c30 100755 --- a/src/test/shell/bazel/starlark_prefetching_test.sh +++ b/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"