Update ConfiguredTargetFunction.computeUnloadedToolchainContexts to #13258
Conversation
directly require the correct execution platform. This fixes several issues around toolchains that depend on toolchains. Fixes bazelbuild#13243.
src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java
Outdated
Show resolved
Hide resolved
| @@ -520,18 +520,31 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc | |||
|
|
|||
| Map<String, ToolchainContextKey> toolchainContextKeys = new HashMap<>(); | |||
| String targetUnloadedToolchainContext = "target-unloaded-toolchain-context"; | |||
| ToolchainContextKey toolchainContextKey; | |||
|
|
|||
| ToolchainContextKey.Builder toolchainContextKeyBuilder = | |||
There was a problem hiding this comment.
I'm having trouble keeping track of all the interoperating pieces.
What precisely does the parent toolchain context mean w.r.t. the current one? That's the toolchain context of the configured target that may or may not exist, depending on which key instantiated ConfiguredTargetFunction?
What's the full logic flow here? What exactly does dropping and loop avoidance do?
I just need a bit more of an overview to contextualize my understanding.
There was a problem hiding this comment.
When a configured target CT depends on a toolchain T, it also passed CT's ToolchainContextKey along. This allows T to make sure that it uses the same execution platform, to enable T's dependencies to use the exec transition and target the correct platform.
Previously, we were just throwing away T's toolchain context key, but that isn't correct: T may have its own required toolchain types. This change is adding a clearer way, where T can determine directly that CT's execution platform is, and then forcing toolchain resolution to use that (or throw an error, if it's not possible for some reason).
The specific use here is that rules_rust's rust_toolchain rule needs a cc_toolchain, and that was failing with the previous code.
There was a problem hiding this comment.
Ah yes, I remember more of the general ToolchainContextKey semantics better, thanks. I don't think I'd considered before the idea of a toolchain requiring another toolchain - that's an interesting extension of the functionality.
|
Fixed the test failure and cleaned up ToolchainResolutionFunction a bit. |
directly require the correct execution platform.
This fixes several issues around toolchains that depend on toolchains.
Fixes #13243.