Skip to content

Commit

Permalink
Add alternative interface for LIPO data transitions, opt in native ru…
Browse files Browse the repository at this point in the history
…les.

This removes the need for ConfigurationTransitionProxy.DATA by providing a
way for the C++ rule defs to directly inject the transition for all rules
to use.

Skylark attributes work differently, so they'll be addressed in another
change.

PiperOrigin-RevId: 183721293
  • Loading branch information
gregestren authored and Copybara-Service committed Jan 29, 2018
1 parent 29f2ba8 commit e046d3a
Show file tree
Hide file tree
Showing 13 changed files with 92 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

package com.google.devtools.build.lib.analysis;

import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransitionProxy.DATA;
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.DISTRIBUTIONS;
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
Expand Down Expand Up @@ -154,7 +153,11 @@ public Object getDefault(AttributeMap rule) {

// The target itself and run_under both run on the same machine. We use the DATA config
// here because the run_under acts like a data dependency (e.g. no LIPO optimization).
.add(attr(":run_under", LABEL).cfg(DATA).value(RUN_UNDER).skipPrereqValidatorCheck())
.add(
attr(":run_under", LABEL)
.cfg(env.getLipoDataTransition())
.value(RUN_UNDER)
.skipPrereqValidatorCheck())
.build();
}

