Automated rollback of commit 337f19cc54e77c45daa1d5f61bf0a8d3daf8268f.

*** Reason for rollback ***

This breaks downstream projects and blocks Bazel's 0.9.0 release. See https://github.com/bazelbuild/bazel/issues/4249 for more information.

*** Original change description ***

Move override check to the createAndOverrideRule function

So we actually test for override also from skylark repositories.

Fixes #3908.

Change-Id: I7650a17834a6915a73c89df46989f72aa2f56920
PiperOrigin-RevId: 178370143

5 files changed, 17 insertions, 59 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
index 95ab59942c..ef6391b254 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
@@ -175,12 +175,7 @@ public class SkylarkRepositoryModule {
         @SuppressWarnings("unchecked")
         Map<String, Object> attributeValues = (Map<String, Object>) args[0];
         return WorkspaceFactoryHelper.createAndAddRepositoryRule(
-            context.getBuilder(),
-            ruleClass,
-            null,
-            attributeValues,
-            ast,
-            (Boolean) env.lookup("$allow_override"));
+            context.getBuilder(), ruleClass, null, attributeValues, ast);
       } catch (InvalidRuleException | NameConflictException | LabelSyntaxException e) {
         throw new EvalException(ast.getLocation(), e.getMessage());
       }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index 3283d99709..dff613b162 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -335,7 +335,7 @@ public class WorkspaceFactory {
                   // This effectively adds a "local_repository(name = "<ws>", path = ".")"
                   // definition to the WORKSPACE file.
                   WorkspaceFactoryHelper.createAndAddRepositoryRule(
-                      builder, localRepositoryRuleClass, bindRuleClass, kwargs, ast, allowOverride);
+                      builder, localRepositoryRuleClass, bindRuleClass, kwargs, ast);
                 } catch (InvalidRuleException | NameConflictException | LabelSyntaxException e) {
                   throw new EvalException(ast.getLocation(), e.getMessage());
                 }
@@ -451,11 +451,21 @@ public class WorkspaceFactory {
           throws EvalException, InterruptedException {
         try {
           Package.Builder builder = PackageFactory.getContext(env, ast).pkgBuilder;
+          if (!allowOverride
+              && kwargs.containsKey("name")
+              && builder.targets.containsKey(kwargs.get("name"))) {
+            throw new EvalException(
+                ast.getLocation(),
+                "Cannot redefine repository after any load statement in the WORKSPACE file"
+                    + " (for repository '"
+                    + kwargs.get("name")
+                    + "')");
+          }
           RuleClass ruleClass = ruleFactory.getRuleClass(ruleClassName);
           RuleClass bindRuleClass = ruleFactory.getRuleClass("bind");
           Rule rule =
               WorkspaceFactoryHelper.createAndAddRepositoryRule(
-                  builder, ruleClass, bindRuleClass, kwargs, ast, allowOverride);
+                  builder, ruleClass, bindRuleClass, kwargs, ast);
           if (!isLegalWorkspaceName(rule.getName())) {
             throw new EvalException(
                 ast.getLocation(), rule + "'s name field must be a legal workspace name");
@@ -505,7 +515,6 @@ public class WorkspaceFactory {
       workspaceEnv.setupDynamic(
           PackageFactory.PKG_CONTEXT,
           new PackageFactory.PackageContext(builder, null, localReporter, AttributeContainer::new));
-      workspaceEnv.setupDynamic("$allow_override", allowOverride);
     } catch (EvalException e) {
       throw new AssertionError(e);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
index 25e5bb8f54..42239f6506 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
@@ -23,7 +23,6 @@ import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Package.Builder;
 import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
-import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import java.util.Map;
 
@@ -35,20 +34,10 @@ public class WorkspaceFactoryHelper {
       RuleClass ruleClass,
       RuleClass bindRuleClass,
       Map<String, Object> kwargs,
-      FuncallExpression ast,
-      boolean allowOverride)
+      FuncallExpression ast)
       throws RuleFactory.InvalidRuleException, Package.NameConflictException, LabelSyntaxException,
-          InterruptedException, EvalException {
-    if (!allowOverride
-        && kwargs.containsKey("name")
-        && pkg.targets.containsKey(kwargs.get("name"))) {
-      throw new EvalException(
-          ast.getLocation(),
-          "Cannot redefine repository after any load statement in the WORKSPACE file"
-              + " (for repository '"
-              + kwargs.get("name")
-              + "')");
-    }
+          InterruptedException {
+
     StoredEventHandler eventHandler = new StoredEventHandler();
     BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs);
     Rule rule =
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 350249de05..7b716c3f3a 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, false);
+            packageBuilder, buildRuleClass(attributes), null, kwargs, ast);
     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 ad9c11a26a..cd636a9f08 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,39 +407,4 @@ 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");
-    }
-  }
 }