From f0ab82da5eac8fbedabf253d07692db7a7d0c648 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 8 Feb 2024 21:24:02 -0800 Subject: [PATCH] Allow `@repo_name` labels in override attributes This is made possible by a refactoring that moves the label parsing of `RepoSpec` attributes coming from module overrides as well as `.bzl` file labels containing repo rules from `BzlmodRepoRuleFunction` into `ModuleFileGlobals`. Also adds a TODO to `BzlmodRepoRuleFunction` to further simplify the logic after the next lockfile version bumps, as old lockfiles are now the only source of non-canonical labels in `RepoSpec`s. Work towards #19301 Closes #21188. PiperOrigin-RevId: 605517195 Change-Id: Id34c81fa9474fef2354ff1fbc898e518a9044640 --- MODULE.bazel.lock | 2 +- .../lib/bazel/bzlmod/ArchiveOverride.java | 6 +- .../bazel/bzlmod/ArchiveRepoSpecBuilder.java | 4 +- .../build/lib/bazel/bzlmod/GitOverride.java | 6 +- .../lib/bazel/bzlmod/GitRepoSpecBuilder.java | 6 +- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 35 +++++++++- .../build/lib/bazel/bzlmod/RepoSpec.java | 5 +- .../bazel/bzlmod/SingleVersionOverride.java | 6 +- .../lib/skyframe/BzlmodRepoRuleFunction.java | 11 ++- .../py/bazel/bzlmod/bazel_overrides_test.py | 68 ++++++++++++++++++- src/test/tools/bzlmod/MODULE.bazel.lock | 34 +++++----- 11 files changed, 137 insertions(+), 46 deletions(-) diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index d3e55473dff748..2856f563d2494c 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -2741,7 +2741,7 @@ "general": { "bzlTransitiveDigest": "wiyY30lgHvAgghminN0h4lxMWexDIg/6r/+eOIA5bPU=", "accumulatedFileDigests": { - "@@//src/test/tools/bzlmod:MODULE.bazel.lock": "c5a21b1716921abcd71d97668caa3dddc331daa5c7a1a80e4e041f2fd0e74767", + "@@//src/test/tools/bzlmod:MODULE.bazel.lock": "fe01365e67163a67a1620e4aeb3aec24e8d6b4cc20c49ca9582a5d090b8185b4", "@@//:MODULE.bazel": "f291782aef1d2989f49c0e884b32ec8d7814ae48c598b6f3060870ccb3f5c0b6" }, "envVariables": {}, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveOverride.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveOverride.java index fec5308b4c34f8..cb0cbebc48a581 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveOverride.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveOverride.java @@ -25,7 +25,7 @@ public abstract class ArchiveOverride implements NonRegistryOverride { public static ArchiveOverride create( ImmutableList urls, - ImmutableList patches, + ImmutableList patches, ImmutableList patchCmds, String integrity, String stripPrefix, @@ -37,8 +37,8 @@ public static ArchiveOverride create( /** The URLs pointing at the archives. Can be HTTP(S) or file URLs. */ public abstract ImmutableList getUrls(); - /** The patches to apply after extracting the archive. Should be a list of labels. */ - public abstract ImmutableList getPatches(); + /** The labels of patches to apply after extracting the archive. */ + public abstract ImmutableList getPatches(); /** The patch commands to execute after extracting the archive. Should be a list of commands. */ public abstract ImmutableList getPatchCmds(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java index ad1d115561db45..d6ffe2e46c577b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java @@ -27,7 +27,7 @@ */ public class ArchiveRepoSpecBuilder { - public static final String HTTP_ARCHIVE_PATH = "@bazel_tools//tools/build_defs/repo:http.bzl"; + public static final String HTTP_ARCHIVE_PATH = "@@bazel_tools//tools/build_defs/repo:http.bzl"; private final ImmutableMap.Builder attrBuilder; @@ -54,7 +54,7 @@ public ArchiveRepoSpecBuilder setStripPrefix(String stripPrefix) { } @CanIgnoreReturnValue - public ArchiveRepoSpecBuilder setPatches(ImmutableList patches) { + public ArchiveRepoSpecBuilder setPatches(ImmutableList patches) { attrBuilder.put("patches", patches); return this; } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitOverride.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitOverride.java index 11374517d5328d..1c209f18ed8543 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitOverride.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitOverride.java @@ -25,7 +25,7 @@ public abstract class GitOverride implements NonRegistryOverride { public static GitOverride create( String remote, String commit, - ImmutableList patches, + ImmutableList patches, ImmutableList patchCmds, int patchStrip, boolean initSubmodules) { @@ -39,8 +39,8 @@ public static GitOverride create( /** The commit hash to use. */ public abstract String getCommit(); - /** The patches to apply after fetching from Git. Should be a list of labels. */ - public abstract ImmutableList getPatches(); + /** The labels of patches to apply after fetching from Git. */ + public abstract ImmutableList getPatches(); /** The patch commands to execute after fetching from Git. Should be a list of commands. */ public abstract ImmutableList getPatchCmds(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitRepoSpecBuilder.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitRepoSpecBuilder.java index 2adf945e939904..ce8420176e1e95 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitRepoSpecBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitRepoSpecBuilder.java @@ -25,7 +25,7 @@ */ public class GitRepoSpecBuilder { - public static final String GIT_REPO_PATH = "@bazel_tools//tools/build_defs/repo:git.bzl"; + public static final String GIT_REPO_PATH = "@@bazel_tools//tools/build_defs/repo:git.bzl"; private final ImmutableMap.Builder attrBuilder; @@ -71,7 +71,7 @@ public GitRepoSpecBuilder setStripPrefix(String stripPrefix) { } @CanIgnoreReturnValue - public GitRepoSpecBuilder setPatches(List patches) { + public GitRepoSpecBuilder setPatches(List patches) { return setAttr("patches", patches); } @@ -113,7 +113,7 @@ private GitRepoSpecBuilder setAttr(String name, boolean value) { } @CanIgnoreReturnValue - private GitRepoSpecBuilder setAttr(String name, List value) { + private GitRepoSpecBuilder setAttr(String name, List value) { if (value != null && !value.isEmpty()) { attrBuilder.put(name, value); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index 70502d4bbb00ec..e508301cbecb7a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -15,6 +15,8 @@ package com.google.devtools.build.lib.bazel.bzlmod; +import static com.google.common.collect.ImmutableList.toImmutableList; + import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.HashBiMap; @@ -513,6 +515,27 @@ private String normalizeLabelString(String rawExtensionBzlFile) { } } + /** + * Returns a {@link Label} when the given string is a valid label, otherwise the string itself. + */ + private Object parseOverrideLabelAttribute(String rawLabel) { + RepositoryMapping repoMapping = + RepositoryMapping.create( + ImmutableMap.builder() + .put("", RepositoryName.MAIN) + .put(module.getRepoName().orElse(module.getName()), RepositoryName.MAIN) + .buildKeepingLast(), + RepositoryName.MAIN); + try { + return Label.parseWithPackageContext( + rawLabel, Label.PackageContext.of(PackageIdentifier.EMPTY_PACKAGE_ID, repoMapping)); + } catch (LabelSyntaxException e) { + // Preserve backwards compatibility by not failing eagerly, rather keep the invalid label and + // let the module repo fail when fetched. + return rawLabel; + } + } + class ModuleExtensionUsageBuilder { private final String extensionBzlFile; private final String extensionName; @@ -848,7 +871,9 @@ public void singleVersionOverride( SingleVersionOverride.create( parsedVersion, registry, - Sequence.cast(patches, String.class, "patches").getImmutableList(), + Sequence.cast(patches, String.class, "patches").stream() + .map(this::parseOverrideLabelAttribute) + .collect(toImmutableList()), Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(), patchStrip.toInt("single_version_override.patch_strip"))); } @@ -984,7 +1009,9 @@ public void archiveOverride( moduleName, ArchiveOverride.create( urlList, - Sequence.cast(patches, String.class, "patches").getImmutableList(), + Sequence.cast(patches, String.class, "patches").stream() + .map(this::parseOverrideLabelAttribute) + .collect(toImmutableList()), Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(), integrity, stripPrefix, @@ -1060,7 +1087,9 @@ public void gitOverride( GitOverride.create( remote, commit, - Sequence.cast(patches, String.class, "patches").getImmutableList(), + Sequence.cast(patches, String.class, "patches").stream() + .map(this::parseOverrideLabelAttribute) + .collect(toImmutableList()), Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(), patchStrip.toInt("git_override.patch_strip"), initSubmodules)); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RepoSpec.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RepoSpec.java index 3a9e28f7caddfd..201312bc0e4a4e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RepoSpec.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RepoSpec.java @@ -29,7 +29,8 @@ public abstract class RepoSpec implements SkyValue { /** - * The label string for the bzl file this repository rule is defined in, empty for native rule. + * The unambiguous canonical label string for the bzl file this repository rule is defined in, + * empty for native rule. */ @Nullable public abstract String bzlFile(); @@ -52,6 +53,8 @@ public static Builder builder() { public abstract static class Builder { public abstract Builder setBzlFile(String bzlFile); + abstract String bzlFile(); + public abstract Builder setRuleClassName(String name); public abstract Builder setAttributes(AttributeValues attributes); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleVersionOverride.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleVersionOverride.java index 723d87587b1680..1e2ceb980e3fd6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleVersionOverride.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleVersionOverride.java @@ -33,7 +33,7 @@ public abstract class SingleVersionOverride implements RegistryOverride { public static SingleVersionOverride create( Version version, String registry, - ImmutableList patches, + ImmutableList patches, ImmutableList patchCmds, int patchStrip) { return new AutoValue_SingleVersionOverride(version, registry, patches, patchCmds, patchStrip); @@ -48,8 +48,8 @@ public static SingleVersionOverride create( @Override public abstract String getRegistry(); - /** The patches to apply after retrieving per the registry. Should be a list of labels. */ - public abstract ImmutableList getPatches(); + /** The labels of patches to apply after retrieving per the registry. */ + public abstract ImmutableList getPatches(); /** * The patch commands to execute after retrieving per the registry. Should be a list of commands. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java index d058da5cda5569..9d965727f655b1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java @@ -70,14 +70,11 @@ public final class BzlmodRepoRuleFunction implements SkyFunction { private final BlazeDirectories directories; /** - * An empty repo mapping anchored to the main repo. - * - *

