From cf3f48ca49f089615417636763d753811acf717f Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 27 Oct 2022 06:02:55 -0700 Subject: [PATCH] Relax `Label` repo visibility validation Failing the build eagerly when `Label` is passed a label string referencing an unknown apparent repository name turned out to be too strict and causes failures on invalid, but unused labels: https://buildkite.com/bazel/bazel-bazel-with-bzlmod/builds/667#01841861-3d3a-4b65-91b7-b7083ee6b42b Instead, fail when a method that requires a valid repository name is called on an invalid `Label` instance. Closes #16578. PiperOrigin-RevId: 484232254 Change-Id: Ic64ea5db691f39a20dda47e60e2d6f5436ebca1e --- .../starlark/StarlarkRuleClassFunctions.java | 10 +----- .../devtools/build/lib/cmdline/Label.java | 31 +++++++++---------- .../devtools/build/skydoc/SkydocMain.java | 7 +++-- .../StarlarkRuleClassFunctionsTest.java | 20 ++++++------ 4 files changed, 30 insertions(+), 38 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index b43e9d71478124..0c11cec4570586 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java @@ -1004,15 +1004,7 @@ public Label label(String labelString, StarlarkThread thread) throws EvalExcepti BazelModuleContext moduleContext = BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)); try { - Label label = Label.parseWithRepoContext(labelString, moduleContext.packageContext()); - if (!label.getRepository().isVisible()) { - throw Starlark.errorf( - "Invalid label string '%s': no repository visible as '@%s' from %s", - labelString, - label.getRepository().getName(), - label.getRepository().getOwnerRepoDisplayString()); - } - return label; + return Label.parseWithRepoContext(labelString, moduleContext.packageContext()); } catch (LabelSyntaxException e) { throw Starlark.errorf("Illegal absolute label syntax: %s", e.getMessage()); } diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java index 7105afce3f9d42..438d1a50f1d8ec 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java @@ -338,7 +338,8 @@ public String getPackageName() { + " \"external/repo\"", useStarlarkSemantics = true) @Deprecated - public String getWorkspaceRootForStarlarkOnly(StarlarkSemantics semantics) { + public String getWorkspaceRootForStarlarkOnly(StarlarkSemantics semantics) throws EvalException { + checkRepoVisibilityForStarlark("workspace_root"); return packageIdentifier .getRepository() .getExecPath(semantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT)) @@ -431,7 +432,8 @@ public String getUnambiguousCanonicalForm() { "The repository part of this label. For instance, " + "
Label(\"@foo//bar:baz\").workspace_name"
               + " == \"foo\"
") - public String getWorkspaceName() { + public String getWorkspaceName() throws EvalException { + checkRepoVisibilityForStarlark("workspace_name"); return packageIdentifier.getRepository().getName(); } @@ -504,21 +506,10 @@ public Label getLocalTargetLabel(String targetName) throws LabelSyntaxException @Param(name = "relName", doc = "The label that will be resolved relative to this one.") }, useStarlarkThread = true) - public Label getRelative(String relName, StarlarkThread thread) - throws LabelSyntaxException, EvalException { - Label label = - getRelativeWithRemapping( - relName, - BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)) - .repoMapping()); - if (!label.getRepository().isVisible()) { - throw Starlark.errorf( - "Invalid label string '%s': no repository visible as '@%s' from %s", - relName, - label.getRepository().getName(), - label.getRepository().getOwnerRepoDisplayString()); - } - return label; + public Label getRelative(String relName, StarlarkThread thread) throws LabelSyntaxException { + return getRelativeWithRemapping( + relName, + BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).repoMapping()); } /** @@ -676,4 +667,10 @@ public String expandToCommandLine() { // TODO(wyv): Consider using StarlarkSemantics here too for optional unambiguity. return getCanonicalForm(); } + + private void checkRepoVisibilityForStarlark(String method) throws EvalException { + if (!getRepository().isVisible()) { + throw Starlark.errorf("'%s' is not allowed on invalid Label %s", method, this); + } + } } diff --git a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java index 573eb849c541e5..af7b186d477816 100644 --- a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java +++ b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java @@ -163,7 +163,7 @@ public static void main(String[] args) providerInfoMap, userDefinedFunctions, aspectInfoMap); - } catch (StarlarkEvaluationException exception) { + } catch (StarlarkEvaluationException | EvalException exception) { exception.printStackTrace(); System.err.println("Stardoc documentation generation failed: " + exception.getMessage()); System.exit(1); @@ -386,7 +386,8 @@ private Module recursiveEval( List ruleInfoList, List providerInfoList, List aspectInfoList) - throws InterruptedException, IOException, LabelSyntaxException, StarlarkEvaluationException { + throws InterruptedException, IOException, LabelSyntaxException, StarlarkEvaluationException, + EvalException { Path path = pathOfLabel(label, semantics); if (pending.contains(path)) { @@ -473,7 +474,7 @@ path, load, pathOfLabel(relativeLabel, semantics), depRoots), return module; } - private Path pathOfLabel(Label label, StarlarkSemantics semantics) { + private Path pathOfLabel(Label label, StarlarkSemantics semantics) throws EvalException { String workspacePrefix = ""; if (!label.getWorkspaceRootForStarlarkOnly(semantics).isEmpty() && !label.getWorkspaceName().equals(workspaceName)) { diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java index ce0d6ffe69802d..fee817ce7b3a1c 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java @@ -2874,18 +2874,20 @@ public void testLabelWithStrictVisibility() throws Exception { .isEqualTo("external/dep~4.5"); assertThat(eval(module, "Label('@@//foo:bar').workspace_root")).isEqualTo(""); - assertThat(assertThrows(EvalException.class, () -> eval(module, "Label('@//foo:bar')"))) + assertThat(eval(module, "str(Label('@//foo:bar'))")).isEqualTo("@//foo:bar"); + assertThat( + assertThrows( + EvalException.class, () -> eval(module, "Label('@//foo:bar').workspace_name"))) .hasMessageThat() - .contains( - "Invalid label string '@//foo:bar': no repository visible as '@' from repository " - + "'@module~1.2.3'"); + .isEqualTo( + "'workspace_name' is not allowed on invalid Label @[unknown repo '' requested from" + + " @module~1.2.3]//foo:bar"); assertThat( assertThrows( - EvalException.class, - () -> eval(module, "Label('@@//foo:bar').relative('@not_dep//foo:bar')"))) + EvalException.class, () -> eval(module, "Label('@//foo:bar').workspace_root"))) .hasMessageThat() - .contains( - "Invalid label string '@not_dep//foo:bar': no repository visible as '@not_dep' " - + "from repository '@module~1.2.3'"); + .isEqualTo( + "'workspace_root' is not allowed on invalid Label @[unknown repo '' requested from" + + " @module~1.2.3]//foo:bar"); } }