From 8caa68194ed3357709f44a79d03f3fea57c5f1f2 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 3 Aug 2023 14:48:33 +0200 Subject: [PATCH] Ensure that extension unique names followed by `~` are prefix-free 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 19a971089952e53bba130a8e3e8b28b6b1e310e6. Before, they were of the following form, which was prefix-free: ``` _main~my_ext2~ _main~my_ext~ ``` --- .../bazel/bzlmod/BazelDepGraphFunction.java | 59 +++++++++++-------- .../bzlmod/BazelDepGraphFunctionTest.java | 4 +- .../bzlmod/ModuleExtensionResolutionTest.java | 49 +++++++++++++++ 3 files changed, 84 insertions(+), 28 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java index eaf358a011d05b..9e7c0a328e8356 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java @@ -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; @@ -247,39 +248,45 @@ static BzlmodFlagsAndEnvVars getFlagsAndEnvVars(Environment env) throws Interrup private ImmutableBiMap calculateUniqueNameForUsedExtensionId( ImmutableTable 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 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); diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java index 2e72f147ed8307..cbca6308c2eddd 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java @@ -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( @@ -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 diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index ebe22607deb7f7..42cc96c9e9e895 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -427,6 +427,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 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(