Skip to content

Commit

Permalink
Relax Label repo visibility validation
Browse files Browse the repository at this point in the history
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 bazelbuild#16578.

PiperOrigin-RevId: 484232254
Change-Id: Ic64ea5db691f39a20dda47e60e2d6f5436ebca1e
  • Loading branch information
fmeum authored and copybara-github committed Oct 27, 2022
1 parent 07c5c1a commit cf3f48c
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
31 changes: 14 additions & 17 deletions src/main/java/com/google/devtools/build/lib/cmdline/Label.java
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,8 @@ public String getPackageName() {
+ " \"external/repo\"</pre>",
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))
Expand Down Expand Up @@ -431,7 +432,8 @@ public String getUnambiguousCanonicalForm() {
"The repository part of this label. For instance, "
+ "<pre class=language-python>Label(\"@foo//bar:baz\").workspace_name"
+ " == \"foo\"</pre>")
public String getWorkspaceName() {
public String getWorkspaceName() throws EvalException {
checkRepoVisibilityForStarlark("workspace_name");
return packageIdentifier.getRepository().getName();
}

Expand Down Expand Up @@ -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());
}

/**
Expand Down Expand Up @@ -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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -386,7 +386,8 @@ private Module recursiveEval(
List<RuleInfoWrapper> ruleInfoList,
List<ProviderInfoWrapper> providerInfoList,
List<AspectInfoWrapper> aspectInfoList)
throws InterruptedException, IOException, LabelSyntaxException, StarlarkEvaluationException {
throws InterruptedException, IOException, LabelSyntaxException, StarlarkEvaluationException,
EvalException {
Path path = pathOfLabel(label, semantics);

if (pending.contains(path)) {
Expand Down Expand Up @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}

0 comments on commit cf3f48c

Please sign in to comment.