Move override check to the createAndOverrideRule function

So we actually test for override also from skylark repositories.

Fixes #3908.

Change-Id: I7650a17834a6915a73c89df46989f72aa2f56920
PiperOrigin-RevId: 176645218

2 files changed, 36 insertions, 1 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
index 7b716c3f3a..350249de05 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
@@ -86,7 +86,7 @@ public class SkylarkRepositoryContextTest {
     ast.setLocation(Location.BUILTIN);
     Rule rule =
         WorkspaceFactoryHelper.createAndAddRepositoryRule(
-            packageBuilder, buildRuleClass(attributes), null, kwargs, ast);
+            packageBuilder, buildRuleClass(attributes), null, kwargs, ast, false);
     HttpDownloader downloader = Mockito.mock(HttpDownloader.class);
     context =
         new SkylarkRepositoryContext(
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
index cd636a9f08..ad9c11a26a 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
@@ -407,4 +407,39 @@ public class SkylarkRepositoryIntegrationTest extends BuildViewTestCase {
     // Just request the last external repository to force the whole loading.
     getConfiguredTarget("@foo//:bar");
   }
+
+  // Regression test for https://github.com/bazelbuild/bazel/issues/3908
+  @Test
+  public void testSkylarkCannotOverrideRules() throws Exception {
+    scratch.overwriteFile(
+        rootDirectory.getRelative("WORKSPACE").getPathString(),
+        ImmutableList.<String>builder()
+            .addAll(analysisMock.getWorkspaceContents(mockToolsConfig))
+            .add("local_repository(name = 'local_repo', path = '/local_repo')") //
+            .add("load('//:test.bzl', 'my_repo')") //
+            .add("my_repo(name = 'local_repo')") //
+            .add("local_repository(name = 'foo', path = '/local_repo')")
+            .build());
+    scratch.file(
+        rootDirectory.getRelative("test.bzl").getPathString(), //
+        "def _impl():", //
+        "  print('BLEH')", //
+        "", //
+        "my_repo = repository_rule(_impl)");
+    scratch.file(rootDirectory.getRelative("BUILD").getPathString());
+    scratch.file("/local_repo/WORKSPACE");
+    scratch.file(
+        "/local_repo/BUILD",
+        "filegroup(name = 'test', srcs = [], visibility = ['//visibility:public'])");
+    // Just request the last external repository to force the whole loading.
+    try {
+      invalidatePackages();
+      getConfiguredTarget("@foo//:test");
+      fail();
+    } catch (AssertionError e) {
+      assertThat(e)
+          .hasMessageThat()
+          .contains("Cannot redefine repository after any load statement in the WORKSPACE file");
+    }
+  }
 }