From eac799dfb5b73c49ff981e53d0941d4b82dc7156 Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Sun, 3 Jul 2022 11:05:00 +0200
Subject: [PATCH] Fail the build on repository rule errors in a module
 extension

Also emit a Starlark stack trace pointing to the particular call site
of the repository rule.

Previously, the root cause error would be printed, but the build would
continue.

Fixes https://github.com/bazelbuild/bazel/issues/14526#issuecomment-1149683214
---
 .../bazel/bzlmod/BzlmodRepoRuleCreator.java   |  8 ++++-
 .../lib/skyframe/BzlmodRepoRuleFunction.java  |  7 +++++
 src/test/py/bazel/bzlmod/bazel_module_test.py | 30 +++++++++++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java
index a3aa839b0d2738..c26fe22d4a5502 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java
@@ -31,6 +31,8 @@
 import com.google.devtools.build.lib.vfs.Root;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import java.util.Map;
+import net.starlark.java.eval.EvalException;
+import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkSemantics;
 import net.starlark.java.eval.StarlarkThread.CallStackEntry;
 import net.starlark.java.syntax.Location;
@@ -52,7 +54,7 @@ public static Rule createRule(
       String callStackEntry,
       RuleClass ruleClass,
       Map<String, Object> attributes)
-      throws InterruptedException, InvalidRuleException, NoSuchPackageException {
+      throws InterruptedException, InvalidRuleException, NoSuchPackageException, EvalException {
     // TODO(bazel-team): Don't use the {@link Rule} class for repository rule.
     // Currently, the repository rule is represented with the {@link Rule} class that's designed
     // for build rules. Therefore, we have to create a package instance for it, which doesn't make
@@ -79,6 +81,10 @@ public static Rule createRule(
       // This literally cannot happen -- we just created the package!
       throw new IllegalStateException(e);
     }
+    if (rule.containsErrors()) {
+      throw Starlark.errorf(
+          "failed to instantiate '%s' from this module extension", ruleClass.getName());
+    }
     packageBuilder.build();
     return rule;
   }
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 be9ca873c066fd..74587ed9086863 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
@@ -44,6 +44,7 @@
 import java.io.IOException;
 import java.util.Optional;
 import javax.annotation.Nullable;
+import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Module;
 import net.starlark.java.eval.StarlarkSemantics;
 import net.starlark.java.syntax.Location;
@@ -152,6 +153,8 @@ private BzlmodRepoRuleValue createRuleFromSpec(
       throw new BzlmodRepoRuleFunctionException(e, Transience.PERSISTENT);
     } catch (NoSuchPackageException e) {
       throw new BzlmodRepoRuleFunctionException(e, Transience.PERSISTENT);
+    } catch (EvalException e) {
+      throw new BzlmodRepoRuleFunctionException(e, Transience.PERSISTENT);
     }
   }
 
@@ -219,5 +222,9 @@ private static final class BzlmodRepoRuleFunctionException extends SkyFunctionEx
     BzlmodRepoRuleFunctionException(IOException e, Transience transience) {
       super(e, transience);
     }
+
+    BzlmodRepoRuleFunctionException(EvalException e, Transience transience) {
+      super(e, transience);
+    }
   }
 }
diff --git a/src/test/py/bazel/bzlmod/bazel_module_test.py b/src/test/py/bazel/bzlmod/bazel_module_test.py
index ae3eb95e1db12c..a3824e3013f8af 100644
--- a/src/test/py/bazel/bzlmod/bazel_module_test.py
+++ b/src/test/py/bazel/bzlmod/bazel_module_test.py
@@ -335,6 +335,36 @@ def testCheckDirectDependencies(self):
         'ERROR: For repository \'B\', the root module requires module version B@1.0, but got B@1.1 in the resolved dependency graph.',
         stderr)
 
+  def testRepositoryRuleErrorInModuleExtensionFailsTheBuild(self):
+    self.ScratchFile('MODULE.bazel', [
+        'module_ext = use_extension("//pkg:extension.bzl", "module_ext")',
+        'use_repo(module_ext, "foo")',
+    ])
+    self.ScratchFile('pkg/BUILD.bazel')
+    self.ScratchFile('pkg/rules.bzl', [
+        'def _repo_rule_impl(ctx):',
+        '    ctx.file("WORKSPACE")',
+        'repo_rule = repository_rule(implementation = _repo_rule_impl)',
+    ])
+    self.ScratchFile('pkg/extension.bzl', [
+        'load(":rules.bzl", "repo_rule")',
+        'def _module_ext_impl(ctx):',
+        '    repo_rule(name = "foo", invalid_attr = "value")',
+        'module_ext = module_extension(implementation = _module_ext_impl)',
+    ])
+    exit_code, _, stderr = self.RunBazel(
+        ['run', '@foo//...'],
+        allow_failure=True)
+    self.AssertExitCode(exit_code, 48, stderr)
+    self.assertIn(
+        "ERROR: <builtin>: //pkg:@.module_ext.foo: no such attribute 'invalid_attr' in 'repo_rule' rule",
+        stderr)
+    self.assertTrue(any(
+        [
+          '/pkg/extension.bzl", line 3, column 14, in _module_ext_impl' in line
+          for line in stderr
+        ]))
+
 
 if __name__ == '__main__':
   unittest.main()