Skip to content

Commit

Permalink
Add a new toolchain type for c++. In order to do this, PlatformConfig…
Browse files Browse the repository at this point in the history
…uration is made a legal configuration fragment for every rule class.

Add a default "dummy" c++ toolchain to prevent resolution errors when legacy toolchain selection logic is used.  Add toolchain mocks to java and shell tests.

PiperOrigin-RevId: 167901210
  • Loading branch information
calpeyser authored and meteorcloudy committed Sep 8, 2017
1 parent f7fc22e commit d852e48
Show file tree
Hide file tree
Showing 25 changed files with 285 additions and 59 deletions.
4 changes: 4 additions & 0 deletions compile.sh
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,11 @@ source scripts/bootstrap/bootstrap.sh
new_step 'Building Bazel with Bazel'
display "."
log "Building output/bazel"
# We set host and target platform directly since the defaults in @bazel_tools
# have not yet been generated.
bazel_build "src:bazel${EXE_EXT}" \
--experimental_host_platform=//tools/platforms:host_platform \
--experimental_platforms=//tools/platforms:target_platform \
|| fail "Could not build Bazel"
bazel_bin_path="$(get_bazel_bin_path)/src/bazel${EXE_EXT}"
[ -e "$bazel_bin_path" ] \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,41 +182,60 @@ public Metadata getMetadata() {
* Share common attributes across both base and Skylark base rules.
*/
public static RuleClass.Builder commonCoreAndSkylarkAttributes(RuleClass.Builder builder) {
return builder
return PlatformSemantics.platformAttributes(builder)
// The visibility attribute is special: it is a nodep label, and loading the
// necessary package groups is handled by {@link LabelVisitor#visitTargetVisibility}.
// Package groups always have the null configuration so that they are not duplicated
// needlessly.
.add(attr("visibility", NODEP_LABEL_LIST).orderIndependent().cfg(HOST)
.nonconfigurable("special attribute integrated more deeply into Bazel's core logic"))
.add(attr("deprecation", STRING).value(deprecationDefault)
.nonconfigurable("Used in core loading phase logic with no access to configs"))
.add(attr("tags", STRING_LIST).orderIndependent().taggable()
.nonconfigurable("low-level attribute, used in TargetUtils without configurations"))
.add(attr("generator_name", STRING).undocumented("internal")
.nonconfigurable("static structure of a rule"))
.add(attr("generator_function", STRING).undocumented("internal")
.nonconfigurable("static structure of a rule"))
.add(attr("generator_location", STRING).undocumented("internal")
.nonconfigurable("static structure of a rule"))
.add(attr("testonly", BOOLEAN).value(testonlyDefault)
.nonconfigurable("policy decision: rules testability should be consistent"))
.add(
attr("visibility", NODEP_LABEL_LIST)
.orderIndependent()
.cfg(HOST)
.nonconfigurable(
"special attribute integrated more deeply into Bazel's core logic"))
.add(
attr("deprecation", STRING)
.value(deprecationDefault)
.nonconfigurable("Used in core loading phase logic with no access to configs"))
.add(
attr("tags", STRING_LIST)
.orderIndependent()
.taggable()
.nonconfigurable("low-level attribute, used in TargetUtils without configurations"))
.add(
attr("generator_name", STRING)
.undocumented("internal")
.nonconfigurable("static structure of a rule"))
.add(
attr("generator_function", STRING)
.undocumented("internal")
.nonconfigurable("static structure of a rule"))
.add(
attr("generator_location", STRING)
.undocumented("internal")
.nonconfigurable("static structure of a rule"))
.add(
attr("testonly", BOOLEAN)
.value(testonlyDefault)
.nonconfigurable("policy decision: rules testability should be consistent"))
.add(attr("features", STRING_LIST).orderIndependent())
.add(attr(":action_listener", LABEL_LIST).cfg(HOST).value(ACTION_LISTENER))
.add(attr(RuleClass.COMPATIBLE_ENVIRONMENT_ATTR, LABEL_LIST)
.allowedRuleClasses(EnvironmentRule.RULE_NAME)
.cfg(Attribute.ConfigurationTransition.HOST)
.allowedFileTypes(FileTypeSet.NO_FILE)
.dontCheckConstraints()
.nonconfigurable("special logic for constraints and select: see ConstraintSemantics")
)
.add(attr(RuleClass.RESTRICTED_ENVIRONMENT_ATTR, LABEL_LIST)
.allowedRuleClasses(EnvironmentRule.RULE_NAME)
.cfg(Attribute.ConfigurationTransition.HOST)
.allowedFileTypes(FileTypeSet.NO_FILE)
.dontCheckConstraints()
.nonconfigurable("special logic for constraints and select: see ConstraintSemantics")
);
.add(
attr(RuleClass.COMPATIBLE_ENVIRONMENT_ATTR, LABEL_LIST)
.allowedRuleClasses(EnvironmentRule.RULE_NAME)
.cfg(Attribute.ConfigurationTransition.HOST)
.allowedFileTypes(FileTypeSet.NO_FILE)
.dontCheckConstraints()
.nonconfigurable(
"special logic for constraints and select: see ConstraintSemantics"))
.add(
attr(RuleClass.RESTRICTED_ENVIRONMENT_ATTR, LABEL_LIST)
.allowedRuleClasses(EnvironmentRule.RULE_NAME)
.cfg(Attribute.ConfigurationTransition.HOST)
.allowedFileTypes(FileTypeSet.NO_FILE)
.dontCheckConstraints()
.nonconfigurable(
"special logic for constraints and select: see ConstraintSemantics"));
}

public static RuleClass.Builder nameAttribute(RuleClass.Builder builder) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public class PlatformOptions extends FragmentOptions {
@Option(
name = "extra_toolchains",
converter = LabelListConverter.class,
defaultValue = "",
defaultValue = "@bazel_tools//tools/cpp:dummy_cc_toolchain",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.UNKNOWN},
metadataTags = {OptionMetadataTag.HIDDEN},
Expand Down Expand Up @@ -98,6 +98,8 @@ public class PlatformOptions extends FragmentOptions {
public PlatformOptions getHost(boolean fallback) {
PlatformOptions host = (PlatformOptions) getDefault();
host.platforms = ImmutableList.of(this.hostPlatform);
host.hostPlatform = this.hostPlatform;
host.extraToolchains = this.extraToolchains;
return host;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import java.util.List;
import javax.annotation.Nullable;

/** Helper class to manage rules' use of platforms. */
public class PlatformSemantics {
Expand All @@ -49,6 +50,7 @@ public List<Label> resolve(
};

/** Implementation for the :execution_platform attribute. */
@Nullable
public static final Attribute.LateBoundLabel<BuildConfiguration> EXECUTION_PLATFORM =
new Attribute.LateBoundLabel<BuildConfiguration>(PlatformConfiguration.class) {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import com.google.devtools.build.lib.analysis.BaseRuleClasses;
import com.google.devtools.build.lib.analysis.DefaultInfo;
import com.google.devtools.build.lib.analysis.OutputGroupProvider;
import com.google.devtools.build.lib.analysis.PlatformSemantics;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.skylark.SkylarkAttr.Descriptor;
import com.google.devtools.build.lib.analysis.test.TestConfiguration;
Expand Down Expand Up @@ -129,10 +128,9 @@ public Label load(String from) throws Exception {
/** Parent rule class for non-executable non-test Skylark rules. */
public static final RuleClass baseRule =
BaseRuleClasses.commonCoreAndSkylarkAttributes(
PlatformSemantics.platformAttributes(
BaseRuleClasses.nameAttribute(
new RuleClass.Builder("$base_rule", RuleClassType.ABSTRACT, true))
.add(attr("expect_failure", STRING))))
BaseRuleClasses.nameAttribute(
new RuleClass.Builder("$base_rule", RuleClassType.ABSTRACT, true))
.add(attr("expect_failure", STRING)))
.build();

/** Parent rule class for executable non-test Skylark rules. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import com.google.devtools.build.lib.rules.cpp.CcToolchain;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
import com.google.devtools.build.lib.rules.cpp.CppFileTypes;
import com.google.devtools.build.lib.rules.cpp.CppHelper;
import com.google.devtools.build.lib.rules.cpp.CppRuleClasses;
import com.google.devtools.build.lib.rules.cpp.CppRuleClasses.LipoTransition;
import com.google.devtools.build.lib.util.FileTypeSet;
Expand Down Expand Up @@ -139,6 +140,7 @@ public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
attr(CcToolchain.CC_TOOLCHAIN_DEFAULT_ATTRIBUTE_NAME, LABEL)
.value(CppRuleClasses.ccToolchainAttribute(env)))
.setPreferredDependencyPredicate(Predicates.<String>or(CPP_SOURCE, C_SOURCE, CPP_HEADER))
.addRequiredToolchains(CppHelper.getCcToolchainType(env.getToolsRepository()))
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Ordering;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
Expand Down Expand Up @@ -997,6 +998,11 @@ public Builder addRequiredToolchains(Iterable<Label> toolchainLabels) {
return this;
}

public Builder addRequiredToolchains(Label... toolchainLabels) {
Iterables.addAll(this.requiredToolchains, Lists.newArrayList(toolchainLabels));
return this;
}

/**
* Returns an Attribute.Builder object which contains a replica of the
* same attribute in the parent rule if exists.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ public class CppHelper {
private static final ImmutableList<String> LINKOPTS_PREREQUISITE_LABEL_KINDS =
ImmutableList.of("deps", "srcs");

/** Returns label used to select resolved cc_toolchain instances based on platform. */
public static Label getCcToolchainType(String toolsRepository) {
return Label.parseAbsoluteUnchecked(toolsRepository + "//tools/cpp:toolchain_type");
}

private CppHelper() {
// prevents construction
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@

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

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.util.ResourceUsage;
import com.google.devtools.build.skyframe.AbstractSkyFunctionEnvironment;
Expand All @@ -33,21 +35,27 @@ public class SkyFunctionEnvironmentForTesting extends AbstractSkyFunctionEnviron

private final BuildDriver buildDriver;
private final ExtendedEventHandler eventHandler;
private final SkyframeExecutor skyframeExecutor;

/** Creates a SkyFunctionEnvironmentForTesting that uses a BuildDriver to evaluate skykeys. */
public SkyFunctionEnvironmentForTesting(
BuildDriver buildDriver, ExtendedEventHandler eventHandler) {
BuildDriver buildDriver,
ExtendedEventHandler eventHandler,
SkyframeExecutor skyframeExecutor) {
this.buildDriver = buildDriver;
this.eventHandler = eventHandler;
this.skyframeExecutor = skyframeExecutor;
}

@Override
protected Map<SkyKey, ValueOrUntypedException> getValueOrUntypedExceptions(
Iterable<? extends SkyKey> depKeys) throws InterruptedException {
ImmutableMap.Builder<SkyKey, ValueOrUntypedException> resultMap = ImmutableMap.builder();
Iterable<SkyKey> keysToEvaluate = ImmutableList.copyOf(depKeys);
EvaluationResult<SkyValue> evaluationResult =
skyframeExecutor.evaluateSkyKeys(eventHandler, keysToEvaluate, true);
buildDriver.evaluate(depKeys, true, ResourceUsage.getAvailableProcessors(), eventHandler);
for (SkyKey depKey : depKeys) {
for (SkyKey depKey : ImmutableSet.copyOf(depKeys)) {
resultMap.put(depKey, ValueOrExceptionUtils.ofValue(evaluationResult.get(depKey)));
}
return resultMap.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,7 @@ public ToolchainContext getToolchainContextForTesting(
Set<Label> requiredToolchains, BuildConfiguration config, ExtendedEventHandler eventHandler)
throws ToolchainContextException, InterruptedException {
SkyFunctionEnvironmentForTesting env =
new SkyFunctionEnvironmentForTesting(buildDriver, eventHandler);
new SkyFunctionEnvironmentForTesting(buildDriver, eventHandler, this);
return ToolchainUtil.createToolchainContext(env, "", requiredToolchains, config);
}

Expand Down Expand Up @@ -1534,7 +1534,7 @@ private EvaluationResult<SkyValue> evaluateSkyKeys(
* Evaluates the given sky keys, blocks, and returns their evaluation results. Enables/disables
* "keep going" on evaluation errors as specified.
*/
private EvaluationResult<SkyValue> evaluateSkyKeys(
EvaluationResult<SkyValue> evaluateSkyKeys(
final ExtendedEventHandler eventHandler,
final Iterable<SkyKey> skyKeys,
final boolean keepGoing) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1238,8 +1238,8 @@ public void testTopLevelTargetsAreTrimmedWithDynamicConfigurations() throws Exce
useConfiguration("--experimental_dynamic_configs=on");
AnalysisResult res = update("//foo:x");
ConfiguredTarget topLevelTarget = Iterables.getOnlyElement(res.getTargetsToBuild());
assertThat(topLevelTarget.getConfiguration().getAllFragments().keySet()).containsExactly(
ruleClassProvider.getUniversalFragment());
assertThat(topLevelTarget.getConfiguration().getAllFragments().keySet())
.containsExactly(ruleClassProvider.getUniversalFragment(), PlatformConfiguration.class);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ public void setup(MockToolsConfig config) throws IOException {
config.create(
"/bazel_tools_workspace/tools/cpp/BUILD",
"package(default_visibility=['//visibility:public'])",
"toolchain_type(name = 'toolchain_type')",
"cc_library(name = 'stl')",
"cc_library(name = 'malloc')",
"cc_toolchain_suite(",
Expand Down Expand Up @@ -123,7 +122,7 @@ public void setup(MockToolsConfig config) throws IOException {
" linker_files = ':empty',",
" module_map = 'crosstool.cppmap', supports_header_parsing = 1,",
" objcopy_files = ':empty', static_runtime_libs = [':empty'], strip_files = ':empty',",
")",
")",
"cc_toolchain(name = 'cc-compiler-armeabi-v7a', all_files = ':empty', ",
" compiler_files = ':empty',",
" cpu = 'local', dwp_files = ':empty', dynamic_runtime_libs = [':empty'], ",
Expand All @@ -145,8 +144,27 @@ public void setup(MockToolsConfig config) throws IOException {
"filegroup(",
" name = 'link_dynamic_library',",
" srcs = ['link_dynamic_library.sh'],",
")");

")",
"toolchain_type(name = 'toolchain_type')",
"toolchain(",
" name = 'toolchain_cc-compiler-piii',",
" toolchain_type = ':toolchain_type',",
" toolchain = '//third_party/crosstool/mock:cc-compiler-piii',",
" target_compatible_with = [':mock_value'],",
")",
"toolchain(",
" name = 'dummy_cc_toolchain',",
" toolchain_type = ':toolchain_type',",
" toolchain = ':dummy_cc_toolchain_impl',",
")",
"load(':dummy_toolchain.bzl', 'dummy_toolchain')",
"dummy_toolchain(name = 'dummy_cc_toolchain_impl')");
config.create(
"/bazel_tools_workspace/tools/cpp/dummy_toolchain.bzl",
"def _dummy_toolchain_impl(ctx):",
" toolchain = platform_common.ToolchainInfo()",
" return [toolchain]",
"dummy_toolchain = rule(_dummy_toolchain_impl, attrs = {})");
config.create(
"/bazel_tools_workspace/tools/cpp/CROSSTOOL",
readCrosstoolFile());
Expand All @@ -156,6 +174,7 @@ public void setup(MockToolsConfig config) throws IOException {
config.create("tools/cpp/link_dynamic_library.sh", "");
}
MockObjcSupport.setup(config);
MockPlatformSupport.setup(config, "/bazel_tools_workspace/platforms");
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.google.devtools.build.lib.packages.util;

import java.io.IOException;
Expand All @@ -20,9 +19,10 @@
public class MockPlatformSupport {

/** Adds mocks for basic host and target platform. */
public static void setup(MockToolsConfig mockToolsConfig) throws IOException {
public static void setup(MockToolsConfig mockToolsConfig, String platformsPath)
throws IOException {
mockToolsConfig.create(
"buildenv/platforms/BUILD",
platformsPath + "/BUILD",
"package(default_visibility=['//visibility:public'])",
"platform(",
" name = 'target_platform',",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,7 @@ public ConfiguredRuleClassProvider createRuleClassProvider() {
BazelRuleClassProvider.BAZEL_SETUP.init(builder);
CoreRules.INSTANCE.init(builder);
BazelRuleClassProvider.CORE_WORKSPACE_RULES.init(builder);
BazelRuleClassProvider.PLATFORM_RULES.init(builder);
BazelRuleClassProvider.GENERIC_RULES.init(builder);
BazelRuleClassProvider.CPP_RULES.init(builder);
builder.addRuleDefinition(new OnlyCppToolchainTypeRule());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,10 @@ public void testRegisteredToolchains() throws Exception {
assertThatEvaluationResult(result).hasEntryThat(toolchainsKey).isNotNull();

RegisteredToolchainsValue value = result.get(toolchainsKey);
assertThat(value.registeredToolchains()).hasSize(2);
// We have two registered toolchains, and one default for c++
assertThat(value.registeredToolchains()).hasSize(3);

DeclaredToolchainInfo registeredToolchain1 = value.registeredToolchains().get(0);
DeclaredToolchainInfo registeredToolchain1 = value.registeredToolchains().get(1);
assertThat(registeredToolchain1).isNotNull();

assertThat(registeredToolchain1.toolchainType()).isEqualTo(testToolchainType);
Expand All @@ -68,7 +69,7 @@ public void testRegisteredToolchains() throws Exception {
assertThat(registeredToolchain1.toolchainLabel())
.isEqualTo(makeLabel("//toolchain:test_toolchain_1"));

DeclaredToolchainInfo registeredToolchain2 = value.registeredToolchains().get(1);
DeclaredToolchainInfo registeredToolchain2 = value.registeredToolchains().get(2);
assertThat(registeredToolchain2).isNotNull();

assertThat(registeredToolchain2.toolchainType()).isEqualTo(testToolchainType);
Expand Down Expand Up @@ -137,7 +138,7 @@ public void testRegisteredToolchains_reload() throws Exception {
requestToolchainsFromSkyframe(toolchainsKey);
assertThatEvaluationResult(result).hasNoError();
assertToolchainLabels(result.get(toolchainsKey))
.containsExactly(makeLabel("//toolchain:test_toolchain_1"));
.contains(makeLabel("//toolchain:test_toolchain_1"));

// Re-write the WORKSPACE.
rewriteWorkspace("register_toolchains('//toolchain:toolchain_2')");
Expand All @@ -146,7 +147,7 @@ public void testRegisteredToolchains_reload() throws Exception {
result = requestToolchainsFromSkyframe(toolchainsKey);
assertThatEvaluationResult(result).hasNoError();
assertToolchainLabels(result.get(toolchainsKey))
.containsExactly(makeLabel("//toolchain:test_toolchain_2"));
.contains(makeLabel("//toolchain:test_toolchain_2"));
}

@Test
Expand Down
Loading

0 comments on commit d852e48

Please sign in to comment.