Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Running a cc_test should respect its toolchain's target_compatible_with #13626

Open
jlaxson opened this issue Jun 30, 2021 · 13 comments
Open

Running a cc_test should respect its toolchain's target_compatible_with #13626

jlaxson opened this issue Jun 30, 2021 · 13 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@jlaxson
Copy link
Contributor

jlaxson commented Jun 30, 2021

Description of the problem / feature request:

I'm not sure if this is a bug or a pebkac or a works as intended. In a cross compile environment, imagine I have two platforms:

platform(
  name = "target",
  constraint_values = ["@platforms//os:linux", "//:my_target_arch"],
  # properties that configure RBE
)
platform(
  name = "exec",
  constraint_values = ["@platforms//os:linux"],
)
toolchain(
  name = "my_toolchain",
  exec_compatible_with = ["@platforms//os:linux"],
  target_compatible_with = ["//:my_target_arch"]
)
toolchain(
  name = "host_clang_toolchain",
  exec_compatible_with = ["@platforms//os:linux"],
  target_compatible_with = ["@platforms//os:linux"]
)
cc_test(
  name = "my_test",
  srcs = ["mysrc.cpp"],
)

and run it as

bazel test //:my_test --platforms=//:target --extra_execution_platforms=//:exec,//:target --incompatible_enable_cc_toolchain_resolution --extra_toolchains=//:my_toolchain,//:host_clang_toolchain --remote_executor=grpc://blah

I see that the test gets built using the target toolchain (//:my_toolchain). But when bazel goes to run the test, it seems to pick the first execution platform //:exec, even though that platform doesn't have the //:my_target_arch constraint defined in the target platform.

I can solve this by adding an exec_compatible_with to the cc_test, but this doesn't work in all scenarios, and it's not really portable for configuring the build by using the --platforms

Feature requests: what underlying problem are you trying to solve with this feature?

I would like for the cc_test to be executed using the //:target platform (because that's the value of --platforms), or a platform in --extra_execution_platforms that matches the constraints in //:target

What operating system are you running Bazel on?

macOS

What's the output of bazel info release?

release 4.1.0

Have you found anything relevant by searching the web?

I'm pretty sure there's a line in the docs somewhere that says a cc_test is always built for the target platform, but I can't find it. It seems like cross-compiling would be more dysfunctional than it is if this were actually a bug, but not sure what I'm doing wrong.

Any other information, logs, or outputs that you want to share?

Will work on a standalone example.

@oquenchil oquenchil added team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request untriaged labels Jul 14, 2021
@oquenchil
Copy link
Contributor

@gregestren Hi Greg, I'm a bit lost in this area. Are constraints deprecated or they will be kept long term? Is this a feature request for the C++ rules team or configurability team? If the former, please re-assign.

@gregestren
Copy link
Contributor

@oquenchil - this is configurability related and speaks to modern APIs. You assigned right. :)

@jlaxson as far as I understand this works as intended. https://docs.bazel.build/versions/main/platforms.html#platforms has some definitions, but the basic summary as I understand is anything that runs on a build machine is considered to run on an execution platform. That includes compilers, but also generally includes tests.

I'm not sure if the --host_platform flag could help (set that to the same as --platforms? @katre can guide on these themes most definitively.

@jlaxson
Copy link
Contributor Author

jlaxson commented Jul 14, 2021

Hi @gregestren

basic summary as I understand is anything that runs on a build machine is considered to run on an execution platform. That includes compilers, but also generally includes tests.

Do you mean "...run on the same execution platform?". It seems to me though that there's a faulty assumption that any test (or I think cc_binary in general?) can run on the same execution platform as the compiler that generated it. It seems pretty reasonable that if I have a toolchain with exec_compatible_with = ["@platforms//os:linux"], target_compatible_with = ["@platforms//os:windows"], the product of that toolchain should not be assumed to execute on a platform defined only as["@platforms//os:linux"]?

@gregestren
Copy link
Contributor

I agree that tests, particularly, complicate our intuitions on what machines are doing what. It can be common for tests to run on different machines than the target code, for example tests emulating mobile devices. But your core point, about not hard-baking in inflexible expectations, is sound.

There's https://docs.bazel.build/versions/main/exec-groups.html#execution-groups-and-platform-execution-properties, as you mentioned earlier, and mentioned doesn't feel flexible enough.

