feat: Expose Python C headers through the toolchain. #1287
Conversation
4cc337f to
ed31bb0
Compare
37e2d5a to
5fd2e12
Compare
b227a1e to
b81ed9e
Compare
This allows getting a build's `cc_library` of Python headers through toolchain resolution instead of having to use the underlying toolchain's repository `:python_headers` target directly. Without this feature, it's not possible to reliably and correctly get the C information about the runtime a build is going to use. Existing solutions require carefully setting up repo names, external state, and/or using specific build rules. In comparison, with this feature, consumers are able to simply ask for the current headers via a special target or manually lookup the toolchain and pull the relevant information; toolchain resolution handles finding the correct headers. The basic way this works is by registering a second toolchain to carry C/C++ related information; as such, it is named `py_cc_toolchain`. The py cc toolchain has the same constraint settings as the regular py toolchain; an expected invariant is that there is a 1:1 correspondence between the two. This base functionality allows a consuming rule implementation to use toolchain resolution to find the Python C toolchain information. Usually what downstream consumers need are the headers to feed into another `cc_library` (or equivalent) target, so, rather than have every project reimplement the same "lookup and forward cc_library info" logic, this is provided by the `//python/cc:current_py_cc_headers` target. Targets that need the headers can then depend on that target as if it was a `cc_library` target. Work towards bazel-contrib#824
|
Ok, CI is happy now -- ready for review. |
There was a problem hiding this comment.
Looks great! The tests are awesome. Lots of nitpicks.
| ) | ||
|
|
||
| def _test_current_toolchain_headers_impl(env, target): | ||
| compilation_context = env.expect.that_target(target).provider( |
There was a problem hiding this comment.
Can you add a few comments on what and how we are testing?
There was a problem hiding this comment.
Done, I think? The tests here are somewhat hand wavy because all we're doing is forwarding along information from the underlying target. Our fake_headers target just has a single header file, so it doesn't have much within it to check.
I'm not sure what you mean by "how", can you be more specific? These are analysis tests (https://bazel.build/rules/testing#testing-rules) and how they work is fairly well documented by Bazel and rules_testing (a macro setups up a graph of targets, then a rule inspects it to do asserts). The surrounding env.expect.that_target(...).contains_exactly(...) stuff are just helper APIs from rules_testing that eliminate a lot of the boiler plate of fetching information and checking state.
There was a problem hiding this comment.
Let me rephrase. The how is it running is with the bazel test rules. The internals of the test are checking somehow that the new rule is working. How is the test checking that? If we changed the code, what do I need to know to update the test?
There was a problem hiding this comment.
I'm still not really clear about what additional comments you want?
I get the impression that you're not familiar with analysis tests and/or rules_testing's apis, and want comments added describing what analysis tests are, how they work, and how rules_testing's API fetch and assert values? But that's beyond the scope and responsibilities of a single test; we have dozens of analysis tests, see tools/build_defs/python/tests.
The essence of an analysis test is it's just a rule implementation that looks at the returned providers of a target. What it looks at, how it gets it, how it asserts the values -- that all depends on the particulars of what it's trying to check. So I can't really answer the question of "If we changed the code, what do I need to know?" beyond a vague "you need to know how to get the appropriate value and assert its state".
In this case, all we're doing is getting the CcInfo provider and looking at a couple of its fields.
| ) | ||
|
|
||
| def _test_current_toolchain_headers_impl(env, target): | ||
| compilation_context = env.expect.that_target(target).provider( |
There was a problem hiding this comment.
Done, I think? The tests here are somewhat hand wavy because all we're doing is forwarding along information from the underlying target. Our fake_headers target just has a single header file, so it doesn't have much within it to check.
I'm not sure what you mean by "how", can you be more specific? These are analysis tests (https://bazel.build/rules/testing#testing-rules) and how they work is fairly well documented by Bazel and rules_testing (a macro setups up a graph of targets, then a rule inspects it to do asserts). The surrounding env.expect.that_target(...).contains_exactly(...) stuff are just helper APIs from rules_testing that eliminate a lot of the boiler plate of fetching information and checking state.
There was a problem hiding this comment.
Just a couple of nits
| ) | ||
|
|
||
| def _test_current_toolchain_headers_impl(env, target): | ||
| compilation_context = env.expect.that_target(target).provider( |
There was a problem hiding this comment.
Let me rephrase. The how is it running is with the bazel test rules. The internals of the test are checking somehow that the new rule is working. How is the test checking that? If we changed the code, what do I need to know to update the test?
This allows getting a build's
cc_libraryof Python headers throughtoolchain resolution instead of having to use the underlying toolchain's
repository
:python_headerstarget directly.Without this feature, it's not possible to reliably and correctly get
the C information about the runtime a build is going to use. Existing
solutions require carefully setting up repo names, external state,
and/or using specific build rules. In comparison, with this feature,
consumers are able to simply ask for the current headers via a helper
target or manually lookup the toolchain and pull the relevant
information; toolchain resolution handles finding the correct headers.
The basic way this works is by registering a second toolchain to carry
C/C++ related information; as such, it is named
py_cc_toolchain. Thepy cc toolchain has the same constraint settings as the regular py
toolchain; an expected invariant is that there is a 1:1 correspondence
between the two. This base functionality allows a consuming rule
implementation to use toolchain resolution to find the Python C
toolchain information.
Usually what downstream consumers need are the headers to feed into
another
cc_library(or equivalent) target, so, rather than have everyproject re-implement the same "lookup and forward cc_library info" logic,
this is provided by the
//python/cc:current_py_cc_headerstarget.Targets that need the headers can then depend on that target as if it
was a
cc_librarytarget.Work towards #824