None of the labels present in RepoSpecs can point to any repo other than the main repo - * or @bazel_tools, because at this point we don't know how any other repo is defined yet. The - * RepoSpecs processed by this class can only contain labels from the MODULE.bazel file (from - * overrides). In the future, they might contain labels from the lockfile, but those will need to - * be canonical label literals, which bypass repo mapping anyway. + * An empty repo mapping anchored to the main repo. Label strings in {@link RepoSpec}s are always + * in unambiguous canonical form and thus require no mapping, except instances read from old + * lockfiles. */ + // TODO(fmeum): Make this mapping truly empty after bumping LOCK_FILE_VERSION. private static final RepositoryMapping EMPTY_MAIN_REPO_MAPPING = RepositoryMapping.create( ImmutableMap.of("", RepositoryName.MAIN, "bazel_tools", RepositoryName.BAZEL_TOOLS), diff --git a/src/test/py/bazel/bzlmod/bazel_overrides_test.py b/src/test/py/bazel/bzlmod/bazel_overrides_test.py index b942700fe314a3..158b789a34377d 100644 --- a/src/test/py/bazel/bzlmod/bazel_overrides_test.py +++ b/src/test/py/bazel/bzlmod/bazel_overrides_test.py @@ -142,23 +142,85 @@ def testRegistryOverride(self): def testArchiveOverride(self): self.writeMainProjectFiles() archive_aaa_1_0 = self.main_registry.archives.joinpath('aaa.1.0.zip') + self.ScratchFile( + 'aaa2.patch', + [ + '--- a/aaa.cc', + '+++ b/aaa.cc', + '@@ -1,6 +1,6 @@', + ' #include ', + ' #include "aaa.h"', + ' void hello_aaa(const std::string& caller) {', + '- std::string lib_name = "aaa@1.0 (locally patched)";', + '+ std::string lib_name = "aaa@1.0 (locally patched again)";', + ' printf("%s => %s\\n", caller.c_str(), lib_name.c_str());', + ' }', + ], + ) + self.ScratchFile( + 'aaa3.patch', + [ + '--- a/aaa.cc', + '+++ b/aaa.cc', + '@@ -1,6 +1,6 @@', + ' #include ', + ' #include "aaa.h"', + ' void hello_aaa(const std::string& caller) {', + '- std::string lib_name = "aaa@1.0 (locally patched again)";', + ( + '+ std::string lib_name = "aaa@1.0 (locally patched again' + ' and again)";' + ), + ' printf("%s => %s\\n", caller.c_str(), lib_name.c_str());', + ' }', + ], + ) + self.ScratchFile( + 'aaa4.patch', + [ + '--- a/aaa.cc', + '+++ b/aaa.cc', + '@@ -1,6 +1,6 @@', + ' #include ', + ' #include "aaa.h"', + ' void hello_aaa(const std::string& caller) {', + ( + '- std::string lib_name = "aaa@1.0 (locally patched again' + ' and again)";' + ), + ( + '+ std::string lib_name = "aaa@1.0 (locally patched all over' + ' again)";' + ), + ' printf("%s => %s\\n", caller.c_str(), lib_name.c_str());', + ' }', + ], + ) self.ScratchFile( 'MODULE.bazel', [ + 'module(name = "main", repo_name = "my_main")', 'bazel_dep(name = "aaa", version = "1.1")', 'bazel_dep(name = "bbb", version = "1.1")', 'archive_override(', ' module_name = "aaa",', ' urls = ["%s"],' % archive_aaa_1_0.as_uri(), - ' patches = ["//:aaa.patch"],', + ' patches = [', + ' "//:aaa.patch",', + ' "@//:aaa2.patch",', + ' "@my_main//:aaa3.patch",', + ' ":aaa4.patch",', + ' ],', ' patch_strip = 1,', ')', ], ) _, stdout, _ = self.RunBazel(['run', '//:main']) - self.assertIn('main function => aaa@1.0 (locally patched)', stdout) + self.assertIn( + 'main function => aaa@1.0 (locally patched all over again)', stdout + ) self.assertIn('main function => bbb@1.1', stdout) - self.assertIn('bbb@1.1 => aaa@1.0 (locally patched)', stdout) + self.assertIn('bbb@1.1 => aaa@1.0 (locally patched all over again)', stdout) def testGitOverride(self): self.writeMainProjectFiles() diff --git a/src/test/tools/bzlmod/MODULE.bazel.lock b/src/test/tools/bzlmod/MODULE.bazel.lock index dcf40eaf6abf1d..940e25521d70f8 100644 --- a/src/test/tools/bzlmod/MODULE.bazel.lock +++ b/src/test/tools/bzlmod/MODULE.bazel.lock @@ -228,7 +228,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -330,7 +330,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -356,7 +356,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -385,7 +385,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -491,7 +491,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -557,7 +557,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -586,7 +586,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -665,7 +665,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -698,7 +698,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -750,7 +750,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -780,7 +780,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -809,7 +809,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -839,7 +839,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -872,7 +872,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -958,7 +958,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -987,7 +987,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [ @@ -1017,7 +1017,7 @@ "local_config_platform": "local_config_platform@_" }, "repoSpec": { - "bzlFile": "@bazel_tools//tools/build_defs/repo:http.bzl", + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { "urls": [