Allow overridding any rule in the WORKSPACE file

...instead of throwing an uncaught exception and printing a stack trace. Fixes #409.

--
MOS_MIGRATED_REVID=111850179

3 files changed, 31 insertions, 65 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java b/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java
index 8ea4ceec26..8570899691 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java
@@ -18,7 +18,6 @@ import com.google.common.base.Verify;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
@@ -31,19 +30,6 @@ import java.util.Map;
  */
 public class ExternalPackageBuilder {
 
-  private Map<PackageIdentifier.RepositoryName, Rule> repositoryMap = Maps.newLinkedHashMap();
-
-  void build(Package.Builder pkg) {
-    for (Rule rule : repositoryMap.values()) {
-      try {
-        pkg.addRule(rule);
-      } catch (Package.NameConflictException e) {
-        throw new IllegalStateException(
-            "Got a name conflict for " + rule + ", which can't happen: " + e.getMessage(), e);
-      }
-    }
-  }
-
   public ExternalPackageBuilder createAndAddRepositoryRule(
       Package.Builder pkg,
       RuleClass ruleClass,
@@ -57,7 +43,7 @@ public class ExternalPackageBuilder {
     Rule tempRule =
         RuleFactory.createRule(pkg, ruleClass, kwargs, eventHandler, ast, ast.getLocation(), null);
     pkg.addEvents(eventHandler.getEvents());
-    repositoryMap.put(PackageIdentifier.RepositoryName.create("@" + tempRule.getName()), tempRule);
+    overwriteRule(pkg, tempRule);
     for (Map.Entry<String, Label> entry :
         ruleClass.getExternalBindingsFunction().apply(tempRule).entrySet()) {
       Label nameLabel = Label.parseAbsolute("//external:" + entry.getKey());
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index f6ec1e86eb..ecf1df70dc 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -1288,8 +1288,6 @@ public class Package {
         return pkg;
       }
 
-      externalPackageData.build(this);
-
       // Freeze targets and distributions.
       targets = ImmutableMap.copyOf(targets);
       defaultDistributionSet =
diff --git a/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java b/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java
index 999da0b5ba..12898477d4 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java
@@ -15,15 +15,8 @@ package com.google.devtools.build.lib.packages;
 
 import static org.junit.Assert.assertEquals;
 
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
-import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.packages.Package.Builder;
-import com.google.devtools.build.lib.syntax.Argument;
-import com.google.devtools.build.lib.syntax.FuncallExpression;
-import com.google.devtools.build.lib.syntax.Identifier;
-import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 
 import org.junit.Before;
@@ -31,8 +24,6 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-import java.util.Map;
-
 /**
  * Test for building external packages.
  */
@@ -43,51 +34,42 @@ public class ExternalPackageTest extends BuildViewTestCase {
 
   @Before
   public final void setWorkspacePath() throws Exception {
-    workspacePath = getOutputPath().getRelative("WORKSPACE");
-  }
-
-  @Test
-  public void testWorkspaceName() {
-    Builder builder = Package.newExternalPackageBuilder(workspacePath, "TESTING");
-    builder.setWorkspaceName("foo");
-    assertEquals("foo", builder.build().getWorkspaceName());
+    workspacePath = rootDirectory.getRelative("WORKSPACE");
   }
 
   @Test
   public void testMultipleRulesWithSameName() throws Exception {
-    Builder builder = Package.newExternalPackageBuilder(workspacePath, "TESTING");
-
-    // The WORKSPACE file allows rules to be overridden.
-    Location buildFile = Location.fromFile(getOutputPath().getRelative("WORKSPACE"));
+    FileSystemUtils.writeIsoLatin1(workspacePath,
+        "local_repository(",
+        "    name = 'my-rule',",
+        "    path = '/foo/bar',",
+        ")",
+        "new_local_repository(",
+        "    name = 'my-rule',",
+        "    path = '/foo/bar',",
+        "    build_file = 'baz',",
+        ")");
 
-    // Add first rule.
-    RuleClass ruleClass =
-        TestRuleClassProvider.getRuleClassProvider().getRuleClassMap().get("local_repository");
-    RuleClass bindRuleClass =
-        TestRuleClassProvider.getRuleClassProvider().getRuleClassMap().get("bind");
-
-    Map<String, Object> kwargs = ImmutableMap.of("name", (Object) "my-rule");
-    FuncallExpression ast =
-        new FuncallExpression(
-            new Identifier(ruleClass.getName()), Lists.<Argument.Passed>newArrayList());
-    ast.setLocation(buildFile);
-    builder
-        .externalPackageData()
-        .createAndAddRepositoryRule(builder, ruleClass, bindRuleClass, kwargs, ast);
+    invalidatePackages();
+    // Make sure the second rule "wins."
+    assertEquals("new_local_repository rule", getTarget("//external:my-rule").getTargetKind());
+  }
 
-    // Add another rule with the same name.
-    ruleClass =
-        TestRuleClassProvider.getRuleClassProvider().getRuleClassMap().get("new_local_repository");
-    ast =
-        new FuncallExpression(
-            new Identifier(ruleClass.getName()), Lists.<Argument.Passed>newArrayList());
-    ast.setLocation(buildFile);
-    builder
-        .externalPackageData()
-        .createAndAddRepositoryRule(builder, ruleClass, bindRuleClass, kwargs, ast);
-    Package pkg = builder.build();
+  @Test
+  public void testOverridingBindRules() throws Exception {
+    FileSystemUtils.writeIsoLatin1(workspacePath,
+        "bind(",
+        "    name = 'my-rule',",
+        "    actual = '//foo:bar',",
+        ")",
+        "new_local_repository(",
+        "    name = 'my-rule',",
+        "    path = '/foo/bar',",
+        "    build_file = 'baz',",
+        ")");
 
+    invalidatePackages();
     // Make sure the second rule "wins."
-    assertEquals("new_local_repository rule", pkg.getTarget("my-rule").getTargetKind());
+    assertEquals("new_local_repository rule", getTarget("//external:my-rule").getTargetKind());
   }
 }