Skip to content

Commit

Permalink
Cover missing cases during module extension label normalization
Browse files Browse the repository at this point in the history
The previous logic missed to normalize cases such as `"extension.bzl"`
and `"//extension.bzl"`, thus resulting in crashes if these styles are
mixed as well as invalid buildozer commands for `use_repo` fixing.

Instead of enumerating cases, parse the label and emit it in unambiguous
canonical form with a leading `@` stripped.
  • Loading branch information
fmeum committed Dec 20, 2023
1 parent 591d125 commit fa98b07
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,13 @@
import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileGlobals.ModuleExtensionUsageBuilder.ModuleExtensionProxy;
import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
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.events.EventHandler;
import com.google.devtools.build.lib.packages.StarlarkExportable;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashMap;
Expand Down Expand Up @@ -480,17 +484,31 @@ public ModuleExtensionProxy useExtension(
}

private String normalizeLabelString(String rawExtensionBzlFile) {
// Normalize the label by adding the current module's repo_name if the label doesn't specify a
// repository name. This is necessary as ModuleExtensionUsages are grouped by the string value
// of this label, but later mapped to their Label representation. If multiple strings map to the
// same Label, this would result in a crash.
// Normalize the label by parsing and stringifying it with a repo mapping that preserves the
// apparent repository name, except that a reference to the main repository via the empty
// repo name is translated to using the module repo name. This is necessary as
// ModuleExtensionUsages are grouped by the string value of this label, but later mapped to
// their Label representation. If multiple strings map to the same Label, this would result in a
// crash.
// ownName can't change anymore as calling module() after this results in an error.
String ownName = module.getRepoName().orElse(module.getName());
if (module.getKey().equals(ModuleKey.ROOT) && rawExtensionBzlFile.startsWith("@//")) {
return "@" + ownName + rawExtensionBzlFile.substring(1);
} else if (rawExtensionBzlFile.startsWith("//")) {
return "@" + ownName + rawExtensionBzlFile;
} else {
RepositoryName ownRepoName = RepositoryName.createUnvalidated(ownName);
try {
ImmutableMap<String, RepositoryName> repoMapping = ImmutableMap.of();
if (module.getKey().equals(ModuleKey.ROOT)) {
repoMapping = ImmutableMap.of("", ownRepoName);
}
Label label =
Label.parseWithPackageContext(
rawExtensionBzlFile,
Label.PackageContext.of(
PackageIdentifier.create(ownRepoName, PathFragment.EMPTY_FRAGMENT),
RepositoryMapping.createAllowingFallback(repoMapping)));
// Skip over the leading "@" of the unambiguous form.
return label.getUnambiguousCanonicalForm().substring(1);
} catch (LabelSyntaxException ignored) {
// Preserve backwards compatibility by not failing eagerly, rather keep the invalid label and
// let the extension fail when evaluated.
return rawExtensionBzlFile;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,10 @@ public void simpleExtension_nonCanonicalLabel() throws Exception {
"use_repo(ext2, 'bar')",
"ext3 = use_extension('@//:defs.bzl', 'ext')",
"ext3.tag(name='quz', data='qu')",
"use_repo(ext3, 'quz')");
"use_repo(ext3, 'quz')",
"ext4 = use_extension('defs.bzl', 'ext')",
"ext4.tag(name='qor', data='qo')",
"use_repo(ext4, 'qor')");
scratch.file(
workspaceRoot.getRelative("defs.bzl").getPathString(),
"load('@data_repo//:defs.bzl','data_repo')",
Expand All @@ -379,15 +382,17 @@ public void simpleExtension_nonCanonicalLabel() throws Exception {
"load('@foo//:data.bzl', foo_data='data')",
"load('@bar//:data.bzl', bar_data='data')",
"load('@quz//:data.bzl', quz_data='data')",
"data = 'foo:'+foo_data+' bar:'+bar_data+' quz:'+quz_data");
"load('@qor//:data.bzl', qor_data='data')",
"data = 'foo:'+foo_data+' bar:'+bar_data+' quz:'+quz_data+' qor:'+qor_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("foo:fu bar:ba quz:qu");
assertThat(result.get(skyKey).getModule().getGlobal("data"))
.isEqualTo("foo:fu bar:ba quz:qu qor:qo");
}

@Test
Expand Down

0 comments on commit fa98b07

Please sign in to comment.