Expand Down Expand Up @@ -328,7 +331,7 @@ public static final class RuleBase implements RuleDefinition {
public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
return builder
.add(attr("deps", LABEL_LIST).legacyAllowAnyFileType())
.add(attr("data", LABEL_LIST).cfg(DATA)
.add(attr("data", LABEL_LIST).cfg(env.getLipoDataTransition())
.allowedFileTypes(FileTypeSet.ANY_FILE)
.dontCheckConstraints())
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1201,6 +1201,7 @@ public RuleContext getRuleContextForTesting(
ImmutableList.of(),
targetConfig,
configurations.getHostConfiguration(),
ruleClassProvider.getLipoDataTransition(),
ruleClassProvider.getPrerequisiteValidator(),
((Rule) target).getRuleClassObject().getConfigurationFragmentPolicy())
.setVisibility(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.devtools.build.lib.analysis.config.DefaultsPackage;
import com.google.devtools.build.lib.analysis.config.DynamicTransitionMapper;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
import com.google.devtools.build.lib.analysis.config.transitions.Transition;
import com.google.devtools.build.lib.analysis.skylark.SkylarkModules;
import com.google.devtools.build.lib.cmdline.Label;
Expand Down Expand Up @@ -229,6 +230,7 @@ public static class Builder implements RuleDefinitionEnvironment {
new Digraph<>();
private ImmutableMap.Builder<Transition, Transition> dynamicTransitionMaps
= ImmutableMap.builder();
private PatchTransition lipoDataTransition;
private Class<? extends BuildConfiguration.Fragment> universalFragment;
private PrerequisiteValidator prerequisiteValidator;
private ImmutableMap.Builder<String, Object> skylarkAccessibleTopLevels =
Expand Down Expand Up @@ -391,6 +393,27 @@ public Builder addPlatformRegexps(Map<String, String> platformRegexps) {
return this;
}

/**
* Sets the C++ LIPO data transition, as defined in {@link
* com.google.devtools.build.lib.rules.cpp.transitions.DisableLipoTransition}.
*
* <p>This is language-specific, so doesn't really belong here. But since non-C++ rules declare
* this transition, we need universal access to it. The need for this interface should go away
* on the deprecation of LIPO for
* <a href="https://clang.llvm.org/docs/ThinLTO.html">ThinLTO</a>.
*/
public Builder setLipoDataTransition(PatchTransition transition) {
Preconditions.checkState(lipoDataTransition == null, "LIPO data transition already set");
lipoDataTransition = Preconditions.checkNotNull(transition);
return this;
}

@Override
public PatchTransition getLipoDataTransition() {
Preconditions.checkState(lipoDataTransition != null);
return lipoDataTransition;
}

private RuleConfiguredTargetFactory createFactory(
Class<? extends RuleConfiguredTargetFactory> factoryClass) {
try {
Expand Down Expand Up @@ -465,6 +488,7 @@ public ConfiguredRuleClassProvider build() {
ImmutableList.copyOf(configurationOptions),
ImmutableList.copyOf(configurationFragmentFactories),
new DynamicTransitionMapper(dynamicTransitionMaps.build()),
lipoDataTransition,
universalFragment,
prerequisiteValidator,
skylarkAccessibleTopLevels.build(),
Expand Down Expand Up @@ -571,6 +595,8 @@ public Label load(String from) {
*/
private final DynamicTransitionMapper dynamicTransitionMapper;

private final PatchTransition lipoDataTransition;

/**
* A configuration fragment that should be available to all rules even when they don't
* explicitly require it.
Expand Down Expand Up @@ -600,6 +626,7 @@ private ConfiguredRuleClassProvider(
ImmutableList<Class<? extends FragmentOptions>> configurationOptions,
ImmutableList<ConfigurationFragmentFactory> configurationFragments,
DynamicTransitionMapper dynamicTransitionMapper,
PatchTransition lipoDataTransition,
Class<? extends BuildConfiguration.Fragment> universalFragment,
PrerequisiteValidator prerequisiteValidator,
ImmutableMap<String, Object> skylarkAccessibleJavaClasses,
Expand All @@ -617,6 +644,7 @@ private ConfiguredRuleClassProvider(
this.configurationOptions = configurationOptions;
this.configurationFragmentFactories = configurationFragments;
this.dynamicTransitionMapper = dynamicTransitionMapper;
this.lipoDataTransition = lipoDataTransition;
this.universalFragment = universalFragment;
this.prerequisiteValidator = prerequisiteValidator;
this.globals = createGlobals(skylarkAccessibleJavaClasses, skylarkModules);
Expand Down Expand Up @@ -672,6 +700,18 @@ public ImmutableList<ConfigurationFragmentFactory> getConfigurationFragments() {
return configurationFragmentFactories;
}

/**
* Returns the C++ LIPO data transition, as defined in {@link
* com.google.devtools.build.lib.rules.cpp.transitions.DisableLipoTransition}.
*
* <p>This is language-specific, so doesn't really belong here. But since non-C++ rules declare
* this transition, we need universal access to it. The need for this interface should go away on
* the deprecation of LIPO for <a href="https://clang.llvm.org/docs/ThinLTO.html">ThinLTO</a>.
*/
public PatchTransition getLipoDataTransition() {
return lipoDataTransition;
}

/**
* Returns the set of configuration options that are supported in this module.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ private ConfiguredTarget createRule(
ImmutableList.of(),
configuration,
hostConfiguration,
ruleClassProvider.getLipoDataTransition(),
ruleClassProvider.getPrerequisiteValidator(),
rule.getRuleClassObject().getConfigurationFragmentPolicy())
.setVisibility(convertVisibility(prerequisiteMap, env.getEventHandler(), rule, null))
Expand Down Expand Up @@ -432,6 +433,7 @@ public ConfiguredAspect createAspect(
aspectPath,
aspectConfiguration,
hostConfiguration,
ruleClassProvider.getLipoDataTransition(),
ruleClassProvider.getPrerequisiteValidator(),
aspect.getDefinition().getConfigurationFragmentPolicy());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ public ImmutableList<TransitiveInfoCollection> getFiles() {
private final ImmutableSet<String> features;
private final String ruleClassNameForLogging;
private final BuildConfiguration hostConfiguration;
private final PatchTransition disableLipoTransition;
private final ConfigurationFragmentPolicy configurationFragmentPolicy;
private final Class<? extends BuildConfiguration.Fragment> universalFragment;
private final ErrorReporter reporter;
Expand Down Expand Up @@ -218,6 +219,7 @@ private RuleContext(
this.features = getEnabledFeatures();
this.ruleClassNameForLogging = ruleClassNameForLogging;
this.hostConfiguration = builder.hostConfiguration;
this.disableLipoTransition = builder.disableLipoTransition;
reporter = builder.reporter;
this.toolchainContext = toolchainContext;
}
Expand Down Expand Up @@ -1079,8 +1081,7 @@ private void checkAttribute(String attributeName, Mode mode) {
+ " is not configured for the target configuration");
}
} else if (mode == Mode.DATA) {
if (!(transition instanceof PatchTransition)
&& transition != ConfigurationTransitionProxy.DATA) {
if (transition != disableLipoTransition && transition != ConfigurationTransitionProxy.DATA) {
throw new IllegalStateException(getRule().getLocation() + ": "
+ getRuleClassNameForLogging() + " attribute " + attributeName
+ " is not configured for the data configuration");
Expand Down Expand Up @@ -1367,6 +1368,7 @@ public static final class Builder implements RuleErrorConsumer {
private Class<? extends BuildConfiguration.Fragment> universalFragment;
private final BuildConfiguration configuration;
private final BuildConfiguration hostConfiguration;
private PatchTransition disableLipoTransition;
private final PrerequisiteValidator prerequisiteValidator;
private final ErrorReporter reporter;
private OrderedSetMultimap<Attribute, ConfiguredTargetAndTarget> prerequisiteMap;
Expand All @@ -1382,6 +1384,7 @@ public static final class Builder implements RuleErrorConsumer {
ImmutableList<Aspect> aspects,
BuildConfiguration configuration,
BuildConfiguration hostConfiguration,
PatchTransition disableLipoTransition,
PrerequisiteValidator prerequisiteValidator,
ConfigurationFragmentPolicy configurationFragmentPolicy) {
this.env = Preconditions.checkNotNull(env);
Expand All @@ -1390,6 +1393,7 @@ public static final class Builder implements RuleErrorConsumer {
this.configurationFragmentPolicy = Preconditions.checkNotNull(configurationFragmentPolicy);
this.configuration = Preconditions.checkNotNull(configuration);
this.hostConfiguration = Preconditions.checkNotNull(hostConfiguration);
this.disableLipoTransition = disableLipoTransition;
this.prerequisiteValidator = prerequisiteValidator;
reporter = new ErrorReporter(env, rule, getRuleClassNameForLogging());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@

package com.google.devtools.build.lib.analysis;

import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
import com.google.devtools.build.lib.cmdline.Label;

import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -49,4 +49,14 @@ public interface RuleDefinitionEnvironment {
*/
@Nullable
Label getLauncherLabel();

/**
* Returns the C++ LIPO data transition, as defined in {@link
* com.google.devtools.build.lib.rules.cpp.transitions.DisableLipoTransition}.
*
* <p>This is language-specific, so doesn't really belong here. But since non-C++ rules declare
* this transition, we need universal access to it. The need for this interface should go away on
* the deprecation of LIPO for <a href="https://clang.llvm.org/docs/ThinLTO.html">ThinLTO</a>.
*/
PatchTransition getLipoDataTransition();
}
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@
import com.google.devtools.build.lib.rules.cpp.CpuTransformer;
import com.google.devtools.build.lib.rules.cpp.proto.CcProtoAspect;
import com.google.devtools.build.lib.rules.cpp.proto.CcProtoLibraryRule;
import com.google.devtools.build.lib.rules.cpp.transitions.DisableLipoTransition;
import com.google.devtools.build.lib.rules.extra.ActionListenerRule;
import com.google.devtools.build.lib.rules.extra.ExtraActionRule;
import com.google.devtools.build.lib.rules.genquery.GenQueryRule;
Expand Down Expand Up @@ -326,6 +327,23 @@ public ImmutableList<RuleSet> requires() {
}
};

/**
* Rules defined before this set will fail when trying to declare a data transition. So it's best
* to define this as early as possible.
*/
public static final RuleSet LIPO_DATA_TRANSITION =
new RuleSet() {
@Override
public void init(Builder builder) {
builder.setLipoDataTransition(DisableLipoTransition.INSTANCE);
}

@Override
public ImmutableList<RuleSet> requires() {
return ImmutableList.of();
}
};

public static final RuleSet CPP_RULES =
new RuleSet() {
@Override
Expand Down Expand Up @@ -624,6 +642,7 @@ public ImmutableList<RuleSet> requires() {

private static final ImmutableSet<RuleSet> RULE_SETS =
ImmutableSet.of(
LIPO_DATA_TRANSITION,
BAZEL_SETUP,
CoreRules.INSTANCE,
CoreWorkspaceRules.INSTANCE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.bazel.rules.common;

import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransitionProxy.DATA;
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
import static com.google.devtools.build.lib.packages.BuildType.LICENSE;
Expand Down Expand Up @@ -69,7 +68,7 @@ public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
<!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
.add(
attr("data", LABEL_LIST)
.cfg(DATA)
.cfg(env.getLipoDataTransition())
.allowedFileTypes(FileTypeSet.ANY_FILE)
.dontCheckConstraints())
.add(attr("output_licenses", LICENSE))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

package com.google.devtools.build.lib.rules.cpp;

import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransitionProxy.DATA;
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
Expand Down Expand Up @@ -94,7 +93,7 @@ public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
.add(attr("alwayslink", BOOLEAN))
.add(
attr("data", LABEL_LIST)
.cfg(DATA)
.cfg(env.getLipoDataTransition())
.allowedFileTypes(FileTypeSet.ANY_FILE)
.dontCheckConstraints())
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@
import com.google.devtools.build.lib.analysis.config.FragmentClassSet;
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransitionProxy;
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
import com.google.devtools.build.lib.analysis.config.transitions.Transition;
Expand Down Expand Up @@ -1134,9 +1133,7 @@ public BuildConfigurationCollection createConfigurations(
// apply LIPO.
BuildConfiguration firstTargetConfig = topLevelTargetConfigs.get(0);
Transition dataTransition =
((ConfiguredRuleClassProvider) ruleClassProvider)
.getDynamicTransitionMapper()
.map(ConfigurationTransitionProxy.DATA);
((ConfiguredRuleClassProvider) ruleClassProvider).getLipoDataTransition();
BuildOptions dataOptions = dataTransition != NoTransition.INSTANCE
? ((PatchTransition) dataTransition).apply(firstTargetConfig.getOptions())
: firstTargetConfig.getOptions();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@
import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Options.ConfigsMode;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransitionProxy;
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.config.transitions.NullTransition;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
Expand Down Expand Up @@ -1497,7 +1496,8 @@ protected BuildConfiguration getTargetConfiguration() {
}

protected BuildConfiguration getDataConfiguration() throws InterruptedException {
return getConfiguration(getTargetConfiguration(), ConfigurationTransitionProxy.DATA);
return getConfiguration(getTargetConfiguration(),
getRuleClassProvider().getLipoDataTransition());
}

protected BuildConfiguration getHostConfiguration() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ private void checkModule(RuleSet top) {
ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder();
builder.setToolsRepository(BazelRuleClassProvider.TOOLS_REPOSITORY);
Set<RuleSet> result = new HashSet<>();
result.add(BazelRuleClassProvider.LIPO_DATA_TRANSITION);
result.add(BazelRuleClassProvider.BAZEL_SETUP);
collectTransitiveClosure(result, top);
for (RuleSet module : result) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -968,6 +968,7 @@ protected AnalysisMock getAnalysisMock() {
public ConfiguredRuleClassProvider createRuleClassProvider() {
ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder();
builder.setToolsRepository("@bazel_tools");
BazelRuleClassProvider.LIPO_DATA_TRANSITION.init(builder);
BazelRuleClassProvider.BAZEL_SETUP.init(builder);
CoreRules.INSTANCE.init(builder);
CoreWorkspaceRules.INSTANCE.init(builder);
Expand Down

0 comments on commit e046d3a

Please sign in to comment.