bazelbuild / platforms

Constraint values for specifying platforms and toolchains
Apache License 2.0
108 stars 73 forks source link

Implement local_config_platform in @platforms #86

Closed Wyverald closed 6 months ago

Wyverald commented 7 months ago

Per design doc: https://docs.google.com/document/d/1g5JAAOfLsvQKBGqzSLFp1hIYFoQsgOslsjaIGV6P7Tk/edit

plan:

  1. Merge this PR and push a new release.
  2. Make bazel_tools depend on the new version of platforms.
  3. Make --host_platform default to @bazel_tools//:host_platform, which is an alias to @platforms//host.
    • This is potentially problematic. Do aliases work with platforms?
  4. Update LocalConfigPlatformFunction.java in Bazel source to make it produce a @local_config_platform that forwards everything here:
    • @local_config_platform//:host becomes an alias to @platforms//host.
    • @local_config_platform//:constraints.bzl simply re-exports the HOST_CONSTRAINTS list from @platforms//host:constraints.bzl.
  5. Add an incompatible flag that removes the built-in module local_config_platform.
  6. The above is cherry-pickable back to Bazel 7.x. We can start advertising to people to stop using @local_config_platform directly, and use the stuff in @platforms//host instead.
  7. Flip the incompatible flag in Bazel 8.0, with the intention to completely remove local_config_platform (both the repo rule and the built-in module) in Bazel 9.0.
  8. Implement the custom constraint injection stuff here as we see fit. (see https://github.com/fmeum/host_platform/tree/main)
  9. Profit??
Wyverald commented 7 months ago

cc @fmeum @katre @brentleyjones

@katre specifically, do aliases work with platforms? If they don't, the "canonical" host platform will need to be @bazel_tools//:host_platform instead of @platforms//host (because default flag values can only be stuff inside built-in modules like @bazel_tools (and @local_config_platform today)) and the migration path is a bit trickier (we'd have to ask people to actually stop using @local_config_platform//:host and switch to @bazel_tools//:host_platform instead).

katre commented 6 months ago

For future reference: context here is https://github.com/bazelbuild/bazel/issues/8766 and https://bazelbuild.slack.com/archives/CA31HN1T3/p1709673438139349

@Wyverald: There are reasons I have been asking for an actual design for this feature from the beginning of https://github.com/bazelbuild/bazel/issues/8766. We very deliberately have resisted putting complex logic in the @platforms repository, because it is foundational to so many other repos (including Bazel itself), and so anything that we add here has a lot of follow-on effects. I'm not opposed to it, but we need to plan for it.

I'd like to, once again, ask people to step back from the fun part of writing code and move to the less fun part of writing and discussing a design document.

Wyverald commented 6 months ago

Design doc: https://docs.google.com/document/d/1g5JAAOfLsvQKBGqzSLFp1hIYFoQsgOslsjaIGV6P7Tk/edit

Wyverald commented 6 months ago

The test I added is a bit ugly, but it works... except on Windows. Not sure what's going on there...

katre commented 6 months ago

Added @aranguyen as an FYI

katre commented 6 months ago

I don't think the sh_test can execute on windows, do our images have bash support anymore?

meteorcloudy commented 6 months ago

I don't think the sh_test can execute on windows, do our images have bash support anymore?

sh_test should work on Windows and we do have MSYS2 available on CI Windows VMs.

Believe it or not, the shell test was failing because the Windows launcher wasn't built for the shell test, and that's because the isExecutedOnWindows function doesn't work when the root module's name is platforms!

meteorcloudy commented 6 months ago

When the root module's name is not platforms

image

when it is platforms:

image
Wyverald commented 6 months ago

🤯 Thanks, Yun! This is yet another direct dependency on @platforms from Bazel itself, which is another hurdle to removing platforms as a "well-known module"...

aignas commented 6 months ago

I am trying to use this and I want to build docs with stardoc for a library that uses this. Any tips on how to construct the bzl_library? I get

$ bazel build //docs/sphinx/...
ERROR: /Users/ignas.anikevicius/src/github/aignas/rules_python/docs/sphinx/BUILD.bazel:75:16: in deps attribute of starlark_doc_extract rule //docs/sphinx:_bzl_api_docs_api_extensions_pip.md_stardoc: missing bzl_library targets for Starlark module(s) @@platforms~host_platform~host_platform//:constraints.bzl. Since this rule was created by the macro 'sphinx_stardocs', the error might have been caused by the macro implementation
ERROR: /Users/ignas.anikevicius/src/github/aignas/rules_python/docs/sphinx/BUILD.bazel:75:16: Analysis of target '//docs/sphinx:_bzl_api_docs_api_extensions_pip.md_stardoc' failed
Use --verbose_failures to see the command lines of failed build steps.
ERROR: Analysis of target '//docs/sphinx:_bzl_api_docs_api_extensions_pip.md' failed; build aborted: Analysis failed
INFO: Elapsed time: 0.220s, Critical Path: 0.00s
INFO: 0 processes.
ERROR: Build did NOT complete successfully

The code is in bazelbuild/rules_python#1827.

EDIT: opened #89 to fix the issue I'm having, we can continue discussion there if this is the right fix.