Skip to content

Implement local_config_platform in @platforms #86

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

Merged
merged 9 commits into from
Mar 21, 2024
Merged

Implement local_config_platform in @platforms #86

merged 9 commits into from
Mar 21, 2024

Conversation

Wyverald
Copy link
Member

@Wyverald Wyverald commented Mar 8, 2024

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 added 4 commits March 8, 2024 00:10

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@Wyverald
Copy link
Member Author

Wyverald commented Mar 8, 2024

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
Copy link
Member

katre commented Mar 11, 2024

For future reference: context here is bazelbuild/bazel#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 bazelbuild/bazel#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
Copy link
Member Author

@Wyverald Wyverald changed the title POC: implement local_config_platform in @platforms Implement local_config_platform in @platforms Mar 20, 2024
@Wyverald Wyverald marked this pull request as ready for review March 20, 2024 22:26
@Wyverald Wyverald requested review from katre and meteorcloudy March 20, 2024 22:26
@Wyverald
Copy link
Member Author

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

@katre katre requested a review from aranguyen March 21, 2024 10:13
@katre
Copy link
Member

katre commented Mar 21, 2024

Added @aranguyen as an FYI

@katre
Copy link
Member

katre commented Mar 21, 2024

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

@meteorcloudy
Copy link
Member

meteorcloudy commented Mar 21, 2024

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
Copy link
Member

When the root module's name is not platforms

image

when it is platforms:

image

@Wyverald
Copy link
Member Author

🤯 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"...

@katre katre merged commit 2af915c into main Mar 21, 2024
2 checks passed
@katre katre deleted the wyv-host branch March 21, 2024 17:06
anakinxc referenced this pull request in secretflow/spu Mar 26, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [platforms](https://togithub.com/bazelbuild/platforms) | http_archive
| patch | `0.0.8` -> `0.0.9` |

---

### Release Notes

<details>
<summary>bazelbuild/platforms (platforms)</summary>

###
[`v0.0.9`](https://togithub.com/bazelbuild/platforms/releases/tag/0.0.9)

[Compare
Source](https://togithub.com/bazelbuild/platforms/compare/0.0.8...0.0.9)

#### What's Changed

- Define constraint for emscripten os by
[@&#8203;googlewalt](https://togithub.com/googlewalt) in
[https://github.com/bazelbuild/platforms/pull/84](https://togithub.com/bazelbuild/platforms/pull/84)
- Implement local_config_platform in `@platforms` by
[@&#8203;Wyverald](https://togithub.com/Wyverald) in
[https://github.com/bazelbuild/platforms/pull/86](https://togithub.com/bazelbuild/platforms/pull/86)

#### WORKSPACE setup

```bzl
load("@&#8203;bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
    name = "platforms",
    urls = [
        "https://mirror.bazel.build/github.com/bazelbuild/platforms/releases/download/0.0.9/platforms-0.0.9.tar.gz",
        "https://github.com/bazelbuild/platforms/releases/download/0.0.9/platforms-0.0.9.tar.gz",
    ],
    sha256 = "5eda539c841265031c2f82d8ae7a3a6490bd62176e0c038fc469eabf91f6149b",
)

### To use the new Starlark host platform in @&#8203;platforms, also include the following snippet:
load("@&#8203;platforms//host:extension.bzl", "host_platform_repo")
host_platform_repo(name = "host_platform")
```

**Full Changelog**:
bazelbuild/platforms@0.0.8...0.0.9

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/secretflow/spu).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjI2MS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@aignas
Copy link

aignas commented Mar 29, 2024

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 bazel-contrib/rules_python#1827.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants