Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax Label repo visibility validation #16578

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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,16 @@ 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')")))
assertThrows(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");
}
}