Forbid overloading of a repository outside of the first part of the workspace file
Fixes #824.
--
MOS_MIGRATED_REVID=114326952
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 90f4ea8..d300653 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
@@ -103,7 +103,7 @@
RuleClassProvider ruleClassProvider,
ImmutableList<EnvironmentExtension> environmentExtensions,
Mutability mutability) {
- this(builder, ruleClassProvider, environmentExtensions, mutability, null, null);
+ this(builder, ruleClassProvider, environmentExtensions, mutability, true, null, null);
}
// TODO(bazel-team): document installDir
@@ -120,6 +120,7 @@
RuleClassProvider ruleClassProvider,
ImmutableList<EnvironmentExtension> environmentExtensions,
Mutability mutability,
+ boolean allowOverride,
@Nullable Path installDir,
@Nullable Path workspaceDir) {
this.builder = builder;
@@ -127,7 +128,7 @@
this.installDir = installDir;
this.workspaceDir = workspaceDir;
this.environmentExtensions = environmentExtensions;
- this.workspaceFunctions = createWorkspaceFunctions(ruleClassProvider);
+ this.workspaceFunctions = createWorkspaceFunctions(ruleClassProvider, allowOverride);
}
/**
@@ -325,18 +326,29 @@
* specified package context.
*/
private static BuiltinFunction newRuleFunction(
- final RuleFactory ruleFactory, final String ruleClassName) {
+ final RuleFactory ruleFactory, final String ruleClassName, final boolean allowOverride) {
return new BuiltinFunction(
ruleClassName, FunctionSignature.KWARGS, BuiltinFunction.USE_AST_ENV) {
public Object invoke(Map<String, Object> kwargs, FuncallExpression ast, Environment env)
throws EvalException, InterruptedException {
try {
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 = builder
- .externalPackageData()
- .createAndAddRepositoryRule(builder, ruleClass, bindRuleClass, kwargs, ast);
+ Rule rule =
+ builder
+ .externalPackageData()
+ .createAndAddRepositoryRule(builder, ruleClass, bindRuleClass, kwargs, ast);
if (!isLegalWorkspaceName(rule.getName())) {
throw new EvalException(
ast.getLocation(), rule + "'s name field must be a legal workspace name");
@@ -352,13 +364,13 @@
}
private static ImmutableMap<String, BaseFunction> createWorkspaceFunctions(
- RuleClassProvider ruleClassProvider) {
+ RuleClassProvider ruleClassProvider, boolean allowOverride) {
ImmutableMap.Builder<String, BaseFunction> mapBuilder = ImmutableMap.builder();
RuleFactory ruleFactory = new RuleFactory(ruleClassProvider);
mapBuilder.put(BIND, newBindFunction(ruleFactory));
for (String ruleClass : ruleFactory.getRuleClassNames()) {
if (!ruleClass.equals(BIND)) {
- BaseFunction ruleFunction = newRuleFunction(ruleFactory, ruleClass);
+ BaseFunction ruleFunction = newRuleFunction(ruleFactory, ruleClass, allowOverride);
mapBuilder.put(ruleClass, ruleFunction);
}
}
@@ -405,7 +417,7 @@
}
public static ClassObject newNativeModule(RuleClassProvider ruleClassProvider) {
- return newNativeModule(createWorkspaceFunctions(ruleClassProvider));
+ return newNativeModule(createWorkspaceFunctions(ruleClassProvider, false));
}
static {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
index 2ab3267..8536558 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
@@ -87,6 +87,7 @@
ruleClassProvider,
packageFactory.getEnvironmentExtensions(),
mutability,
+ key.getIndex() == 0,
directories.getEmbeddedBinariesRoot(),
directories.getWorkspace());
if (key.getIndex() > 0) {
@@ -108,8 +109,6 @@
if (importResult == null) {
return null;
}
- // TODO(dmarting): give a nice error message when redefining a repository name and
- // getIndex() > 0.
parser.execute(ast, importResult.importMap);
} catch (PackageFunctionException | NameConflictException e) {
throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
index 9cab77d..9f60fd2 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
@@ -109,6 +109,7 @@
TestRuleClassProvider.getRuleClassProvider(),
ImmutableList.<PackageFactory.EnvironmentExtension>of(),
Mutability.create("test"),
+ true,
root,
root);
Exception exception = null;
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index 7f9e9e1..4040b86 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -81,7 +81,7 @@
")");
}
- private void setUpAttributeErrorTest() throws Exception {
+ private void setUpAttributeErrorTest() throws Exception {
scratch.file("test/BUILD",
"load('/test/macros', 'macro_native_rule', 'macro_skylark_rule', 'skylark_rule')",
"macro_native_rule(name = 'm_native',",
@@ -121,7 +121,7 @@
// about the macro.
assertContainsEvent(
"ERROR /workspace/test/BUILD:2:1: in deps attribute of cc_library rule //test:m_native: "
- + "java_library rule '//test:jlib' is misplaced here (expected ");
+ + "java_library rule '//test:jlib' is misplaced here (expected ");
// Skip the part of the error message that has details about the allowed deps since the mocks
// for the mac tests might have different values for them.
assertContainsEvent(". Since this "
@@ -141,9 +141,9 @@
// about the macro.
assertContainsEvent(
"ERROR /workspace/test/BUILD:4:1: in deps attribute of skylark_rule rule "
- + "//test:m_skylark: '//test:jlib' does not have mandatory provider 'some_provider'. "
- + "Since this rule was created by the macro 'macro_skylark_rule', the error might have "
- + "been caused by the macro implementation in /workspace/test/macros.bzl:12:36");
+ + "//test:m_skylark: '//test:jlib' does not have mandatory provider 'some_provider'. "
+ + "Since this rule was created by the macro 'macro_skylark_rule', the error might "
+ + "have been caused by the macro implementation in /workspace/test/macros.bzl:12:36");
}
}
@@ -216,8 +216,8 @@
getConfiguredTarget("//test:skyrule");
assertContainsEvent(
"ERROR /workspace/test/BUILD:3:10: Label '//test:sub/my_sub_lib.h' crosses boundary of "
- + "subpackage 'test/sub' (perhaps you meant to put the colon here: "
- + "'//test/sub:my_sub_lib.h'?)");
+ + "subpackage 'test/sub' (perhaps you meant to put the colon here: "
+ + "'//test/sub:my_sub_lib.h'?)");
}
@Test
@@ -650,10 +650,11 @@
@Test
public void testParamFileSuffix() throws Exception {
SkylarkRuleContext ruleContext = createRuleContext("//foo:foo");
- Object result = evalRuleContextCode(
- ruleContext,
- "ruleContext.new_file(ruleContext.files.tools[0], "
- + "ruleContext.files.tools[0].basename + '.params')");
+ Object result =
+ evalRuleContextCode(
+ ruleContext,
+ "ruleContext.new_file(ruleContext.files.tools[0], "
+ + "ruleContext.files.tools[0].basename + '.params')");
PathFragment fragment = ((Artifact) result).getRootRelativePath();
assertEquals("foo/t.exe.params", fragment.getPathString());
}
@@ -755,4 +756,46 @@
invalidatePackages();
assertThat(getConfiguredTarget("@r1//:test")).isNotNull();
}
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void testLoadBlockRepositoryRedefinition() throws Exception {
+ reporter.removeHandler(failFastHandler);
+ scratch.file("/bar/WORKSPACE");
+ scratch.file("/bar/bar.txt");
+ scratch.file("/bar/BUILD", "filegroup(name = 'baz', srcs = ['bar.txt'])");
+ scratch.file("/baz/WORKSPACE");
+ scratch.file("/baz/baz.txt");
+ scratch.file("/baz/BUILD", "filegroup(name = 'baz', srcs = ['baz.txt'])");
+ scratch.overwriteFile(
+ "WORKSPACE",
+ "local_repository(name = 'foo', path = '/bar')",
+ "local_repository(name = 'foo', path = '/baz')");
+ invalidatePackages();
+ assertThat(
+ (List<Label>)
+ getConfiguredTarget("@foo//:baz")
+ .getTarget()
+ .getAssociatedRule()
+ .getAttributeContainer()
+ .getAttr("srcs"))
+ .contains(Label.parseAbsolute("@foo//:baz.txt"));
+
+ scratch.overwriteFile("BUILD");
+ scratch.overwriteFile("bar.bzl", "dummy = 1");
+ scratch.overwriteFile(
+ "WORKSPACE",
+ "local_repository(name = 'foo', path = '/bar')",
+ "load('//:bar.bzl', 'dummy')",
+ "local_repository(name = 'foo', path = '/baz')");
+ try {
+ invalidatePackages();
+ createRuleContext("@foo//:baz");
+ fail("Should have failed because repository 'foo' is overloading after a load!");
+ } catch (Exception ex) {
+ assertContainsEvent(
+ "ERROR /workspace/WORKSPACE:3:1: Cannot redefine repository after any load statement "
+ + "in the WORKSPACE file (for repository 'foo')");
+ }
+ }
}
