Add new ToolchainTypeInfo provider to track toolchain type information. #6235

katre · 2018-09-25T16:29:33Z

Will eventually be part of checking the toolchain provider types, as
part of #6015.

Change-Id: I74e9372fb19d1f78c00a340c484c3c4d4b2f2ab1

Will eventually be part of checking the toolchain provider types, as part of bazelbuild#6015. Change-Id: I74e9372fb19d1f78c00a340c484c3c4d4b2f2ab1

This will be required after bazelbuild#6235 requires actual dependencies on toolchain_type targets. Part of the work on bazelbuild#6015. Change-Id: I21e5399dc0c43bb7b4d42e1f1bd5b9a1c820075e

gregestren

Forgive my ignorance, but this is motivated by C++ toolchain resolution, and the desire to not have to litter awkward checking throughout the code base, right?

What's the current state of Starlark-defined C++ toolchain definitions and the rules that consume them? i..e. which rule definitions for C++ would this logic use to do the checking?

gregestren · 2018-09-26T21:58:10Z

src/main/java/com/google/devtools/build/lib/analysis/platform/ToolchainTypeInfo.java

@@ -0,0 +1,70 @@
+package com.google.devtools.build.lib.analysis.platform;


File header comments?

gregestren · 2018-09-26T22:00:44Z

src/main/java/com/google/devtools/build/lib/rules/platform/ToolchainRule.java


        /* <!-- #BLAZE_RULE(toolchain).ATTRIBUTE(toolchain_type) -->
        The type of the toolchain, which should be the label of a toolchain_type() target.
        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
        .add(
-            attr(TOOLCHAIN_TYPE_ATTR, BuildType.NODEP_LABEL)
+            attr(TOOLCHAIN_TYPE_ATTR, BuildType.LABEL)


What does the NODEP_LABEL to LABEL change signify?

I assume this is so that the toolchain_type() rule referred to by the label gets analyzed and thus the ToolchainTypeInfo gets created.

Yes, exactly. There's now an actual dependency on the toolchain type, so:
a) it must exist, and
b) the new provider can now be used.

gregestren · 2018-09-26T22:02:00Z

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/platform/ToolchainTypeInfoApi.java

+import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
+
+/** Info object representing data about a specific platform. */


Is this the right comment?

Nope, fixed it.

lberki · 2018-09-27T09:20:16Z

src/main/java/com/google/devtools/build/lib/analysis/platform/ToolchainTypeInfo.java

+  public static final NativeProvider<ToolchainTypeInfo> PROVIDER =
+      new NativeProvider<ToolchainTypeInfo>(ToolchainTypeInfo.class, SKYLARK_NAME) {};
+
+  private final Label typeLabel;


What other information is expected to be here other than the label?

I have a branch in progress that adds a (string-valued) provider_id attribute, which can then be checked against the actual provider during toolchain resolution.

Understood! I don't know what your plans are and thus I can't say anything wise about them, but I think this change by itself is a net win, since I consider nodep labels a hack (they are confusing because label attributes usually mean a dependency).

I'm somewhat worried about the "string-valued provider_id attribute" part, but we can discuss that in a separate thread.

lberki · 2018-09-27T09:21:38Z

src/main/java/com/google/devtools/build/lib/rules/platform/ToolchainRule.java


        /* <!-- #BLAZE_RULE(toolchain).ATTRIBUTE(toolchain_type) -->
        The type of the toolchain, which should be the label of a toolchain_type() target.
        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
        .add(
-            attr(TOOLCHAIN_TYPE_ATTR, BuildType.NODEP_LABEL)
+            attr(TOOLCHAIN_TYPE_ATTR, BuildType.LABEL)


I assume this is so that the toolchain_type() rule referred to by the label gets analyzed and thus the ToolchainTypeInfo gets created.

This will be required after #6235 requires actual dependencies on toolchain_type targets. Part of the work on #6015. Change-Id: I21e5399dc0c43bb7b4d42e1f1bd5b9a1c820075e Closes #6239. Change-Id: I9d784437e8838875b3f8618e53752ba1f635f31e PiperOrigin-RevId: 214768353

Change-Id: Iec98478ea0051440129d9e19efbf6c3aff68d3da

Change-Id: I6022b2f8ace0b41599753584823a3fda82b85370

katre · 2018-09-27T16:05:37Z

Correct, this came about from a conversation @lberki and I had about type checking C++ toolchain providers. The same issues will also come up for Java toolchain providers.

While the C++ crosstool is moving into Starlark, I'm not aware of any long-term plans to re-write the CppToolchainProvider as Starlark code, as it is very heavily used in Bazel internals. Given that, we still need the check that a target that requires a c++ toolchain gets an actual CppToolchainProvider, in order to avoid the current potential class cast errors.

Change-Id: Ic1d98c1cbeb3c40a3d3a81c03ccfb6c8f0a87b86

gregestren

Thanks for the explanation. I defer to @lberki for further comments on this change.

As to the larger issue, the exact API for (2) in #6015 still remains TBD?

katre · 2018-09-27T20:53:13Z

For the larger check, I need to test what exactly is available to validate on. Every toolchain has to be a subclass of ToolchainInfo, so that is the actual provider type for them all. I may need to do class name matching for native providers instead, or add a "type" to the ToolchainInfo for comparison. Still considering.

katre · 2018-10-02T14:32:24Z

Yes, still working out the best way to do the actual check on the provider type. It turns out to be fairly tricky. But I think this change is still worthwhile even without the provider type checking.

Change-Id: I53d6a0d41c05029b98dddac4c8b8a7f60aa218d6

Change-Id: I80592a3e130dea99bee9ccecfaaf2b64b73bc80a

*** Reason for rollback *** This change is causing many rules_* jobs to fail at HEAD: https://buildkite.com/bazel/bazel-with-downstream-projects-bazel/builds/480 Filed issue: #6311 *** Original change description *** Add new ToolchainTypeInfo provider to track toolchain type information. Will eventually be part of checking the toolchain provider types, as part of #6015. Closes #6235. PiperOrigin-RevId: 215911302

Add new ToolchainTypeInfo provider to track toolchain type information.

0a5a5a1

Will eventually be part of checking the toolchain provider types, as part of bazelbuild#6015. Change-Id: I74e9372fb19d1f78c00a340c484c3c4d4b2f2ab1

katre requested a review from gregestren September 25, 2018 16:29

katre requested a review from lberki as a code owner September 25, 2018 16:29

googlebot added the cla: yes label Sep 25, 2018

katre mentioned this pull request Sep 25, 2018

Fix the bootstrap scripts to use the correct BUILD file for tools/sh. #6239

Closed

iirina added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Sep 26, 2018

gregestren reviewed Sep 26, 2018

View reviewed changes

lberki reviewed Sep 27, 2018

View reviewed changes

katre added 2 commits September 27, 2018 12:02

Added copyright headers

7f2bf60

Change-Id: Iec98478ea0051440129d9e19efbf6c3aff68d3da

Fixed doc comment

e66801b

Change-Id: I6022b2f8ace0b41599753584823a3fda82b85370

Merge branch 'master' into ttp-01-toolchain-type-provider

50e54d1

Change-Id: Ic1d98c1cbeb3c40a3d3a81c03ccfb6c8f0a87b86

gregestren approved these changes Sep 27, 2018

View reviewed changes

lberki approved these changes Oct 1, 2018

View reviewed changes

katre added 2 commits October 3, 2018 10:41

Merge branch 'master' into ttp-01-toolchain-type-provider

f4f095e

Change-Id: I53d6a0d41c05029b98dddac4c8b8a7f60aa218d6

Updated PlatformProviderUtils for API changes

9e5af06

Change-Id: I80592a3e130dea99bee9ccecfaaf2b64b73bc80a

bazel-io closed this in 2348893 Oct 4, 2018

katre deleted the ttp-01-toolchain-type-provider branch October 23, 2018 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new ToolchainTypeInfo provider to track toolchain type information. #6235

Add new ToolchainTypeInfo provider to track toolchain type information. #6235

katre commented Sep 25, 2018

gregestren left a comment

gregestren Sep 26, 2018

katre Sep 27, 2018

gregestren Sep 26, 2018

lberki Sep 27, 2018

katre Sep 27, 2018

gregestren Sep 26, 2018

katre Sep 27, 2018

lberki Sep 27, 2018

katre Sep 27, 2018

lberki Oct 1, 2018

lberki Sep 27, 2018

katre commented Sep 27, 2018

gregestren left a comment

katre commented Sep 27, 2018

katre commented Oct 2, 2018

		@@ -0,0 +1,70 @@
		package com.google.devtools.build.lib.analysis.platform;

Add new ToolchainTypeInfo provider to track toolchain type information. #6235

Add new ToolchainTypeInfo provider to track toolchain type information. #6235

Conversation

katre commented Sep 25, 2018

gregestren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katre commented Sep 27, 2018

gregestren left a comment

Choose a reason for hiding this comment

katre commented Sep 27, 2018

katre commented Oct 2, 2018