Skip to content

Commit

Permalink
Allow repository rules to provide default bindings for external labels.
Browse files Browse the repository at this point in the history 
The idea is that an android_sdk_repository rule would by default bind @external:android/sdk to itself, thus avoiding an unnecessary roundtrip through //tools/android:sdk . If we then also eventually bind that external label to something, we can avoid having the //tools/android:sdk rule altogether.

--
MOS_MIGRATED_REVID=96285812
  • Loading branch information
@lberki @damienmg
lberki authored and damienmg committed Jun 18, 2015
1 parent 005ed82 commit 0e1a994
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 22 deletions.
19 changes: 19 additions & 0 deletions ...main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,44 @@
import static com.google.devtools.build.lib.packages.Type.INTEGER;
import static com.google.devtools.build.lib.packages.Type.STRING;

import com.google.common.base.Function;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.bazel.rules.workspace.WorkspaceBaseRule;
import com.google.devtools.build.lib.bazel.rules.workspace.WorkspaceConfiguredTargetFactory;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleClass.Builder;
import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
import com.google.devtools.build.lib.syntax.Label;

import java.util.Map;

import javax.annotation.Nullable;

/**
* Definition of the {@code android_sdk} repository rule.
*/
public class AndroidSdkRepositoryRule implements RuleDefinition {
public static final String NAME = "android_sdk_repository";

public static final Function<? super Rule, Map<String, Label>> BINDINGS_FUNCTION =
new Function< Rule, Map<String, Label>>() {
@Nullable
@Override
public Map<String, Label> apply(Rule rule) {
return ImmutableMap.of(
"android/sdk", Label.parseAbsoluteUnchecked("@" + rule.getName() + "//:sdk"));
}
};

@Override
public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
return builder
.setUndocumented()
.setWorkspaceOnly()
.setExternalBindingsFunction(BINDINGS_FUNCTION)
.add(attr("path", STRING).mandatory().nonconfigurable("WORKSPACE rule"))
.add(attr("build_tools_version", STRING).mandatory().nonconfigurable("WORKSPACE rule"))
.add(attr("api_level", INTEGER).mandatory().nonconfigurable("WORKSPACE rule"))
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/google/devtools/build/lib/packages/ExternalPackage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,11 @@ public Builder createAndAddRepositoryRule(RuleClass ruleClass, Map<String, Objec
ast.getLocation());
addEvents(eventHandler.getEvents());
repositoryMap.put(RepositoryName.create("@" + tempRule.getName()), tempRule);
for (Map.Entry<String, Label> entry :
ruleClass.getExternalBindingsFunction().apply(tempRule).entrySet()) {
Label nameLabel = Label.parseAbsolute("//external:" + entry.getKey());
addBinding(nameLabel, new Binding(entry.getValue(), tempRule.getLocation()));
}
return this;
}

