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
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 95ab599..ef6391b 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 @@
@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 3283d99..dff613b 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 @@
// 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 @@
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 @@
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 25e5bb8..42239f6 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.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 @@
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 350249d..7b716c3 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 @@
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 ad9c11a..cd636a9 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 @@
// 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");
- }
- }
}
