Let use_target_platform_constraints copy unmatched constraints - #30688
Let use_target_platform_constraints copy unmatched constraints#30688moroten wants to merge 1 commit into
Conversation
Instead of requiring empty target_compatible_with and
exec_compatible_with, use the following backwards compatible logic:
exec_constraints = toolchain.exec_compatible_with
if toolchain.use_target_platform_constraints:
unmatched_constraints = target_platform.constraints - toolchain.target_compatible_with
exec_constraints += unmatched_constraints
This is useful for the following test toolchain, where building a test
for a target platform with wasm+gpu constraints should not execute the
test on a platform without gpu. In this example, the execution requirements
will be linux+x86_64+gpu.
toolchain(
name = "wasm_on_linux",
exec_compatible_with = ["@platforms//os:linux", "@platforms//cpu:x86_64"],
target_compatible_with = ["@platforms//cpu:wasm64"],
use_target_platform_constraints = True,
toolchain = "@bazel_tools//tools/test:empty_toolchain",
toolchain_type = "@bazel_tools//tools/test:default_test_toolchain_type",
)
Fixes bazelbuild#30560
github-actions
Bot
added
team-Configurability
The RELNOTES read as if this was an incompatible change, which it isn't (since such toolchains would previously have resulted in an error). I think it's worth phrasing that differently. |
Updated RELNOTES to be more general. The docs describes the functionality anyway. |
fmeum
left a comment
fmeum
left a comment
There was a problem hiding this comment.
Now that I have reviewed this more closely, I'm wondering whether the algorithm really has the right level of generality. In your motivating example, a reasonable target platform for WASM may set @platforms//os:none or @platforms//os:wasi, which would then cause the toolchain to no longer match an execution platform with other constraint values for that setting.
Do you have other motivating examples? Perhaps we need more complex logic that maps target to exec constraints or allow the exec platform to override values?
| * constraints of the target platform. | ||
| */ | ||
| @Test | ||
| public void testExecPlatformMatchesTargetConstraintsWithDefaultTestToolchainFRME() throws Exception { |
There was a problem hiding this comment.
The name doesn't match the content (and FRME is probably not meant to be there either ;-))
| @@ -58,27 +59,15 @@ | |||
| } | |||
|
|
|||
| public boolean hasTargetToExecConstraints() { | |||
There was a problem hiding this comment.
This can just be dropped/inlined
| ToolchainTypeInfo toolchainType, | ||
| ConstraintCollection execConstraints, | ||
| ConstraintCollection targetConstraints, | ||
| boolean useTargetPlatformConstraints, |
| * their {@link ConstraintSettingInfo}. | ||
| */ | ||
| public ImmutableList<ConstraintValueInfo> findMissing(ConstraintCollection expected) { | ||
| return findMissing(expected.constraints().values()); |
There was a problem hiding this comment.
This may be preexisting, but I think that parent on a platform isn't handled correctly by this as well as the "target to exec" logic. Could you add a test exercising that behavior and fix it if needed?
| # :linux | ||
| # :aarch64 | ||
| # :is_device | ||
| # linux_gpu_test_toolchain will require exec platform with: |
There was a problem hiding this comment.
The comment needs to be adapted to this test case, there is no linux_gpu_test_toolchain.
| assertThat(testAction.getExecProperties()).containsExactly("os", targetOs, "cpu", targetCpu); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
I would like to see a test that verifies that target_compatible_with is checked for a toolchain with use_target_platform_constraints.
| * rules. | ||
| */ | ||
| @AutoCodec | ||
| public record DeclaredToolchainInfo( |
There was a problem hiding this comment.
Either update DeclaredToolchainInfoTest.toolchainInfo_equalsTester or (preferably) drop it now that this is a record.
Does such a WASM platform require that no OS is running? Is WASI actually an OS or something that may be available on any OS?
My motivating example is in Buildbarn. |
I see how this feature would be useful there, but if I'm being honest, Based on your WASM example, I could see us needing to specify entire constraint settings (rather than values) to ignore when doing the target -> exec constraint transformation. But I would want to wait for more real world use cases hitting this before deciding what the "right" level of genericity is. Sorry for not noticing this earlier! |
2 participants
Description
Instead of requiring empty target_compatible_with and exec_compatible_with, use the following backwards compatible logic:
Motivation
This is useful for the following test toolchain, where building a test for a target platform with wasm+gpu constraints should not execute the test on a platform without gpu. In this example, the execution requirements will be linux+x86_64+gpu.
toolchain(
name = "wasm_on_linux",
exec_compatible_with = ["@platforms//os:linux", "@platforms//cpu:x86_64"],
target_compatible_with = ["@platforms//cpu:wasm64"],
use_target_platform_constraints = True,
toolchain = "@bazel_tools//tools/test:empty_toolchain",
toolchain_type = "@bazel_tools//tools/test:default_test_toolchain_type",
)
Fixes #30560
Build API Changes
Backwards compatible change of the
use_target_platform_constraints=Truecase of thetoolchainrule.Checklist
Release Notes
RELNOTES[NEW]: Toolchains using
use_target_platform_constraints=Truecan now specify additional execution and target constraints.