Skip to content

Commit

Permalink
Fail Label call referencing invalid repository with strict visibility
Browse files Browse the repository at this point in the history
With Bzlmod's enforcement of strict visibility of repositories based on repository mapping, a `Label` created from a repo-absolute (but not canonical) label string such as `@repo//foo:bar` will be in an error state if there is no repository visible from the calling .bzl module under the apparent name `repo`.

Before this commit, such instances of `Label` could be freely constructed and used from Starlark, even though some of their fields return nonsensical values (for example, `workspace_root` will be `"external"` for `Label("@//foo:bar")` called from a repo without visibility into the main repository).

This commit ensures that instead the `Label` call (and also analogous calls to `relative`) fail with an error such as:
```
Invalid label string '@//foo:bar': no repository visible as '@' from repository '@current_repo'
```

Fixes #16528

Closes #16530.

PiperOrigin-RevId: 483373288
Change-Id: I4831e179a2d0363aa2f27ca4a25d79a634103f8f
  • Loading branch information
fmeum authored and copybara-github committed Oct 24, 2022
1 parent 1e2e195 commit 3f92232
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1004,7 +1004,15 @@ public Label label(String labelString, StarlarkThread thread) throws EvalExcepti
BazelModuleContext moduleContext =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread));
try {
return Label.parseWithRepoContext(labelString, moduleContext.packageContext());
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;
} catch (LabelSyntaxException e) {
throw Starlark.errorf("Illegal absolute label syntax: %s", e.getMessage());
}
Expand Down
21 changes: 17 additions & 4 deletions src/main/java/com/google/devtools/build/lib/cmdline/Label.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,10 @@
import net.starlark.java.annot.Param;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.annot.StarlarkMethod;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Module;
import net.starlark.java.eval.Printer;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.StarlarkThread;
import net.starlark.java.eval.StarlarkValue;
Expand Down Expand Up @@ -502,10 +504,21 @@ 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 {
return getRelativeWithRemapping(
relName,
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).repoMapping());
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;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ public class ModuleExtensionResolutionTest extends FoundationTestCase {
@Before
public void setup() throws Exception {
workspaceRoot = scratch.dir("/ws");
String bazelToolsPath = "/ws/embedded_tools";
scratch.file(bazelToolsPath + "/MODULE.bazel", "module(name = 'bazel_tools')");
scratch.file(bazelToolsPath + "/WORKSPACE");
modulesRoot = scratch.dir("/modules");
differencer = new SequencedRecordingDifferencer();
evaluationContext =
Expand Down Expand Up @@ -178,7 +181,11 @@ public void setup() throws Exception {
externalFilesHelper))
.put(
SkyFunctions.MODULE_FILE,
new ModuleFileFunction(registryFactory, workspaceRoot, ImmutableMap.of()))
new ModuleFileFunction(
registryFactory,
workspaceRoot,
// Required to load @_builtins.
ImmutableMap.of("bazel_tools", LocalPathOverride.create(bazelToolsPath))))
.put(SkyFunctions.PRECOMPUTED, new PrecomputedFunction())
.put(SkyFunctions.BZL_COMPILE, new BzlCompileFunction(packageFactory, hashFunction))
.put(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,16 @@
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.starlark.StarlarkAttrModule;
import com.google.devtools.build.lib.analysis.starlark.StarlarkModules;
import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleClassFunctions.StarlarkRuleFunction;
import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.analysis.util.TestAspects;
import com.google.devtools.build.lib.cmdline.BazelModuleContext;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.collect.nestedset.Depset;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventKind;
Expand Down Expand Up @@ -64,6 +69,7 @@
import com.google.devtools.build.lib.testutil.MoreAsserts;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import com.google.devtools.build.lib.util.FileTypeSet;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Arrays;
import java.util.List;
import javax.annotation.Nullable;
Expand All @@ -75,8 +81,10 @@
import net.starlark.java.eval.StarlarkCallable;
import net.starlark.java.eval.StarlarkInt;
import net.starlark.java.eval.StarlarkList;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.Structure;
import net.starlark.java.eval.Tuple;
import net.starlark.java.syntax.FileOptions;
import net.starlark.java.syntax.ParserInput;
import net.starlark.java.syntax.Program;
import net.starlark.java.syntax.StarlarkFile;
Expand Down Expand Up @@ -2828,4 +2836,56 @@ public void testAnalysisTestErrorOverridingName() throws Exception {
ev.assertContainsError(
"Error in analysis_test: 'name' cannot be set or overridden in 'attr_values'");
}

private Object eval(Module module, String... lines) throws Exception {
ParserInput input = ParserInput.fromLines(lines);
return Starlark.eval(input, FileOptions.DEFAULT, module, ev.getStarlarkThread());
}

@Test
public void testLabelWithStrictVisibility() throws Exception {
ImmutableMap.Builder<String, Object> predeclared = ImmutableMap.builder();
StarlarkModules.addPredeclared(predeclared);
RepositoryName currentRepo = RepositoryName.createUnvalidated("module~1.2.3");
RepositoryName otherRepo = RepositoryName.createUnvalidated("dep~4.5");
Label bzlLabel =
Label.create(
PackageIdentifier.create(currentRepo, PathFragment.create("lib")), "label.bzl");
Object clientData =
BazelModuleContext.create(
bzlLabel,
RepositoryMapping.create(
ImmutableMap.of("my_module", currentRepo, "dep", otherRepo), currentRepo),
"lib/label.bzl",
/*loads=*/ ImmutableMap.of(),
/*bzlTransitiveDigest=*/ new byte[0]);
Module module =
Module.withPredeclaredAndData(
StarlarkSemantics.DEFAULT, predeclared.buildOrThrow(), clientData);

assertThat(eval(module, "Label('//foo:bar').workspace_root"))
.isEqualTo("external/module~1.2.3");
assertThat(eval(module, "Label('@my_module//foo:bar').workspace_root"))
.isEqualTo("external/module~1.2.3");
assertThat(eval(module, "Label('@@module~1.2.3//foo:bar').workspace_root"))
.isEqualTo("external/module~1.2.3");
assertThat(eval(module, "Label('@dep//foo:bar').workspace_root")).isEqualTo("external/dep~4.5");
assertThat(eval(module, "Label('@@dep~4.5//foo:bar').workspace_root"))
.isEqualTo("external/dep~4.5");
assertThat(eval(module, "Label('@@//foo:bar').workspace_root")).isEqualTo("");

assertThat(assertThrows(EvalException.class, () -> eval(module, "Label('@//foo:bar')")))
.hasMessageThat()
.contains(
"Invalid label string '@//foo:bar': no repository visible as '@' from repository "
+ "'@module~1.2.3'");
assertThat(
assertThrows(
EvalException.class,
() -> eval(module, "Label('@@//foo:bar').relative('@not_dep//foo:bar')")))
.hasMessageThat()
.contains(
"Invalid label string '@not_dep//foo:bar': no repository visible as '@not_dep' "
+ "from repository '@module~1.2.3'");
}
}

0 comments on commit 3f92232

Please sign in to comment.