Skip to content

Commit

Permalink
Ensure that extension unique names followed by ~ are prefix-free
Browse files Browse the repository at this point in the history
Fixes errors such as the following when a module provides two extensions that share a name:

```
ERROR: /my/workspace/library_path/BUILD:4:13: no such package '@_main~my_ext~2~xyz//':
The repository '@_main~my_ext~2~xyz' could not be resolved:
Repository '@_main~my_ext~2~xyz' is not defined and referenced by '//library_path:library_name'
ERROR: Analysis of target '//my_path:my_target' failed; build aborted:
```

This was a consequence of the extension unique names followed by `~` being

```
_main~my_ext~2~
_main~my_ext~
```

since 19a9710. Before, they were of the following form, which was prefix-free:

```
_main~my_ext2~
_main~my_ext~
```

Fixes bazelbuild#19155

Closes bazelbuild#19156.

PiperOrigin-RevId: 553567725
Change-Id: I98650663fea3bfee77752a06a99132e507d91aef
  • Loading branch information
fmeum authored and bazel-io committed Aug 3, 2023
1 parent 283ed36 commit 1c8abf1
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static com.google.common.collect.ImmutableMap.toImmutableMap;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.BiMap;
import com.google.common.collect.HashBiMap;
import com.google.common.collect.ImmutableBiMap;
Expand Down Expand Up @@ -247,39 +248,45 @@ static BzlmodFlagsAndEnvVars getFlagsAndEnvVars(Environment env) throws Interrup

private ImmutableBiMap<String, ModuleExtensionId> calculateUniqueNameForUsedExtensionId(
ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById) {
// Calculate a unique name for each used extension id.
// Calculate a unique name for each used extension id with the following property that is
// required for BzlmodRepoRuleFunction to unambiguously identify the extension that generates a
// given repo:
// After appending a single `~` to each such name, none of the resulting strings is a prefix of
// any other such string.
BiMap<String, ModuleExtensionId> extensionUniqueNames = HashBiMap.create();
for (ModuleExtensionId id : extensionUsagesById.rowKeySet()) {
// Ensure that the resulting extension name (and thus the repository names derived from it) do
// not start with a tilde.
RepositoryName repository = id.getBzlFileLabel().getRepository();
String nonEmptyRepoPart = repository.isMain() ? "_main" : repository.getName();
// When using a namespace, prefix the extension name with "_" to distinguish the prefix from
// those generated by non-namespaced extension usages. Extension names are identified by their
// Starlark identifier, which in the case of an exported symbol cannot start with "_".
String bestName =
id.getIsolationKey()
.map(
namespace ->
String.format(
"%s~_%s~%s~%s~%s",
nonEmptyRepoPart,
id.getExtensionName(),
namespace.getModule().getName(),
namespace.getModule().getVersion(),
namespace.getUsageExportedName()))
.orElse(nonEmptyRepoPart + "~" + id.getExtensionName());
if (extensionUniqueNames.putIfAbsent(bestName, id) == null) {
continue;
}
int suffix = 2;
while (extensionUniqueNames.putIfAbsent(bestName + "~" + suffix, id) != null) {
suffix++;
int attempt = 1;
while (extensionUniqueNames.putIfAbsent(makeUniqueNameCandidate(id, attempt), id) != null) {
attempt++;
}
}
return ImmutableBiMap.copyOf(extensionUniqueNames);
}

private static String makeUniqueNameCandidate(ModuleExtensionId id, int attempt) {
// Ensure that the resulting extension name (and thus the repository names derived from it) do
// not start with a tilde.
RepositoryName repository = id.getBzlFileLabel().getRepository();
String nonEmptyRepoPart = repository.isMain() ? "_main" : repository.getName();
// When using a namespace, prefix the extension name with "_" to distinguish the prefix from
// those generated by non-namespaced extension usages. Extension names are identified by their
// Starlark identifier, which in the case of an exported symbol cannot start with "_".
Preconditions.checkArgument(attempt >= 1);
String extensionNameDisambiguator = attempt == 1 ? "" : String.valueOf(attempt);
return id.getIsolationKey()
.map(
namespace ->
String.format(
"%s~_%s%s~%s~%s~%s",
nonEmptyRepoPart,
id.getExtensionName(),
extensionNameDisambiguator,
namespace.getModule().getName(),
namespace.getModule().getVersion(),
namespace.getUsageExportedName()))
.orElse(nonEmptyRepoPart + "~" + id.getExtensionName() + extensionNameDisambiguator);
}

static class BazelDepGraphFunctionException extends SkyFunctionException {
BazelDepGraphFunctionException(ExternalDepsException e, Transience transience) {
super(e, transience);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ public void createValue_moduleExtensions() throws Exception {
maven, "rules_jvm_external~1.0~maven",
pip, "rules_python~2.0~pip",
myext, "dep~2.0~myext",
myext2, "dep~2.0~myext~2");
myext2, "dep~2.0~myext2");

assertThat(value.getFullRepoMapping(ModuleKey.ROOT))
.isEqualTo(
Expand Down Expand Up @@ -323,7 +323,7 @@ public void createValue_moduleExtensions() throws Exception {
"oneext",
"dep~2.0~myext~myext",
"twoext",
"dep~2.0~myext~2~myext"));
"dep~2.0~myext2~myext"));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,55 @@ public void simpleExtension_nonCanonicalLabel_repoName() throws Exception {
assertThat(result.get(skyKey).getModule().getGlobal("data")).isEqualTo("foo:fu bar:ba quz:qu");
}