What kind of API would you find most effective, short of hard-coding all tests to the target platform, which IMO also would suffer the same inflexibility?

@pcjanzen
Copy link
Contributor

related previous discussion: #12719 (comment)

@katre
Copy link
Member

katre commented Jul 19, 2021

I talk with @gregestren about this today, and we agree that it's slightly confusing, but we don't see a good way forward yet.

Currently, test rules have two exec groups:

  • The default exec group, which every rule has, and which is used to build the test artifacts
  • The test exec group, which is used specifically to execute the test artifacts.

In the future, we're like to provide more control over how the test exec group works, including specifying what its target platform is. Currently, the "target platform" is the platform where the test artifact is executed, and that ends up being the same exec platform that the rest of the target uses most times.

You can tweak that slightly now: rules can provide specific execution constraints only for the test exec group, and targets can specify custom execution properties for the test exec group (which might be used by your remote executor, if you have one, see https://docs.bazel.build/versions/4.1.0/exec-groups.html#using-execution-groups-to-set-execution-properties for details).

We don't currently have a way for a target to specify new execution constraints only for the test exec group. I imagine this would look like:

cc_test(
  name = "my_test",
  srcs = ["mysrc.cpp"],
  exec_compatible_with = {
    "test": [
      "@platforms//cpu:linux",
      ":my_custom_constraint",
    ],
  },
)

Is this something that would help solve your problem? We can take this as a feature request, although I am not sure when we'd be able to take action on it.

@jlaxson
Copy link
Contributor Author

jlaxson commented Jul 19, 2021

What kind of API would you find most effective, short of hard-coding all tests to the target platform, which IMO also would suffer the same inflexibility?

I think just one degree of freedom of being able to decide if test running defaults to the toolchain's exec platform or target platform will still lack a ton of flexibility, but would mostly address my concern, and provide enough for running on-device cross compiled tests.

In the future, we're like to provide more control over how the test exec group works, including specifying what its target platform is. Currently, the "target platform" is the platform where the test artifact is executed, and that ends up being the same exec platform that the rest of the target uses most times.

I think this also meets my needs provided it can be some global setting (--test_platform=?). I can't envision how I'd really leverage only per-target configurability for this need.

We don't currently have a way for a target to specify new execution constraints only for the test exec group. I imagine this would look like...

When you asked for suggestions, this is roughly where my head went. I think it's a bit preferable for use cases I can think of vs. just being able to override exec_properties directly. But this seems like it'll be very brittle on multi-platform workspaces given how exec_compatible_with is nonconfigurable.

Is this something that would help solve your problem? We can take this as a feature request, although I am not sure when we'd be able to take action on it.

With buy-in to a solution I'd take a crack at implementing it.

@gregestren
Copy link
Contributor

Is this something that would help solve your problem? We can take this as a feature request, although I am not sure when we'd be able to take action on it.

With buy-in to a solution I'd take a crack at implementing it.

+1 if you think we're on the path toward a coherent design.

@katre
Copy link
Member

katre commented Jul 21, 2021

That sounds great. To make sure we're all on the same page, I'd like you to go through the proposals process (detailed at https://github.com/bazelbuild/proposals). It's not particularly onerous, but it helps to ensure that we're all on the same page and agree on what needs to be added.

I'm happy to review both the proposal and the actual PRs involved, and generally to provide assistance and advice.

@gregestren gregestren added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Jul 22, 2021
@github-actions
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label May 10, 2023
@cameron-martin
Copy link
Contributor

This is not stale, since assigning execution constraints per exec group is still a valuable change.

Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Jul 14, 2024
@fmeum fmeum removed the stale Issues or PRs that are stale (no activity for 30 days) label Aug 7, 2024
@fmeum
Copy link
Collaborator

fmeum commented Aug 7, 2024

A powerful but verbose approach to influence the exec platform that works today is to register a custom @bazel_tools//tools/cpp:test_runner_toolchain_type toolchain that translates target constraints for tests into exec constraints on the platform it should run on. This is currently scarcely documented publicly, but (thanks to @trybka) there is a draft version of a write-up of this feature: https://github.com/trybka/scraps/blob/master/cc_test.md

Over at #23202 (comment), I proposed syntax for a way to add exec constraints to individual exec groups per target. @katre @gregestren What are your thoughts on that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Development

No branches or pull requests

8 participants
@jlaxson @katre @cameron-martin @fmeum @gregestren @oquenchil @pcjanzen and others