Expand Down
30 changes: 30 additions & 0 deletions src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,13 @@
import static com.google.devtools.build.lib.packages.Type.LABEL_LIST;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
import com.google.common.base.Functions;
import com.google.common.base.Preconditions;
import com.google.common.base.Predicate;
import com.google.common.base.Predicates;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Ordering;
import com.google.devtools.build.lib.events.EventHandler;
Expand Down Expand Up @@ -91,6 +94,8 @@
*/
@Immutable
public final class RuleClass {
public static final Function<? super Rule, Map<String, Label>> NO_EXTERNAL_BINDINGS =
Functions.<Map<String, Label>>constant(ImmutableMap.<String, Label>of());
/**
* A constraint for the package name of the Rule instances.
*/
Expand Down Expand Up @@ -428,6 +433,8 @@ public String toString() {
private Predicate<String> preferredDependencyPredicate = Predicates.alwaysFalse();
private List<Class<?>> advertisedProviders = new ArrayList<>();
private BaseFunction configuredTargetFunction = null;
private Function<? super Rule, Map<String, Label>> externalBindingsFunction =
NO_EXTERNAL_BINDINGS;
private SkylarkEnvironment ruleDefinitionEnvironment = null;
private Set<Class<?>> configurationFragments = new LinkedHashSet<>();
private boolean failIfMissingConfigurationFragment;
Expand Down Expand Up @@ -510,10 +517,13 @@ public RuleClass build(String name) {
== (configuredTargetFactory == null && configuredTargetFunction == null));
Preconditions.checkState(skylarkExecutable == (configuredTargetFunction != null));
Preconditions.checkState(skylarkExecutable == (ruleDefinitionEnvironment != null));
Preconditions.checkState(workspaceOnly || externalBindingsFunction == NO_EXTERNAL_BINDINGS);

return new RuleClass(name, skylarkExecutable, documented, publicByDefault, binaryOutput,
workspaceOnly, outputsDefaultExecutable, implicitOutputsFunction, configurator,
configuredTargetFactory, validityPredicate, preferredDependencyPredicate,
ImmutableSet.copyOf(advertisedProviders), configuredTargetFunction,
externalBindingsFunction,
ruleDefinitionEnvironment, configurationFragments, failIfMissingConfigurationFragment,
supportsConstraintChecking, attributes.values().toArray(new Attribute[0]));
}
Expand Down Expand Up @@ -688,6 +698,11 @@ public Builder setConfiguredTargetFunction(BaseFunction func) {
return this;
}

public Builder setExternalBindingsFunction(Function<? super Rule, Map<String, Label>> func) {
this.externalBindingsFunction = func;
return this;
}

/**
* Sets the rule definition environment. Meant for Skylark usage.
*/
Expand Down Expand Up @@ -840,6 +855,11 @@ public Attribute.Builder<?> copy(String name) {
*/
@Nullable private final BaseFunction configuredTargetFunction;

/**
* Returns the extra bindings a workspace function adds to the WORKSPACE file.
*/
private final Function<? super Rule, Map<String, Label>> externalBindingsFunction;

/**
* The Skylark rule definition environment of this RuleClass.
* Null for non Skylark executable RuleClasses.
Expand Down Expand Up @@ -900,6 +920,7 @@ public Attribute.Builder<?> copy(String name) {
PredicateWithMessage<Rule> validityPredicate, Predicate<String> preferredDependencyPredicate,
ImmutableSet<Class<?>> advertisedProviders,
@Nullable BaseFunction configuredTargetFunction,
Function<? super Rule, Map<String, Label>> externalBindingsFunction,
@Nullable SkylarkEnvironment ruleDefinitionEnvironment,
Set<Class<?>> allowedConfigurationFragments, boolean failIfMissingConfigurationFragment,
boolean supportsConstraintChecking,
Expand All @@ -917,6 +938,7 @@ public Attribute.Builder<?> copy(String name) {
this.preferredDependencyPredicate = preferredDependencyPredicate;
this.advertisedProviders = advertisedProviders;
this.configuredTargetFunction = configuredTargetFunction;
this.externalBindingsFunction = externalBindingsFunction;
this.ruleDefinitionEnvironment = ruleDefinitionEnvironment;
// Do not make a defensive copy as builder does that already
this.attributes = attributes;
Expand Down Expand Up @@ -1511,6 +1533,14 @@ public boolean hasBinaryOutput() {
return configuredTargetFunction;
}

/**
* Returns a function that computes the external bindings a repository function contributes to
* the WORKSPACE file.
*/
public Function<? super Rule, Map<String, Label>> getExternalBindingsFunction() {
return externalBindingsFunction;
}

/**
* Returns this RuleClass's rule definition environment.
*/
Expand Down
48 changes: 26 additions & 22 deletions src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.ImplicitOutputsFunction.substitutePlaceholderIntoTemplate;
import static com.google.devtools.build.lib.packages.RuleClass.NO_EXTERNAL_BINDINGS;
import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
import static com.google.devtools.build.lib.packages.Type.INTEGER;
import static com.google.devtools.build.lib.packages.Type.LABEL;
Expand Down Expand Up @@ -87,8 +88,8 @@ private static RuleClass createRuleClassA() throws Label.SyntaxException {
return new RuleClass("ruleA", false, false, false, false, false, false,
ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE,
DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(),
PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, null,
ImmutableSet.<Class<?>>of(), false, true,
PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null,
NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(), false, true,
attr("my-string-attr", STRING).mandatory().build(),
attr("my-label-attr", LABEL).mandatory().legacyAllowAnyFileType()
.value(Label.parseAbsolute("//default:label")).build(),
Expand All @@ -106,8 +107,8 @@ private static RuleClass createRuleClassB(RuleClass ruleClassA) {
return new RuleClass("ruleB", false, false, false, false, false, false,
ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY,
PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE,
ImmutableSet.<Class<?>>of(), null, null, ImmutableSet.<Class<?>>of(), false, true,
attributes.toArray(new Attribute[0]));
ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(),
false, true, attributes.toArray(new Attribute[0]));
}

public void testRuleClassBasics() throws Exception {
Expand Down Expand Up @@ -218,7 +219,8 @@ public void testDuplicatedDeps() throws Exception {
RuleClass depsRuleClass = new RuleClass("ruleDeps", false, false, false, false, false, false,
ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY,
PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE,
ImmutableSet.<Class<?>>of(), null, null, ImmutableSet.<Class<?>>of(), false, true,
ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(),
false, true,
attr("list1", LABEL_LIST).mandatory().legacyAllowAnyFileType().build(),
attr("list2", LABEL_LIST).mandatory().legacyAllowAnyFileType().build(),
attr("list3", LABEL_LIST).mandatory().legacyAllowAnyFileType().build());
Expand Down Expand Up @@ -247,8 +249,8 @@ public void testCreateRuleWithLegacyPublicVisibility() throws Exception {
RuleClass ruleClass = new RuleClass("ruleVis", false, false, false, false, false, false,
ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY,
PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE,
ImmutableSet.<Class<?>>of(), null, null, ImmutableSet.<Class<?>>of(), false, true,
attr("visibility", LABEL_LIST).legacyAllowAnyFileType().build());
ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(),
false, true, attr("visibility", LABEL_LIST).legacyAllowAnyFileType().build());
Map<String, Object> attributeValues = new HashMap<>();
attributeValues.put("visibility", Arrays.asList("//visibility:legacy_public"));

Expand Down Expand Up @@ -332,8 +334,8 @@ public void testImplicitOutputs() throws Exception {
"stuff-%{outs}-bar"),
RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY,
PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE,
ImmutableSet.<Class<?>>of(), null, null, ImmutableSet.<Class<?>>of(), false, true,
attr("outs", OUTPUT_LIST).build());
ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(),
false, true, attr("outs", OUTPUT_LIST).build());

Map<String, Object> attributeValues = new HashMap<>();
attributeValues.put("outs", Collections.singletonList("explicit_out"));
Expand All @@ -353,8 +355,8 @@ public void testImplicitOutsWithBasenameDirname() throws Exception {
RuleClass ruleClass = new RuleClass("ruleClass", false, false, false, false, false, false,
ImplicitOutputsFunction.fromTemplates("%{dirname}lib%{basename}.bar"), RuleClass.NO_CHANGE,
DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(),
PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, null,
ImmutableSet.<Class<?>>of(), false, true);
PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS,
null, ImmutableSet.<Class<?>>of(), false, true);

Rule rule = createRule(ruleClass, "myRule", Collections.<String, Object>emptyMap(),
testRuleLocation);
Expand All @@ -374,8 +376,8 @@ private static RuleClass getRuleClassWithComputedDefault(Attribute computedDefau
return new RuleClass("ruleClass", false, false, false, false, false, false,
ImplicitOutputsFunction.fromTemplates("empty"), RuleClass.NO_CHANGE,
DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(),
PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, null,
ImmutableSet.<Class<?>>of(), false, true,
PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS,
null, ImmutableSet.<Class<?>>of(), false, true,
attr("condition", BOOLEAN).value(false).build(),
attr("declared1", BOOLEAN).value(false).build(),
attr("declared2", BOOLEAN).value(false).build(),
Expand Down Expand Up @@ -517,8 +519,8 @@ public void testOutputsAreOrdered() throws Exception {
ImplicitOutputsFunction.fromTemplates("first-%{name}", "second-%{name}", "out-%{outs}"),
RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY,
PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE,
ImmutableSet.<Class<?>>of(), null, null, ImmutableSet.<Class<?>>of(), false, true,
attr("outs", OUTPUT_LIST).build());
ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(),
false, true, attr("outs", OUTPUT_LIST).build());

Map<String, Object> attributeValues = new HashMap<>();
attributeValues.put("outs", ImmutableList.of("third", "fourth"));
Expand All @@ -540,7 +542,8 @@ public void testSubstitutePlaceholderIntoTemplate() throws Exception {
RuleClass ruleClass = new RuleClass("ruleA", false, false, false, false, false, false,
ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY,
PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE,
ImmutableSet.<Class<?>>of(), null, null, ImmutableSet.<Class<?>>of(), false, true,
ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(),
false, true,
attr("a", STRING_LIST).mandatory().build(),
attr("b", STRING_LIST).mandatory().build(),
attr("c", STRING_LIST).mandatory().build(),
Expand Down Expand Up @@ -625,8 +628,8 @@ private RuleClass setupNonEmpty(Attribute... attributes) {
"ruleMNE", false, false, false, false, false, false,
ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY,
PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE,
ImmutableSet.<Class<?>>of(), null, null, ImmutableSet.<Class<?>>of(), false, true,
attributes);
ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(),
false, true, attributes);
return mandNonEmptyRuleClass;
}

Expand All @@ -636,8 +639,9 @@ public void testNonEmptyWrongDefVal() throws Exception {
"ruleMNE", false, false, false, false, false, false,
ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY,
PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE,
ImmutableSet.<Class<?>>of(), null, null, ImmutableSet.<Class<?>>of(), false, true,
attr("list", LABEL_LIST).nonEmpty().legacyAllowAnyFileType().value(emptyList).build());
ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(),
false, true, attr("list", LABEL_LIST)
.nonEmpty().legacyAllowAnyFileType().value(emptyList).build());

Map<String, Object> attributeValues = new LinkedHashMap<>();
reporter.removeHandler(failFastHandler);
Expand Down Expand Up @@ -721,8 +725,8 @@ private RuleClass createParentRuleClass() {
RuleClass parentRuleClass = new RuleClass("parent_rule", false, false, false, false, false,
false, ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY,
PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE,
ImmutableSet.<Class<?>>of(), null, null, ImmutableSet.<Class<?>>of(DummyFragment.class),
false, true, attr("attr", STRING).build());
ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null,
ImmutableSet.<Class<?>>of(DummyFragment.class), false, true, attr("attr", STRING).build());
return parentRuleClass;
}

Expand Down

0 comments on commit 0e1a994

Please sign in to comment.