[7.0.0] Fix JVM repository rules with Bzlmod (bazelbuild#20088)

* Lets the `http_jar` and `jvm_{maven_}import_external` rules load from
`@rules_java` as resolved within `@bazel_tools` rather than the calling
module extension's repository.
* Uses the user-provided apparent repository name in
`jvm_maven_import_external` to generate a target name for the jar that
makes the shorthand `@my_import` form work with Bzlmod.
* Ensure that Bazel loads the `http_jar` rule with the full repo mapping
for `bazel_tools` so that it sees `rules_java`, while still loading
`http_archive` with a trivial repo mapping to prevent a cycle during
module repo creation. This requires introducing a new key type for
`BzlLoadFunction`.

Closes bazelbuild#19997.

PiperOrigin-RevId: 579002601
Change-Id: I94c85e9759d0913384b10b9bbd0d0cb960cdd89a

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitOverride.java

@@ -24,6 +24,8 @@
 /** Specifies that a module should be retrieved from a Git repository. */
 @AutoValue
 public abstract class GitOverride implements NonRegistryOverride {
+  public static final String GIT_REPOSITORY_PATH = "@bazel_tools//tools/build_defs/repo:git.bzl";
+
   public static GitOverride create(
       String remote,
       String commit,
@@ -60,7 +62,7 @@ public RepoSpec getRepoSpec(RepositoryName repoName) {
         .put("patch_cmds", getPatchCmds())
         .put("patch_args", ImmutableList.of("-p" + getPatchStrip()));
     return RepoSpec.builder()
-        .setBzlFile("@bazel_tools//tools/build_defs/repo:git.bzl")
+        .setBzlFile(GIT_REPOSITORY_PATH)
         .setRuleClassName("git_repository")
         .setAttributes(AttributeValues.create(attrBuilder.buildOrThrow()))
         .build();

src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java

@@ -619,21 +619,8 @@ private static boolean requiresBuiltinsInjection(BzlLoadValue.Key key) {
         // https://github.com/bazelbuild/bazel/issues/17713
         // `@_builtins` depends on `@bazel_tools` for repo mapping, so we ignore some bzl files
         // to avoid a cyclic dependency
-        || (key instanceof BzlLoadValue.KeyForBzlmod && !isFileSafeForUninjectedEvaluation(key));
-  }
-
-  private static final PackageIdentifier BAZEL_TOOLS_BOOTSTRAP_RULES_PACKAGE =
-      PackageIdentifier.create(
-          RepositoryName.BAZEL_TOOLS, PathFragment.create("tools/build_defs/repo"));
-
-  private static boolean isFileSafeForUninjectedEvaluation(BzlLoadValue.Key key) {
-    // We don't inject _builtins for repo rules to avoid a Skyframe cycle.
-    // The cycle is caused only with bzlmod because the `@_builtins` repo does not declare its own
-    // module deps and requires `@bazel_tools` to re-use the latter's repo mapping. This triggers
-    // Bazel module resolution, and if there are any non-registry overrides in the root MODULE.bazel
-    // file (such as `git_override` or `archive_override`), the corresponding bzl files will be
-    // evaluated.
-    return key.getLabel().getPackageIdentifier().equals(BAZEL_TOOLS_BOOTSTRAP_RULES_PACKAGE);
+        || (key instanceof BzlLoadValue.KeyForBzlmod
+            && !(key instanceof BzlLoadValue.KeyForBzlmodBootstrap));
   }
 
   /**
@@ -946,7 +933,7 @@ private static RepositoryMapping getRepositoryMapping(
     }
 
     if (key instanceof BzlLoadValue.KeyForBzlmod) {
-      if (key.getLabel().getPackageIdentifier().equals(BAZEL_TOOLS_BOOTSTRAP_RULES_PACKAGE)) {
+      if (key instanceof BzlLoadValue.KeyForBzlmodBootstrap) {
         // Special case: we're only here to get one of the rules in the @bazel_tools repo that
         // load Bazel modules. At this point we can't load from any other modules and thus use a
         // repository mapping that contains only @bazel_tools itself.
@@ -1324,7 +1311,7 @@ private ImmutableMap<String, Object> getAndDigestPredeclaredEnvironment(
         // TODO(#11954): We should converge all .bzl dialects regardless of whether they're loaded
         //  by BUILD, WORKSPACE, or MODULE. At the moment, WORKSPACE-loaded and MODULE-loaded .bzl
         //  files are already converged, so we use the same environment for both.
-        if (injectionDisabled || isFileSafeForUninjectedEvaluation(key)) {
+        if (injectionDisabled || key instanceof BzlLoadValue.KeyForBzlmodBootstrap) {
           return starlarkEnv.getUninjectedWorkspaceBzlEnv();
         }
         // Note that we don't actually fingerprint the injected builtins here. The actual builtins

src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadValue.java

@@ -17,7 +17,9 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.BzlVisibility;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
@@ -340,7 +342,7 @@ BzlCompileValue.Key getCompileKey(Root root) {
   /** A key for loading a .bzl to get the repo rule required by Bzlmod generated repositories. */
   @Immutable
   @AutoCodec.VisibleForSerialization
-  static final class KeyForBzlmod extends Key {
+  static class KeyForBzlmod extends Key {
     private final Label label;
 
     private KeyForBzlmod(Label label) {
@@ -363,9 +365,22 @@ BzlCompileValue.Key getCompileKey(Root root) {
     }
   }
 
+  @Immutable
+  @AutoCodec.VisibleForSerialization
+  static class KeyForBzlmodBootstrap extends KeyForBzlmod {
+    private KeyForBzlmodBootstrap(Label label) {
+      super(label);
+    }
+
+    @Override
+    Key getKeyForLoad(Label loadLabel) {
+      return keyForBzlmodBootstrap(loadLabel);
+    }
+  }
+
   /** Constructs a key for loading a regular (non-workspace) .bzl file, from the .bzl's label. */
   public static Key keyForBuild(Label label) {
-    return keyInterner.intern(new KeyForBuild(label, /*isBuildPrelude=*/ false));
+    return keyInterner.intern(new KeyForBuild(label, /* isBuildPrelude= */ false));
   }
 
   /**
@@ -387,11 +402,18 @@ static Key keyForBuiltins(Label label) {
 
   /** Constructs a key for loading the special prelude .bzl. */
   static Key keyForBuildPrelude(Label label) {
-    return keyInterner.intern(new KeyForBuild(label, /*isBuildPrelude=*/ true));
+    return keyInterner.intern(new KeyForBuild(label, /* isBuildPrelude= */ true));
   }
 
   /** Constructs a key for loading a .bzl for Bzlmod repos */
   public static Key keyForBzlmod(Label label) {
     return keyInterner.intern(new KeyForBzlmod(label));
   }
+
+  public static Key keyForBzlmodBootstrap(Label label) {
+    Preconditions.checkArgument(
+        label.getRepository().equals(RepositoryName.BAZEL_TOOLS),
+        "keyForBzlmodBootstrap must be called with a label in the bazel_tools repository");
+    return keyInterner.intern(new KeyForBzlmodBootstrap(label));
+  }
 }

src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java

@@ -19,12 +19,14 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.bazel.bzlmod.ArchiveRepoSpecBuilder;
 import com.google.devtools.build.lib.bazel.bzlmod.AttributeValues;
 import com.google.devtools.build.lib.bazel.bzlmod.BazelDepGraphValue;
 import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleCreator;
 import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue;
+import com.google.devtools.build.lib.bazel.bzlmod.GitOverride;
 import com.google.devtools.build.lib.bazel.bzlmod.ModuleExtensionId;
 import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue;
 import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue;
@@ -53,6 +55,7 @@
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Optional;
+import java.util.Set;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Module;
@@ -185,13 +188,13 @@ private BzlmodRepoRuleValue createRuleFromSpec(
       ruleClass = ruleClassProvider.getRuleClassMap().get(repoSpec.ruleClassName());
     } else {
       ImmutableMap<String, Module> loadedModules =
-          loadBzlModules(env, repoSpec.bzlFile(), starlarkSemantics);
+          loadBzlModules(env, repoSpec.bzlFile(), repoSpec.getRuleClass(), starlarkSemantics);
       if (env.valuesMissing()) {
         return null;
       }
       ruleClass = getStarlarkRuleClass(repoSpec, loadedModules);
     }
-    
+
     try {
       Rule rule =
           BzlmodRepoRuleCreator.createRule(
@@ -242,9 +245,16 @@ private AttributeValues resolveRemotePatchesUrl(RepoSpec repoSpec) {
     return repoSpec.attributes();
   }
 
+  // Starlark rules loaded from bazel_tools that may define Bazel module repositories and thus must
+  // be loaded without relying on any other modules.
+  private static final Set<String> BOOTSTRAP_RULE_CLASSES =
+      ImmutableSet.of(
+          ArchiveRepoSpecBuilder.HTTP_ARCHIVE_PATH + "%http_archive",
+          GitOverride.GIT_REPOSITORY_PATH + "%git_repository");
+
   /** Loads modules from the given bzl file. */
   private ImmutableMap<String, Module> loadBzlModules(
-      Environment env, String bzlFile, StarlarkSemantics starlarkSemantics)
+      Environment env, String bzlFile, String ruleClass, StarlarkSemantics starlarkSemantics)
       throws InterruptedException, BzlmodRepoRuleFunctionException {
     ImmutableList<Pair<String, Location>> programLoads =
         ImmutableList.of(Pair.of(bzlFile, Location.BUILTIN));
@@ -268,8 +278,12 @@ private ImmutableMap<String, Module> loadBzlModules(
     }
 
     Preconditions.checkArgument(loadLabels.size() == 1);
-    ImmutableList<BzlLoadValue.Key> keys =
-        ImmutableList.of(BzlLoadValue.keyForBzlmod(loadLabels.get(0)));
+    ImmutableList<BzlLoadValue.Key> keys;
+    if (BOOTSTRAP_RULE_CLASSES.contains(ruleClass)) {
+      keys = ImmutableList.of(BzlLoadValue.keyForBzlmodBootstrap(loadLabels.get(0)));
+    } else {
+      keys = ImmutableList.of(BzlLoadValue.keyForBzlmod(loadLabels.get(0)));
+    }
 
     // Load the .bzl module.
     try {

src/test/py/bazel/bzlmod/bazel_module_test.py

@@ -785,6 +785,33 @@ def testLoadRulesJavaSymbolThroughBazelTools(self):
 
     self.RunBazel(['build', '@data//:data.txt'])
 
+  def testHttpJar(self):
+    """Tests that using http_jar does not require a bazel_dep on rules_java."""
+
+    my_jar_path = self.ScratchFile('my_jar.jar')
+    my_jar_uri = pathlib.Path(my_jar_path).as_uri()
+
+    self.ScratchFile(
+        'MODULE.bazel',
+        [
+            (
+                'http_jar ='
+                ' use_repo_rule("@bazel_tools//tools/build_defs/repo:http.bzl",'
+                ' "http_jar")'
+            ),
+            'http_jar(',
+            '  name = "my_jar",',
+            '  url = "%s",' % my_jar_uri,
+            (
+                '  sha256 ='
+                ' "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",'
+            ),
+            ')',
+        ],
+    )
+
+    self.RunBazel(['build', '@my_jar//jar'])
+
 
 if __name__ == '__main__':
   absltest.main()

tools/build_defs/repo/http.bzl

@@ -192,7 +192,7 @@ def _http_file_impl(ctx):
     return _update_sha256_attr(ctx, _http_file_attrs, download_info)
 
 _HTTP_JAR_BUILD = """\
-load("@rules_java//java:defs.bzl", "java_import")
+load("{rules_java_defs}", "java_import")
 
 package(default_visibility = ["//visibility:public"])
 
@@ -224,7 +224,10 @@ def _http_jar_impl(ctx):
         integrity = ctx.attr.integrity,
     )
     ctx.file("WORKSPACE", "workspace(name = \"{name}\")".format(name = ctx.name))
-    ctx.file("jar/BUILD", _HTTP_JAR_BUILD.format(file_name = downloaded_file_name))
+    ctx.file("jar/BUILD", _HTTP_JAR_BUILD.format(
+        file_name = downloaded_file_name,
+        rules_java_defs = str(Label("@rules_java//java:defs.bzl")),
+    ))
 
     return _update_sha256_attr(ctx, _http_jar_attrs, download_info)
 

tools/build_defs/repo/jvm.bzl

@@ -136,7 +136,7 @@ def _jvm_import_external(repository_ctx):
         "",
         "alias(",
         "    name = \"%s\"," % extension,
-        "    actual = \"@%s\"," % repository_ctx.name,
+        "    actual = \"//:%s\"," % name,
         ")",
         "",
         "filegroup(",
@@ -255,6 +255,7 @@ jvm_import_external = repository_rule(
 )
 
 def jvm_maven_import_external(
+        name,
         artifact,
         server_urls,
         fetch_sources = False,
@@ -269,9 +270,10 @@ def jvm_maven_import_external(
     srcjar_urls = kwargs.pop("srcjar_urls", None)
 
     rule_name = kwargs.pop("rule_name", "java_import")
+    rules_java_defs = str(Label("@rules_java//java:defs.bzl"))
     rule_load = kwargs.pop(
         "rule_load",
-        'load("@rules_java//java:defs.bzl", "java_import")',
+        'load("{}", "java_import")'.format(rules_java_defs),
     )
 
     if fetch_sources:
@@ -289,6 +291,8 @@ def jvm_maven_import_external(
     tags.append("maven_coordinates=" + artifact)
 
     jvm_import_external(
+        name = name,
+        generated_rule_name = kwargs.pop("generated_rule_name", name),
         artifact_urls = jar_urls,
         srcjar_urls = srcjar_urls,
         canonical_id = artifact,