Skip to content

Commit

Permalink
Disallow path label syntax for restricted register_toolchains
Browse files Browse the repository at this point in the history
This change only applies to --experimental_single_package_toolchain_binding mode, which restricts register_toolchains to be clear about binding toolchains in only a single package.

path label syntax should be disabled given the semantic intention of --experimental_single_package_toolchain_binding because: If one were to specify register_toolchains("foo/bar/baz"), it may result in bindings to either foo/bar/baz:baz, foo/bar:baz, or foo:bar/baz (depending on which BUILD files are present).

Also: add a test that this restriction already exists for register_toolchains in module files (regardless of the restriction flag)
PiperOrigin-RevId: 632505958
Change-Id: I38a0b1e12fc4b975ae02f9e5d4e141a3c45d06da
  • Loading branch information
c-parsons authored and Kila2 committed May 13, 2024
1 parent bbbf0b1 commit 21673d9
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,12 @@ public void registerToolchains(Sequence<?> toolchainLabels, StarlarkThread threa
"invalid target pattern \"%s\": register_toolchain target patterns may only refer to "
+ "targets within a single package",
tp.getOriginalPattern());
} else if (tp.getType() == TargetPattern.Type.PATH_AS_TARGET) {
throw Starlark.errorf(
"invalid target pattern \"%s\": register_toolchain target patterns may only refer to "
+ "targets with a declared package (relative path syntax omitting ':' is "
+ "ambiguous)",
tp.getOriginalPattern());
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1535,7 +1535,7 @@ public void isolatedUsage_notEnabled() throws Exception {
}

@Test
public void testRegisterToolchains_singlePackageRestriction_error() throws Exception {
public void testRegisterToolchains_singlePackageRestriction_underDir() throws Exception {
// Test intentionally introduces errors.
reporter.removeHandler(failFastHandler);
PrecomputedValue.STARLARK_SEMANTICS.set(
Expand All @@ -1562,6 +1562,29 @@ public void testRegisterToolchains_singlePackageRestriction_error() throws Excep
+ "may only refer to targets within a single package");
}

@Test
public void testRegisterToolchains_pathSyntax() throws Exception {
// Test intentionally introduces errors.
reporter.removeHandler(failFastHandler);

scratch.overwriteFile(
rootDirectory.getRelative("MODULE.bazel").getPathString(),
"module(name='aaa')",
"register_toolchains('bar/baz')");

FakeRegistry registry = registryFactory.newFakeRegistry("/foo");
ModuleFileFunction.REGISTRIES.set(differencer, ImmutableSet.of(registry.getUrl()));

EvaluationResult<RootModuleFileValue> result =
evaluator.evaluate(
ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext);
assertThat(result.hasError()).isTrue();

assertContainsEvent(
"Expected absolute target patterns (must begin with '//' or '@') for 'register_toolchains'"
+ " argument, but got 'bar/baz' as an argument");
}

@Test
public void testRegisterToolchains_singlePackageRestriction() throws Exception {
PrecomputedValue.STARLARK_SEMANTICS.set(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ public void testRegisterToolchains_singlePackageRestriction() throws Exception {
}

@Test
public void testRegisterToolchains_singlePackageRestriction_error() throws Exception {
public void testRegisterToolchains_singlePackageRestriction_underDir() throws Exception {
// Test intentionally introduces errors.
reporter.removeHandler(failFastHandler);

Expand All @@ -289,6 +289,26 @@ public void testRegisterToolchains_singlePackageRestriction_error() throws Excep
+ "may only refer to targets within a single package");
}

@Test
public void testRegisterToolchains_singlePackageRestriction_pathSyntax() throws Exception {
// Test intentionally introduces errors.
reporter.removeHandler(failFastHandler);

setBuildLanguageOptions("--experimental_single_package_toolchain_binding");

String[] lines = {"register_toolchains('foo/bar')"};
createWorkspaceFile(lines);

SkyKey key = ExternalPackageFunction.key();
EvaluationResult<PackageValue> evaluationResult = eval(key);
Package pkg = evaluationResult.get(key).getPackage();
assertThat(pkg.containsErrors()).isTrue();
assertContainsEvent(
"invalid target pattern \"foo/bar\": register_toolchain target patterns may only refer "
+ "to targets with a declared package (relative path syntax omitting ':' is "
+ "ambiguous)");
}

@Test
public void testNoWorkspaceFile() throws Exception {
// Create and immediately delete to make sure we got the right file.
Expand Down

0 comments on commit 21673d9

Please sign in to comment.