bazel packages: record Starlark stack in RuleClass at creation
This change does three things:
1) reduces the space used by instances of Rule, which are numerous;
2) causes RuleClass instances to record the Starlark stack upon creation;
3) reports the RuleClass stack in query --output=build.
The motivation for this change was to remove a field from class Rule to
nullify the space increase of the Rule.callstack field added to support
the --record_rule_instantiation_callstack feature. The removed field,
Rule.definitionInformation, provided debugging information for certain
errors, such as a repo cycle; notably it was a pile of text, and was
populated only for repository rules. It did not warrant a field in such
a high-cardinality object.
Now, the same debugging information is provided by computing it on the
fly from structured information: Rule.callstack and RuleClass.callstack.
In addition, RuleClass.callstack is reported to users through query.
This requires that we record stacks not just for rule instantiation
(added in commit 40a737c8f369270490e00b69d98c8c8ea31ff697 / commit 40a737c, subject to --record_rule_instantiation_callstack),
but also for their RuleClass declarations, which we now do unconditionally.
Details:
- plumb (semantics, callstack) to rule instantiation instead of (@Nullable thread, location).
- inline BRF.addRule, WFH.createAndAddRepositoryRule.
- eliminate location parameter to WFH.addRepoMappings, fix unsafe cast, and test.
- improve the format of the "definition information" pile of text so that it
includes both stacks. (Given than no-one cared enough about it to bother adding
a test, this is probably wasted effort and a good deed that will be punished.)
- RuleClass.callstack is not currently subject to serialization.
One behavior change: The location associated with a repository rule was formerly
that of the innermost call at instantiation. Now, because the call stack is plumbed
down to createRule, both rule.location and generator.location are set to the outermost
call, just as ordinary package rules have behaved since Google Issue 23974287 was
fixed in 2015 (CL 103004059 / commit 0cb41b0).
RELNOTES:
'query --output=build' now shows where rule classes (not just rules) are created.
The location associated with a repository rule is now that of the
outermost function call at instantiation, not the innermost call.
PiperOrigin-RevId: 302105043
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
index 7100116..aaef7d5 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
@@ -307,6 +307,10 @@
// We'll set the name later, pass the empty string for now.
RuleClass.Builder builder = new RuleClass.Builder("", type, true, parent);
+
+ ImmutableList<StarlarkThread.CallStackEntry> callstack = thread.getCallStack();
+ builder.setCallStack(callstack.subList(0, callstack.size() - 1)); // pop 'rule' itself
+
ImmutableList<Pair<String, SkylarkAttr.Descriptor>> attributes =
attrObjectToAttributesList(attrs);
@@ -661,8 +665,8 @@
pkgContext,
ruleClass,
attributeValues,
- thread.getCallerLocation(),
- thread,
+ thread.getSemantics(),
+ thread.getCallStack(),
new AttributeContainer(ruleClass));
} catch (InvalidRuleException | NameConflictException e) {
throw new EvalException(null, e.getMessage());
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryResolvedEvent.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryResolvedEvent.java
index 9838e9b..a77d6fa 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryResolvedEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryResolvedEvent.java
@@ -21,7 +21,6 @@
import static com.google.devtools.build.lib.rules.repository.ResolvedHashesFunction.REPOSITORIES;
import static com.google.devtools.build.lib.rules.repository.ResolvedHashesFunction.RULE_CLASS;
-import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.events.ExtendedEventHandler.ResolvedEvent;
@@ -31,6 +30,7 @@
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.Printer;
import com.google.devtools.build.lib.syntax.Starlark;
+import com.google.devtools.build.lib.syntax.StarlarkThread;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.Path;
import java.io.IOException;
@@ -82,10 +82,7 @@
String originalClass =
rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel() + "%" + rule.getRuleClass();
resolvedInformationBuilder.put(ORIGINAL_RULE_CLASS, originalClass);
-
- if (!Strings.isNullOrEmpty(rule.getDefinitionInformation())) {
- resolvedInformationBuilder.put(DEFINITION_INFORMATION, rule.getDefinitionInformation());
- }
+ resolvedInformationBuilder.put(DEFINITION_INFORMATION, getRuleDefinitionInformation(rule));
ImmutableMap.Builder<String, Object> origAttrBuilder = ImmutableMap.builder();
ImmutableMap.Builder<String, Object> defaults = ImmutableMap.builder();
@@ -218,6 +215,35 @@
return message;
}
+ /** Returns an unstructured message explaining the origin of this rule. */
+ public static String getRuleDefinitionInformation(Rule rule) {
+ StringBuilder buf = new StringBuilder();
+
+ // Emit stack of rule instantiation.
+ buf.append("Repository ").append(rule.getName()).append(" instantiated at:\n");
+ ImmutableList<StarlarkThread.CallStackEntry> stack = rule.getCallStack().toList();
+ if (stack.isEmpty()) {
+ buf.append(" no stack (--record_rule_instantiation_callstack not enabled)\n");
+ } else {
+ for (StarlarkThread.CallStackEntry frame : stack) {
+ buf.append(" ").append(frame.location).append(": in ").append(frame.name).append('\n');
+ }
+ }
+
+ // Emit stack of rule class declaration.
+ stack = rule.getRuleClassObject().getCallStack();
+ if (stack.isEmpty()) {
+ buf.append("Repository rule ").append(rule.getRuleClass()).append(" is built-in.\n");
+ } else {
+ buf.append("Repository rule ").append(rule.getRuleClass()).append(" defined at:\n");
+ for (StarlarkThread.CallStackEntry frame : stack) {
+ buf.append(" ").append(frame.location).append(": in ").append(frame.name).append('\n');
+ }
+ }
+
+ return buf.toString();
+ }
+
/**
* Compare two maps from Strings to objects, returning a pair of the map with all entries not in
* the original map or in the original map, but with a different value, and the keys dropped from
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
index 41975a5..899f3fd 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
@@ -15,7 +15,6 @@
package com.google.devtools.build.lib.bazel.repository.skylark;
import com.google.common.base.Preconditions;
-import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -99,12 +98,10 @@
Map<String, String> markerData,
SkyKey key)
throws RepositoryFunctionException, InterruptedException {
- if (rule.getDefinitionInformation() != null) {
- env.getListener()
- .post(
- new SkylarkRepositoryDefinitionLocationEvent(
- rule.getName(), rule.getDefinitionInformation()));
- }
+
+ String defInfo = RepositoryResolvedEvent.getRuleDefinitionInformation(rule);
+ env.getListener().post(new SkylarkRepositoryDefinitionLocationEvent(rule.getName(), defInfo));
+
StarlarkFunction function = rule.getRuleClassObject().getConfiguredTargetFunction();
if (declareEnvironmentDependencies(markerData, env, getEnviron(rule)) == null) {
return null;
@@ -212,7 +209,7 @@
rule, skylarkRepositoryContext.getAttr(), outputDirectory, result);
if (resolved.isNewInformationReturned()) {
env.getListener().handle(Event.debug(resolved.getMessage()));
- env.getListener().handle(Event.debug(rule.getDefinitionInformation()));
+ env.getListener().handle(Event.debug(defInfo));
}
String ruleClass =
@@ -248,9 +245,9 @@
+ rule.getName()
+ "':\n "
+ e.getMessage()));
- if (!Strings.isNullOrEmpty(rule.getDefinitionInformation())) {
- env.getListener().handle(Event.info(rule.getDefinitionInformation()));
- }
+ env.getListener()
+ .handle(Event.info(RepositoryResolvedEvent.getRuleDefinitionInformation(rule)));
+
throw new RepositoryFunctionException(e, Transience.TRANSIENT);
}
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 8bedec7..ff4cc57 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
@@ -20,12 +20,11 @@
import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
import static com.google.devtools.build.lib.syntax.SkylarkType.castMap;
-import com.google.common.collect.Lists;
+import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.analysis.BaseRuleClasses;
import com.google.devtools.build.lib.analysis.skylark.SkylarkAttr.Descriptor;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
-import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.AttributeValueSource;
import com.google.devtools.build.lib.packages.BazelStarlarkContext;
import com.google.devtools.build.lib.packages.Package;
@@ -73,6 +72,10 @@
// We'll set the name later, pass the empty string for now.
RuleClass.Builder builder = new RuleClass.Builder("", RuleClassType.WORKSPACE, true);
+ ImmutableList<StarlarkThread.CallStackEntry> callstack = thread.getCallStack();
+ builder.setCallStack(
+ callstack.subList(0, callstack.size() - 1)); // pop 'repository_rule' itself
+
builder.addOrOverrideAttribute(attr("$local", BOOLEAN).defaultValue(local).build());
builder.addOrOverrideAttribute(attr("$configure", BOOLEAN).defaultValue(configure).build());
if (thread.getSemantics().experimentalRepoRemoteExec()) {
@@ -105,7 +108,7 @@
(Label) Module.ofInnermostEnclosingStarlarkFunction(thread).getLabel(),
thread.getTransitiveContentHashCode());
builder.setWorkspaceOnly();
- return new RepositoryRuleFunction(builder, thread.getCallerLocation(), implementation);
+ return new RepositoryRuleFunction(builder, implementation);
}
// RepositoryRuleFunction is the result of repository_rule(...).
@@ -113,17 +116,12 @@
private static final class RepositoryRuleFunction extends BaseFunction
implements SkylarkExportable {
private final RuleClass.Builder builder;
+ private final BaseFunction implementation;
private Label extensionLabel;
private String exportedName;
- private final Location ruleClassDefinitionLocation;
- private final BaseFunction implementation;
- private RepositoryRuleFunction(
- RuleClass.Builder builder,
- Location ruleClassDefinitionLocation,
- BaseFunction implementation) {
+ private RepositoryRuleFunction(RuleClass.Builder builder, BaseFunction implementation) {
this.builder = builder;
- this.ruleClassDefinitionLocation = ruleClassDefinitionLocation;
this.implementation = implementation;
}
@@ -192,35 +190,18 @@
PackageContext context = PackageFactory.getContext(thread);
Package.Builder packageBuilder = context.getBuilder();
- // TODO(adonovan): is this safe? Check.
- String externalRepoName = (String) kwargs.get("name");
-
- StringBuilder callStack =
- new StringBuilder("Call stack for the definition of repository '")
- .append(externalRepoName)
- .append("' which is a ")
- .append(ruleClassName)
- .append(" (rule definition at ")
- .append(ruleClassDefinitionLocation.toString())
- .append("):");
- for (StarlarkThread.CallStackEntry frame : Lists.reverse(thread.getCallStack())) {
- callStack.append("\n - ").append(frame.location);
- }
-
- WorkspaceFactoryHelper.addMainRepoEntry(
- packageBuilder, externalRepoName, thread.getSemantics());
-
- Location loc = thread.getCallerLocation();
- WorkspaceFactoryHelper.addRepoMappings(packageBuilder, kwargs, externalRepoName, loc);
-
+ // TODO(adonovan): is this cast safe? Check.
+ String name = (String) kwargs.get("name");
+ WorkspaceFactoryHelper.addMainRepoEntry(packageBuilder, name, thread.getSemantics());
+ WorkspaceFactoryHelper.addRepoMappings(packageBuilder, kwargs, name);
Rule rule =
WorkspaceFactoryHelper.createAndAddRepositoryRule(
context.getBuilder(),
ruleClass,
- null,
+ /*bindRuleClass=*/ null,
WorkspaceFactoryHelper.getFinalKwargs(kwargs),
- loc,
- callStack.toString());
+ thread.getSemantics(),
+ thread.getCallStack());
return rule;
} catch (InvalidRuleException | NameConflictException | LabelSyntaxException e) {
throw Starlark.errorf("%s", e.getMessage());
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index dc05bdd..88bc70a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -365,27 +365,19 @@
}
BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase(ruleClass.getName());
try {
- addRule(getContext(thread), kwargs, thread);
+ RuleFactory.createAndAddRule(
+ getContext(thread),
+ ruleClass,
+ new BuildLangTypedAttributeValuesMap(kwargs),
+ thread.getSemantics(),
+ thread.getCallStack(),
+ new AttributeContainer(ruleClass));
} catch (RuleFactory.InvalidRuleException | Package.NameConflictException e) {
throw new EvalException(null, e.getMessage());
}
return Starlark.NONE;
}
- private void addRule(PackageContext context, Map<String, Object> kwargs, StarlarkThread thread)
- throws RuleFactory.InvalidRuleException, Package.NameConflictException,
- InterruptedException {
- BuildLangTypedAttributeValuesMap attributeValues =
- new BuildLangTypedAttributeValuesMap(kwargs);
- RuleFactory.createAndAddRule(
- context,
- ruleClass,
- attributeValues,
- thread.getCallerLocation(),
- thread,
- new AttributeContainer(ruleClass));
- }
-
@Override
public RuleClass getRuleClass() {
return ruleClass;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
index 5e2c1b5..4c7f970 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -74,8 +74,6 @@
private boolean containsErrors;
- private String definitionInformation;
-
private final Location location;
private final CallStack callstack;
@@ -125,10 +123,6 @@
this.visibility = visibility;
}
- void setDefinitionInformation(String info) {
- this.definitionInformation = info;
- }
-
void setAttributeValue(Attribute attribute, Object value, boolean explicit) {
attributes.setAttributeValue(attribute, value, explicit);
}
@@ -281,10 +275,6 @@
return callstack;
}
- public String getDefinitionInformation() {
- return definitionInformation;
- }
-
public ImplicitOutputsFunction getImplicitOutputsFunction() {
return implicitOutputsFunction;
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 5dbfe84..666451c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -641,6 +641,7 @@
attr("local", Type.BOOLEAN).build());
private String name;
+ private ImmutableList<StarlarkThread.CallStackEntry> callstack = ImmutableList.of();
private final RuleClassType type;
private final boolean skylark;
private boolean skylarkTestable = false;
@@ -830,6 +831,7 @@
return new RuleClass(
name,
+ callstack,
key,
type,
skylark,
@@ -956,6 +958,12 @@
return this;
}
+ /** Sets the Starlark call stack associated with this rule class's creation. */
+ public Builder setCallStack(ImmutableList<StarlarkThread.CallStackEntry> callstack) {
+ this.callstack = callstack;
+ return this;
+ }
+
public Builder setSkylarkTestable() {
Preconditions.checkState(skylark, "Cannot set skylarkTestable on a non-Starlark rule");
skylarkTestable = true;
@@ -1398,6 +1406,7 @@
}
private final String name; // e.g. "cc_library"
+ private final ImmutableList<StarlarkThread.CallStackEntry> callstack; // of call to 'rule'
private final String key; // Just the name for native, label + name for skylark
@@ -1539,6 +1548,7 @@
@VisibleForTesting
RuleClass(
String name,
+ ImmutableList<StarlarkThread.CallStackEntry> callstack,
String key,
RuleClassType type,
boolean isSkylark,
@@ -1573,6 +1583,7 @@
Collection<Attribute> attributes,
@Nullable BuildSetting buildSetting) {
this.name = name;
+ this.callstack = callstack;
this.key = key;
this.type = type;
this.isSkylark = isSkylark;
@@ -1678,6 +1689,15 @@
return name;
}
+ /**
+ * Returns the stack of Starlark active function calls at the moment this rule class was created.
+ * Entries appear outermost first, and exclude the built-in itself ('rule' or 'repository_rule').
+ * Empty for non-Starlark rules.
+ */
+ public ImmutableList<StarlarkThread.CallStackEntry> getCallStack() {
+ return callstack;
+ }
+
/** Returns the type of rule that this RuleClass represents. Only for use during serialization. */
public RuleClassType getRuleClassType() {
return type;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
index 6043e28..e080fdf 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
@@ -24,6 +24,7 @@
import com.google.devtools.build.lib.packages.Attribute.SkylarkComputedDefaultTemplate.CannotPrecomputeDefaultsException;
import com.google.devtools.build.lib.packages.Package.NameConflictException;
import com.google.devtools.build.lib.packages.PackageFactory.PackageContext;
+import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
import java.util.Map;
import java.util.Set;
@@ -76,8 +77,8 @@
RuleClass ruleClass,
BuildLangTypedAttributeValuesMap attributeValues,
EventHandler eventHandler,
- Location location,
- @Nullable StarlarkThread thread,
+ StarlarkSemantics semantics,
+ ImmutableList<StarlarkThread.CallStackEntry> callstack,
AttributeContainer attributeContainer)
throws InvalidRuleException, InterruptedException {
Preconditions.checkNotNull(ruleClass);
@@ -106,21 +107,16 @@
ruleClass + " cannot be in the WORKSPACE file " + "(used by " + label + ")");
}
- ImmutableList<StarlarkThread.CallStackEntry> callstack =
- thread != null ? thread.getCallStack() : ImmutableList.of();
-
AttributesAndLocation generator =
- generatorAttributesForMacros(pkgBuilder, attributeValues, callstack, location, label);
+ generatorAttributesForMacros(pkgBuilder, attributeValues, callstack, label);
- if (thread != null) {
- // The raw stack is of the form [<toplevel>@BUILD:1, macro@lib.bzl:1, cc_library@<builtin>].
- // If we're recording it (--record_rule_instantiation_callstack),
- // pop the innermost frame for the rule, since it's obvious.
- callstack =
- thread.getSemantics().recordRuleInstantiationCallstack()
- ? callstack.subList(0, callstack.size() - 1) // pop
- : ImmutableList.of(); // save space
- }
+ // The raw stack is of the form [<toplevel>@BUILD:1, macro@lib.bzl:1, cc_library@<builtin>].
+ // If we're recording it (--record_rule_instantiation_callstack),
+ // pop the innermost frame for the rule, since it's obvious.
+ callstack =
+ semantics.recordRuleInstantiationCallstack()
+ ? callstack.subList(0, callstack.size() - 1) // pop
+ : ImmutableList.of(); // save space
try {
// Examines --incompatible_disable_third_party_license_checking to see if we should check
@@ -129,14 +125,13 @@
// This flag is overridable by RuleClass.ThirdPartyLicenseEnforcementPolicy (which is checked
// in RuleClass). This lets Bazel and Blaze migrate away from license logic on independent
// timelines. See --incompatible_disable_third_party_license_checking comments for details.
- boolean checkThirdPartyLicenses =
- thread != null && !thread.getSemantics().incompatibleDisableThirdPartyLicenseChecking();
+ boolean checkThirdPartyLicenses = semantics.incompatibleDisableThirdPartyLicenseChecking();
return ruleClass.createRule(
pkgBuilder,
label,
generator.attributes,
eventHandler,
- generator.location,
+ generator.location, // see b/23974287 for rationale
callstack,
attributeContainer,
checkThirdPartyLicenses);
@@ -157,8 +152,8 @@
* a map entry for each non-optional attribute of this class of rule.
* @param eventHandler a eventHandler on which errors and warnings are reported during rule
* creation
- * @param location the location at which this rule was declared
- * @param thread the lexical environment of the function call which declared this rule (optional)
+ * @param semantics the Starlark semantics
+ * @param callstack the stack of active calls in the Starlark thread
* @param attributeContainer the {@link AttributeContainer} the rule will contain
* @throws InvalidRuleException if the rule could not be constructed for any reason (e.g. no
* {@code name} attribute is defined)
@@ -171,8 +166,8 @@
RuleClass ruleClass,
BuildLangTypedAttributeValuesMap attributeValues,
EventHandler eventHandler,
- Location location,
- @Nullable StarlarkThread thread,
+ StarlarkSemantics semantics,
+ ImmutableList<StarlarkThread.CallStackEntry> callstack,
AttributeContainer attributeContainer)
throws InvalidRuleException, NameConflictException, InterruptedException {
Rule rule =
@@ -181,8 +176,8 @@
ruleClass,
attributeValues,
eventHandler,
- location,
- thread,
+ semantics,
+ callstack,
attributeContainer);
pkgBuilder.addRule(rule);
return rule;
@@ -211,8 +206,8 @@
PackageContext context,
RuleClass ruleClass,
BuildLangTypedAttributeValuesMap attributeValues,
- Location loc,
- @Nullable StarlarkThread thread,
+ StarlarkSemantics semantics,
+ ImmutableList<StarlarkThread.CallStackEntry> callstack,
AttributeContainer attributeContainer)
throws InvalidRuleException, NameConflictException, InterruptedException {
return createAndAddRule(
@@ -220,8 +215,8 @@
ruleClass,
attributeValues,
context.eventHandler,
- loc,
- thread,
+ semantics,
+ callstack,
attributeContainer);
}
@@ -319,8 +314,11 @@
Package.Builder pkgBuilder,
BuildLangTypedAttributeValuesMap args,
ImmutableList<StarlarkThread.CallStackEntry> stack,
- Location location,
Label label) {
+ // For a callstack [BUILD <toplevel>, .bzl <function>, <rule>],
+ // location is that of the caller of 'rule' (the .bzl function).
+ Location location = stack.size() < 2 ? Location.BUILTIN : stack.get(stack.size() - 2).location;
+
boolean hasName = args.containsAttributeNamed("generator_name");
boolean hasFunc = args.containsAttributeNamed("generator_function");
// TODO(bazel-team): resolve cases in our code where hasName && !hasFunc, or hasFunc && !hasName
@@ -336,33 +334,34 @@
if (stack.size() < 2 || !stack.get(1).location.file().endsWith(".bzl")) {
return new AttributesAndLocation(args, location); // macro is not a Starlark function
}
- // TODO(adonovan): is it correct that we clobber the location used by
- // BuildLangTypedAttributeValuesMap?
- location = stack.get(0).location;
+ Location generatorLocation = stack.get(0).location; // location of call to generator
String generatorFunction = stack.get(1).name;
ImmutableMap.Builder<String, Object> builder = ImmutableMap.builder();
for (Map.Entry<String, Object> attributeAccessor : args.getAttributeAccessors()) {
String attributeName = args.getName(attributeAccessor);
builder.put(attributeName, args.getValue(attributeAccessor));
}
- String generatorName = pkgBuilder.getGeneratorNameByLocation().get(location);
+ String generatorName = pkgBuilder.getGeneratorNameByLocation().get(generatorLocation);
if (generatorName == null) {
generatorName = (String) args.getAttributeValue("name");
}
builder.put("generator_name", generatorName);
builder.put("generator_function", generatorFunction);
- String relativePath = maybeGetRelativeLocation(location, label);
+ String relativePath = maybeGetRelativeLocation(generatorLocation, label);
if (relativePath != null) {
builder.put("generator_location", relativePath);
}
try {
- return new AttributesAndLocation(
- new BuildLangTypedAttributeValuesMap(builder.build()), location);
- } catch (IllegalArgumentException ex) {
+ args = new BuildLangTypedAttributeValuesMap(builder.build());
+ } catch (IllegalArgumentException unused) {
// We just fall back to the default case and swallow any messages.
- return new AttributesAndLocation(args, location);
}
+
+ // TODO(adonovan): is it appropriate to use generatorLocation as the rule's main location?
+ // Or would 'location' (the immediate call) be more informative? When there are errors, the
+ // location of the toplevel call of the generator may be quite unrelated to the error message.
+ return new AttributesAndLocation(args, generatorLocation);
}
/**
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 708eb65..132a15b 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
@@ -23,7 +23,6 @@
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.events.NullEventHandler;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Package.NameConflictException;
@@ -289,19 +288,16 @@
if (!allowOverride
&& externalRepoName != null
&& builder.getTarget(externalRepoName) != null) {
- throw new EvalException(
- null,
- "Cannot redefine repository after any load statement in the WORKSPACE file"
- + " (for repository '"
- + kwargs.get("name")
- + "')");
+ throw Starlark.errorf(
+ "Cannot redefine repository after any load statement in the WORKSPACE file (for"
+ + " repository '%s')",
+ externalRepoName);
}
// Add an entry in every repository from @<mainRepoName> to "@" to avoid treating
// @<mainRepoName> as a separate repository. This will be overridden if the main
// repository has a repo_mapping entry from <mainRepoName> to something.
WorkspaceFactoryHelper.addMainRepoEntry(builder, externalRepoName, thread.getSemantics());
- Location loc = thread.getCallerLocation();
- WorkspaceFactoryHelper.addRepoMappings(builder, kwargs, externalRepoName, loc);
+ WorkspaceFactoryHelper.addRepoMappings(builder, kwargs, externalRepoName);
RuleClass ruleClass = ruleFactory.getRuleClass(ruleClassName);
RuleClass bindRuleClass = ruleFactory.getRuleClass("bind");
Rule rule =
@@ -310,7 +306,8 @@
ruleClass,
bindRuleClass,
WorkspaceFactoryHelper.getFinalKwargs(kwargs),
- loc);
+ thread.getSemantics(),
+ thread.getCallStack());
if (!WorkspaceGlobals.isLegalWorkspaceName(rule.getName())) {
throw new EvalException(
null,
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 157bbd3..412ea2d 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
@@ -17,17 +17,18 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.google.common.base.Verify;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
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.RepositoryName;
-import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
-import com.google.devtools.build.lib.syntax.Dict;
import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.SkylarkType;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
+import com.google.devtools.build.lib.syntax.StarlarkThread;
import java.util.Map;
import java.util.stream.Collectors;
@@ -39,22 +40,10 @@
RuleClass ruleClass,
RuleClass bindRuleClass,
Map<String, Object> kwargs,
- Location loc)
+ StarlarkSemantics semantics,
+ ImmutableList<StarlarkThread.CallStackEntry> callstack)
throws RuleFactory.InvalidRuleException, Package.NameConflictException, LabelSyntaxException,
InterruptedException {
- return createAndAddRepositoryRule(pkg, ruleClass, bindRuleClass, kwargs, loc, null);
- }
-
- public static Rule createAndAddRepositoryRule(
- Package.Builder pkg,
- RuleClass ruleClass,
- RuleClass bindRuleClass,
- Map<String, Object> kwargs,
- Location loc,
- String definitionInfo)
- throws RuleFactory.InvalidRuleException, Package.NameConflictException, LabelSyntaxException,
- InterruptedException {
-
StoredEventHandler eventHandler = new StoredEventHandler();
BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs);
Rule rule =
@@ -63,8 +52,8 @@
ruleClass,
attributeValues,
eventHandler,
- loc,
- /* thread= */ null,
+ semantics,
+ callstack,
new AttributeContainer(ruleClass));
pkg.addEvents(eventHandler.getEvents());
pkg.addPosts(eventHandler.getPosts());
@@ -77,10 +66,10 @@
bindRuleClass,
nameLabel,
entry.getValue(),
- rule.getLocation(),
+ semantics,
+ callstack,
new AttributeContainer(bindRuleClass));
}
- rule.setDefinitionInformation(definitionInfo);
return rule;
}
@@ -124,28 +113,18 @@
/**
* Processes {@code repo_mapping} attribute and populates the package builder with the mappings.
*
- * @throws EvalException if {@code repo_mapping} is present in kwargs but is not a {@link Dict}
+ * @throws EvalException if {@code repo_mapping} is present in kwargs but is not a
+ * string-to-string dict.
*/
public static void addRepoMappings(
- Package.Builder builder,
- Map<String, Object> kwargs,
- String externalRepoName,
- Location location)
+ Package.Builder builder, Map<String, Object> kwargs, String externalRepoName)
throws EvalException, LabelSyntaxException {
-
- if (kwargs.containsKey("repo_mapping")) {
- if (!(kwargs.get("repo_mapping") instanceof Dict)) {
- throw new EvalException(
- location,
- "Invalid value for 'repo_mapping': '"
- + kwargs.get("repo_mapping")
- + "'. Value must be a dict.");
- }
- @SuppressWarnings("unchecked")
- Map<String, String> map = (Map<String, String>) kwargs.get("repo_mapping");
+ Object repoMapping = kwargs.get("repo_mapping");
+ if (repoMapping != null) {
+ Map<String, String> map =
+ SkylarkType.castMap(repoMapping, String.class, String.class, "repo_mapping");
for (Map.Entry<String, String> e : map.entrySet()) {
- // Create repository names with validation, LabelSyntaxException is thrown is the name
- // is not valid.
+ // Create repository names with validation; may throw LabelSyntaxException.
builder.addRepositoryMappingEntry(
RepositoryName.create("@" + externalRepoName),
RepositoryName.create(e.getKey()),
@@ -159,7 +138,8 @@
RuleClass bindRuleClass,
Label virtual,
Label actual,
- Location location,
+ StarlarkSemantics semantics,
+ ImmutableList<StarlarkThread.CallStackEntry> callstack,
AttributeContainer attributeContainer)
throws RuleFactory.InvalidRuleException, Package.NameConflictException, InterruptedException {
@@ -175,13 +155,7 @@
new BuildLangTypedAttributeValuesMap(attributes);
Rule rule =
RuleFactory.createRule(
- pkg,
- bindRuleClass,
- attributeValues,
- handler,
- location,
- /* thread= */ null,
- attributeContainer);
+ pkg, bindRuleClass, attributeValues, handler, semantics, callstack, attributeContainer);
overwriteRule(pkg, rule);
rule.setVisibility(ConstantRuleVisibility.PUBLIC);
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
index de1af56..252de68 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
@@ -72,40 +72,42 @@
Dict<?, ?> managedDirectories, // <String, Object>
StarlarkThread thread)
throws EvalException, InterruptedException {
- if (allowOverride) {
- if (!isLegalWorkspaceName(name)) {
- throw Starlark.errorf("%s is not a legal workspace name", name);
- }
- String errorMessage = LabelValidator.validateTargetName(name);
- if (errorMessage != null) {
- throw Starlark.errorf("%s", errorMessage);
- }
- PackageFactory.getContext(thread).pkgBuilder.setWorkspaceName(name);
- Package.Builder builder = PackageFactory.getContext(thread).pkgBuilder;
- RuleClass localRepositoryRuleClass = ruleFactory.getRuleClass("local_repository");
- RuleClass bindRuleClass = ruleFactory.getRuleClass("bind");
- Map<String, Object> kwargs = ImmutableMap.<String, Object>of("name", name, "path", ".");
- try {
- // This effectively adds a "local_repository(name = "<ws>", path = ".")"
- // definition to the WORKSPACE file.
- WorkspaceFactoryHelper.createAndAddRepositoryRule(
- builder, localRepositoryRuleClass, bindRuleClass, kwargs, thread.getCallerLocation());
- } catch (InvalidRuleException | NameConflictException | LabelSyntaxException e) {
- throw Starlark.errorf("%s", e.getMessage());
- }
- // Add entry in repository map from "@name" --> "@" to avoid issue where bazel
- // treats references to @name as a separate external repo
- builder.addRepositoryMappingEntry(
- RepositoryName.MAIN,
- RepositoryName.createFromValidStrippedName(name),
- RepositoryName.MAIN);
- parseManagedDirectories(
- managedDirectories.getContents(String.class, Object.class, "managed_directories"));
- return NONE;
- } else {
+ if (!allowOverride) {
throw Starlark.errorf(
"workspace() function should be used only at the top of the WORKSPACE file");
}
+ if (!isLegalWorkspaceName(name)) {
+ throw Starlark.errorf("%s is not a legal workspace name", name);
+ }
+ String errorMessage = LabelValidator.validateTargetName(name);
+ if (errorMessage != null) {
+ throw Starlark.errorf("%s", errorMessage);
+ }
+ PackageFactory.getContext(thread).pkgBuilder.setWorkspaceName(name);
+ Package.Builder builder = PackageFactory.getContext(thread).pkgBuilder;
+ RuleClass localRepositoryRuleClass = ruleFactory.getRuleClass("local_repository");
+ RuleClass bindRuleClass = ruleFactory.getRuleClass("bind");
+ Map<String, Object> kwargs = ImmutableMap.<String, Object>of("name", name, "path", ".");
+ try {
+ // This effectively adds a "local_repository(name = "<ws>", path = ".")"
+ // definition to the WORKSPACE file.
+ WorkspaceFactoryHelper.createAndAddRepositoryRule(
+ builder,
+ localRepositoryRuleClass,
+ bindRuleClass,
+ kwargs,
+ thread.getSemantics(),
+ thread.getCallStack());
+ } catch (InvalidRuleException | NameConflictException | LabelSyntaxException e) {
+ throw Starlark.errorf("%s", e.getMessage());
+ }
+ // Add entry in repository map from "@name" --> "@" to avoid issue where bazel
+ // treats references to @name as a separate external repo
+ builder.addRepositoryMappingEntry(
+ RepositoryName.MAIN, RepositoryName.createFromValidStrippedName(name), RepositoryName.MAIN);
+ parseManagedDirectories(
+ managedDirectories.getContents(String.class, Object.class, "managed_directories"));
+ return NONE;
}
@Override
@@ -281,11 +283,10 @@
ruleClass,
nameLabel,
actual == NONE ? null : Label.parseAbsolute((String) actual, ImmutableMap.of()),
- thread.getCallerLocation(),
+ thread.getSemantics(),
+ thread.getCallStack(),
new AttributeContainer(ruleClass));
- } catch (RuleFactory.InvalidRuleException
- | Package.NameConflictException
- | LabelSyntaxException e) {
+ } catch (InvalidRuleException | Package.NameConflictException | LabelSyntaxException e) {
throw Starlark.errorf("%s", e.getMessage());
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/BuildOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/BuildOutputFormatter.java
index 36a5bdc..636d30e 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/BuildOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/BuildOutputFormatter.java
@@ -139,14 +139,29 @@
writer.append(")").append(lineTerm);
// Display the instantiation stack, if any.
+ appendStack(
+ String.format("# Rule %s instantiated at (most recent call last):", rule.getName()),
+ rule.getCallStack().toList());
+
+ // Display the stack of the rule class definition, if any.
+ appendStack(
+ String.format("# Rule %s defined at (most recent call last):", rule.getRuleClass()),
+ rule.getRuleClassObject().getCallStack());
+
+ // TODO(adonovan): also list inputs and outputs of the rule.
+
+ writer.append(lineTerm);
+ }
+
+ private void appendStack(String title, List<StarlarkThread.CallStackEntry> stack)
+ throws IOException {
// For readability, ensure columns line up.
int maxLocLen = 0;
- List<StarlarkThread.CallStackEntry> stack = rule.getCallStack().toList().reverse();
for (StarlarkThread.CallStackEntry fr : stack) {
maxLocLen = Math.max(maxLocLen, fr.location.toString().length());
}
if (maxLocLen > 0) {
- writer.append("# Instantiation stack:\n");
+ writer.append(title).append(lineTerm);
for (StarlarkThread.CallStackEntry fr : stack) {
String loc = fr.location.toString(); // TODO(b/151151653): display root-relative
// Java's String.format doesn't support
@@ -155,14 +170,9 @@
for (int i = loc.length(); i < maxLocLen; i++) {
writer.append(' ');
}
- writer.append(" called from ").append(fr.name).append("\n");
+ writer.append(" in ").append(fr.name).append(lineTerm);
}
}
-
- // TODO(adonovan): also record and show creation stack for rule.getRuleClassObject(),
- // and list inputs and outputs of the rule.
-
- writer.append(lineTerm);
}
/** Outputs the given attribute value BUILD-style. Does not support selects. */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/WorkspaceAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/rules/repository/WorkspaceAttributeMapper.java
index 432360a..61c5593 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/WorkspaceAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/WorkspaceAttributeMapper.java
@@ -22,7 +22,6 @@
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.vfs.PathFragment;
import javax.annotation.Nullable;
/**
@@ -62,15 +61,12 @@
public Object getObject(String attributeName) throws EvalException {
Object value = rule.getAttributeContainer().getAttr(checkNotNull(attributeName));
if (value instanceof SelectorList) {
- String message;
- // Is there a basename function for strings?
- String base = PathFragment.create(rule.getLocation().file()).getBaseName();
- if (WorkspaceFileHelper.matchWorkspaceFileName(base)) {
- message = "select() cannot be used in WORKSPACE files";
- } else {
- message = "select() cannot be used in macros called from WORKSPACE files";
- }
- throw new EvalException(rule.getLocation(), message);
+ throw new EvalException(
+ rule.getLocation(),
+ String.format(
+ "got value of type 'select' for attribute '%s' of %s rule '%s'; select may not be "
+ + "used in repository rules",
+ attributeName, rule.getRuleClass(), rule.getName()));
}
return value;
}
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 11c4116..331a0e0 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
@@ -113,6 +113,15 @@
}
}
+ private static final ImmutableList<StarlarkThread.CallStackEntry> DUMMY_STACK =
+ ImmutableList.of(
+ new StarlarkThread.CallStackEntry( //
+ "<toplevel>", Location.fromFileLineColumn("BUILD", 10, 1)),
+ new StarlarkThread.CallStackEntry( //
+ "foo", Location.fromFileLineColumn("foo.bzl", 42, 1)),
+ new StarlarkThread.CallStackEntry( //
+ "myrule", Location.fromFileLineColumn("bar.bzl", 30, 6)));
+
protected void setUpContextForRule(
Map<String, Object> kwargs,
ImmutableSet<PathFragment> ignoredPathFragments,
@@ -129,7 +138,12 @@
ExtendedEventHandler listener = Mockito.mock(ExtendedEventHandler.class);
Rule rule =
WorkspaceFactoryHelper.createAndAddRepositoryRule(
- packageBuilder, buildRuleClass(attributes), null, kwargs, Location.BUILTIN);
+ packageBuilder,
+ buildRuleClass(attributes),
+ null,
+ kwargs,
+ starlarkSemantics,
+ DUMMY_STACK);
DownloadManager downloader = Mockito.mock(DownloadManager.class);
SkyFunction.Environment environment = Mockito.mock(SkyFunction.Environment.class);
when(environment.getListener()).thenReturn(listener);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
index 8952ceb..5a45849 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
@@ -92,6 +92,13 @@
}
};
+ private static final ImmutableList<StarlarkThread.CallStackEntry> DUMMY_STACK =
+ ImmutableList.of(
+ new StarlarkThread.CallStackEntry(
+ "<toplevel>", Location.fromFileLineColumn("BUILD", 10, 1)),
+ new StarlarkThread.CallStackEntry("bar", Location.fromFileLineColumn("bar.bzl", 42, 1)),
+ new StarlarkThread.CallStackEntry("rule", Location.BUILTIN));
+
private static final class DummyFragment extends BuildConfiguration.Fragment {}
private static final ImmutableList<StarlarkThread.CallStackEntry> NO_STACK = ImmutableList.of();
@@ -896,7 +903,8 @@
: ruleDefinitionStarlarkThread.getTransitiveContentHashCode();
return new RuleClass(
name,
- name,
+ DUMMY_STACK,
+ /*key=*/ name,
RuleClassType.NORMAL,
/*isSkylark=*/ skylarkExecutable,
/*skylarkTestable=*/ false,
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
index 76b7455..3a2cf37 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
@@ -30,6 +31,7 @@
import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
+import com.google.devtools.build.lib.syntax.StarlarkThread;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.RootedPath;
@@ -45,7 +47,13 @@
private ConfiguredRuleClassProvider provider = TestRuleClassProvider.getRuleClassProvider();
private final RuleFactory ruleFactory = new RuleFactory(provider);
- private static final Location DUMMY_LOCATION = Location.fromFileLineColumn("dummy", 42, 1);
+ private static final ImmutableList<StarlarkThread.CallStackEntry> DUMMY_STACK =
+ ImmutableList.of(
+ new StarlarkThread.CallStackEntry(
+ "<toplevel>", Location.fromFileLineColumn("BUILD", 42, 1)),
+ new StarlarkThread.CallStackEntry("foo", Location.fromFileLineColumn("foo.bzl", 10, 1)),
+ new StarlarkThread.CallStackEntry(
+ "myrule", Location.fromFileLineColumn("bar.bzl", 30, 6)));
@Test
public void testCreateRule() throws Exception {
@@ -69,8 +77,8 @@
ruleClass,
new BuildLangTypedAttributeValuesMap(attributeValues),
new Reporter(new EventBus()),
- DUMMY_LOCATION,
- /*thread=*/ null,
+ StarlarkSemantics.DEFAULT_SEMANTICS,
+ DUMMY_STACK,
new AttributeContainer(ruleClass));
assertThat(rule.getAssociatedRule()).isSameInstanceAs(rule);
@@ -87,6 +95,11 @@
assertThat(rule.getRuleClass()).isEqualTo("cc_library");
assertThat(rule.getTargetKind()).isEqualTo("cc_library rule");
+ // The rule reports the location of the outermost call (aka generator), in the BUILD file.
+ // Thie behavior was added to fix b/23974287, but it loses informtion and is redundant
+ // w.r.t. generator_location. A better fix to that issue would be to keep rule.location as
+ // the innermost call, and to report the entire call stack at the first error for the rule.
+ assertThat(rule.getLocation().file()).isEqualTo("BUILD");
assertThat(rule.getLocation().line()).isEqualTo(42);
assertThat(rule.getLocation().column()).isEqualTo(1);
assertThat(rule.containsErrors()).isFalse();
@@ -124,8 +137,8 @@
ruleClass,
new BuildLangTypedAttributeValuesMap(attributeValues),
new Reporter(new EventBus()),
- Location.fromFile(myPkgPath.toString()),
- /*thread=*/ null,
+ StarlarkSemantics.DEFAULT_SEMANTICS,
+ DUMMY_STACK,
new AttributeContainer(ruleClass));
assertThat(rule.containsErrors()).isFalse();
}
@@ -155,8 +168,8 @@
ruleClass,
new BuildLangTypedAttributeValuesMap(attributeValues),
new Reporter(new EventBus()),
- DUMMY_LOCATION,
- /*thread=*/ null,
+ StarlarkSemantics.DEFAULT_SEMANTICS,
+ DUMMY_STACK,
new AttributeContainer(ruleClass)));
assertThat(e).hasMessageThat().contains("must be in the WORKSPACE file");
}
@@ -186,8 +199,8 @@
ruleClass,
new BuildLangTypedAttributeValuesMap(attributeValues),
new Reporter(new EventBus()),
- Location.fromFileLineColumn(myPkgPath.toString(), 42, 1),
- /*thread=*/ null,
+ StarlarkSemantics.DEFAULT_SEMANTICS,
+ DUMMY_STACK,
new AttributeContainer(ruleClass)));
assertThat(e).hasMessageThat().contains("cannot be in the WORKSPACE file");
}
@@ -229,8 +242,8 @@
ruleClass,
new BuildLangTypedAttributeValuesMap(attributeValues),
new Reporter(new EventBus()),
- Location.fromFileLineColumn(myPkgPath.toString(), 42, 1),
- /*thread=*/ null,
+ StarlarkSemantics.DEFAULT_SEMANTICS,
+ DUMMY_STACK,
new AttributeContainer(ruleClass)));
assertWithMessage(e.getMessage())
.that(e.getMessage().contains("output file name can't be equal '.'"))
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 7cdd3c8..7aaa3e8 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
@@ -173,24 +173,18 @@
}
@Test
- public void testMappingsNotAMap() throws Exception {
- helper.parse(
- "local_repository(",
- " name = 'foo',",
- " path = '/foo',",
- " repo_mapping = 1",
- ")");
+ public void testRepoMappingNotAStringStringDict() throws Exception {
+ helper.parse("local_repository(name='foo', path='/foo', repo_mapping=1)");
assertThat(helper.getParserError())
- .contains("Invalid value for 'repo_mapping': '1'. Value must be a dict.");
+ .contains("expected a dictionary for 'repo_mapping' but got 'int' instead");
- helper.parse(
- "local_repository(",
- " name = 'foo',",
- " path = '/foo',",
- " repo_mapping = 'hello'",
- ")");
+ helper.parse("local_repository(name='foo', path='/foo', repo_mapping='hello')");
assertThat(helper.getParserError())
- .contains("Invalid value for 'repo_mapping': 'hello'. Value must be a dict.");
+ .contains("expected a dictionary for 'repo_mapping' but got 'string' instead");
+
+ helper.parse("local_repository(name='foo', path='/foo', repo_mapping={1: 1})");
+ assertThat(helper.getParserError())
+ .contains("expected <String, String> type for 'repo_mapping' but got <int, int> instead");
}
@Test
diff --git a/src/test/shell/bazel/external_integration_test.sh b/src/test/shell/bazel/external_integration_test.sh
index 1a9d083..43532a1 100755
--- a/src/test/shell/bazel/external_integration_test.sh
+++ b/src/test/shell/bazel/external_integration_test.sh
@@ -2238,12 +2238,17 @@
)
EOF
- bazel build @foo//... > "${TEST_log}" 2>&1 && fail "expected failure"
+ bazel build --record_rule_instantiation_callstack @foo//... > "${TEST_log}" 2>&1 && fail "expected failure"
+ inplace-sed -e "s?$WRKDIR/?WRKDIR/?g" -e "s?$TEST_TMPDIR/?TEST_TMPDIR/?g" "${TEST_log}"
+
expect_log 'error.*repository.*foo'
expect_log '@bar//:foo.build'
- expect_log 'path/to/main/foo.bzl'
- expect_log 'path/to/main/repos.bzl'
- expect_log 'path/to/main/WORKSPACE'
+ expect_log "Repository foo instantiated at:"
+ expect_log " WRKDIR/path/to/main/WORKSPACE:"
+ expect_log " WRKDIR/path/to/main/repos.bzl:5"
+ expect_log " WRKDIR/path/to/main/foo.bzl:4"
+ expect_log "Repository rule http_archive defined at:"
+ expect_log " TEST_TMPDIR/.*/external/bazel_tools/tools/build_defs/repo/http.bzl:"
}
function test_circular_definition_reported() {
@@ -2309,7 +2314,8 @@
EOF
touch BUILD
- bazel build //... > "${TEST_log}" 2>&1 && fail "expected failure" || :
+ bazel build --record_rule_instantiation_callstack //... > "${TEST_log}" 2>&1 && fail "expected failure" || :
+ inplace-sed -e 's?$(pwd)/?PWD/?g' "${TEST_log}"
expect_not_log '[iI]nternal [eE]rror'
expect_not_log 'IllegalStateException'
@@ -2360,13 +2366,17 @@
load("@data//:value.bzl", "value")
EOF
- bazel build //... > "${TEST_log}" 2>&1 && fail "expected failure" || :
+ # TODO(adonovan): add a test that the error message contains a hint to set the flag if unset.
+ bazel build --record_rule_instantiation_callstack //... > "${TEST_log}" 2>&1 && fail "expected failure" || :
+ inplace-sed -e 's?$(pwd)/?PWD/?g' "${TEST_log}"
- expect_log "add.*this_repo_is_missing.*WORKSPACE"
+ expect_log "you have to add.*this_repo_is_missing.*WORKSPACE"
# Also verify that the repository class and its definition is reported, to
# help finding out where the implict dependency comes from.
- expect_log 'data.*is.*data_repo'
- expect_log 'data_repo.*main/withimplicit.bzl:6'
+ expect_log "Repository data instantiated at:"
+ expect_log ".../WORKSPACE:47"
+ expect_log "Repository rule data_repo defined at:"
+ expect_log ".../withimplicit.bzl:6"
}
function test_overwrite_existing_workspace_build() {
diff --git a/src/test/shell/bazel/workspace_resolved_test.sh b/src/test/shell/bazel/workspace_resolved_test.sh
index 0ea10a4..12bf50b 100755
--- a/src/test/shell/bazel/workspace_resolved_test.sh
+++ b/src/test/shell/bazel/workspace_resolved_test.sh
@@ -52,14 +52,18 @@
EOF
bazel clean --expunge
- bazel build --experimental_repository_resolved_file=../repo.bzl @ext//... \
+ bazel build --experimental_repository_resolved_file=../repo.bzl --record_rule_instantiation_callstack @ext//... \
> "${TEST_log}" 2>&1 || fail "Expected success"
+ inplace-sed -e "s?$(pwd)?PWD?g" "$TEST_log"
bazel shutdown
# We expect the additional argument to be reported to the user...
expect_log 'extra_arg.*foobar'
- # ...as well as the location of the definition.
- expect_log 'fetchrepo/WORKSPACE:2'
+ # ...as well as the location of the rule instantiation and definition.
+ expect_log 'Repository ext instantiated at:'
+ expect_log ' PWD/WORKSPACE:2:'
+ expect_log 'Repository rule trivial_rule defined at:'
+ expect_log ' PWD/rule.bzl:5:'
# Verify that bazel can read the generated repo.bzl file and that it contains
# the expected information
@@ -1177,6 +1181,7 @@
EOF
bazel sync --distdir=${EXTREPODIR}/test_WORKSPACE/distdir \
+ --record_rule_instantiation_callstack \
--experimental_repository_resolved_file=resolved.bzl
echo; cat resolved.bzl; echo
@@ -1199,13 +1204,16 @@
return repo["definition_information"]
EOF
- bazel build //:ext_def
+ bazel build --record_rule_instantiation_callstack //:ext_def
cat `bazel info bazel-genfiles`/ext_def.txt > "${TEST_log}"
-
- expect_log "WORKSPACE:3"
- expect_log "first/path/foo.bzl:4"
- expect_log "another/directory/bar.bzl:4"
+ inplace-sed -e "s?$(pwd)/?PWD/?g" -e "s?$TEST_TMPDIR/?TEST_TMPDIR/?g" "${TEST_log}"
+ expect_log "Repository ext instantiated at:"
+ expect_log " PWD/WORKSPACE:3"
+ expect_log " PWD/first/path/foo.bzl:4"
+ expect_log " PWD/another/directory/bar.bzl:4"
+ expect_log "Repository rule http_archive defined at:"
+ expect_log " TEST_TMPDIR/.*/external/bazel_tools/tools/build_defs/repo/http.bzl:"
}
run_suite "workspace_resolved_test tests"
diff --git a/src/test/shell/bazel/workspace_test.sh b/src/test/shell/bazel/workspace_test.sh
index 3741027..0c33210 100755
--- a/src/test/shell/bazel/workspace_test.sh
+++ b/src/test/shell/bazel/workspace_test.sh
@@ -132,7 +132,7 @@
)
EOF
bazel build @foo//... &> $TEST_log && fail "Failure expected" || true
- expect_log "select() cannot be used in WORKSPACE files"
+ expect_log "got value of type 'select' for attribute 'build_file' of new_local_repository rule 'foo'; select may not be used in repository rules"
}
function test_macro_select() {
@@ -153,7 +153,7 @@
EOF
bazel build @foo//... &> $TEST_log && fail "Failure expected" || true
- expect_log "select() cannot be used in macros called from WORKSPACE files"
+ expect_log "got value of type 'select' for attribute 'build_file' of new_local_repository rule 'foo'"
}
function test_clean() {
diff --git a/src/test/shell/integration_test_setup.sh b/src/test/shell/integration_test_setup.sh
index 1b68adc..bbf862e 100755
--- a/src/test/shell/integration_test_setup.sh
+++ b/src/test/shell/integration_test_setup.sh
@@ -42,3 +42,14 @@
# Load the test environment
source "$DIR/testenv.sh" || print_message_and_exit "testenv.sh not found!"
fi
+
+# inplace-sed: a version of sed -i that actually works on Linux and Darwin.
+# https://unix.stackexchange.com/questions/92895
+# https://stackoverflow.com/questions/5694228
+function inplace-sed() {
+ if [ $(uname) = "Darwin" ]; then
+ sed -i "" "$@"
+ else
+ sed -i "$@"
+ fi
+}