@Test
public void multipleExtensions_sameName() throws Exception {
scratch.file(
workspaceRoot.getRelative("MODULE.bazel").getPathString(),
"bazel_dep(name='data_repo', version='1.0')",
"first_ext = use_extension('//first_ext:defs.bzl', 'ext')",
"first_ext.tag(name='foo', data='first_fu')",
"first_ext.tag(name='bar', data='first_ba')",
"use_repo(first_ext, first_foo='foo', first_bar='bar')",
"second_ext = use_extension('//second_ext:defs.bzl', 'ext')",
"second_ext.tag(name='foo', data='second_fu')",
"second_ext.tag(name='bar', data='second_ba')",
"use_repo(second_ext, second_foo='foo', second_bar='bar')");
scratch.file(workspaceRoot.getRelative("first_ext/BUILD").getPathString());
scratch.file(
workspaceRoot.getRelative("first_ext/defs.bzl").getPathString(),
"load('@data_repo//:defs.bzl','data_repo')",
"tag = tag_class(attrs = {'name':attr.string(),'data':attr.string()})",
"def _ext_impl(ctx):",
" for mod in ctx.modules:",
" for tag in mod.tags.tag:",
" data_repo(name=tag.name,data=tag.data)",
"ext = module_extension(implementation=_ext_impl, tag_classes={'tag':tag})");
scratch.file(workspaceRoot.getRelative("second_ext/BUILD").getPathString());
scratch.file(
workspaceRoot.getRelative("second_ext/defs.bzl").getPathString(),
"load('//first_ext:defs.bzl', _ext = 'ext')",
"ext = _ext");
scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
scratch.file(
workspaceRoot.getRelative("data.bzl").getPathString(),
"load('@first_foo//:data.bzl', first_foo_data='data')",
"load('@first_bar//:data.bzl', first_bar_data='data')",
"load('@second_foo//:data.bzl', second_foo_data='data')",
"load('@second_bar//:data.bzl', second_bar_data='data')",
"data = 'first_foo:'+first_foo_data+' first_bar:'+first_bar_data"
+ "+' second_foo:'+second_foo_data+' second_bar:'+second_bar_data");

SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
EvaluationResult<BzlLoadValue> result =
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
if (result.hasError()) {
throw result.getError().getException();
}
assertThat(result.get(skyKey).getModule().getGlobal("data"))
.isEqualTo(
"first_foo:first_fu first_bar:first_ba second_foo:second_fu " + "second_bar:second_ba");
}

@Test
public void multipleModules() throws Exception {
scratch.file(
Expand Down

0 comments on commit 1c8abf1

Please sign in to comment.