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

Remove bazel cpu constraints usage #1797

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

keith
Copy link
Contributor

@keith keith commented Dec 20, 2024

This fixes a warning in an upcoming version of bazel. This use case
seems to solvable by using platform constraints directly. Fixes:

WARNING: @@abseil-cpp+//absl/random/internal:cpu_aarch64: select() on cpu is deprecated. Use platform constraints instead: https://bazel.build/docs/configurable-attributes#platforms.

Comment on lines 47 to 48
"@platforms//os:windows": ABSL_RANDOM_HWAES_MSVC_X64_FLAGS,
"@platforms//cpu:x86_64": ABSL_RANDOM_HWAES_X64_FLAGS,
Copy link
Member

@derekmauro derekmauro Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my summary of this pull request is "thanks for trying, I don't think this works, but it does let us know we have a problem."

The problem is that @platforms//os:windows and @platforms//cpu:x86_64 can both be true. The options controlled by this are really about the compiler (not the OS), and the CPU.

I'm actually thinking that this logic should not be in this file at all. Since for this code we use runtime dispatch when we are not sure at compile time if the CPU supports certain instructions, I'm thinking that using [[target(foo)]] in the source code might be a better solution.

cc: @laramiel

Copy link
Contributor Author

@keith keith Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that @platforms//os:windows and @platforms//cpu:x86_64 can both be true.

Yea I was hoping CI could prove this out but I can't see the logs here. I've pushed a solution to that which uses config_setting_group + match_all to disambiguate (I don't have access to a windows machine to verify that it works)

This fixes a warning in an upcoming version of bazel. This use case
seems to solvable by using platform constraints directly. Fixes:

```
WARNING: @@abseil-cpp+//absl/random/internal:cpu_aarch64: select() on cpu is deprecated. Use platform constraints instead: https://bazel.build/docs/configurable-attributes#platforms.
```
@keith keith force-pushed the ks/remove-bazel-cpu-constraints-usage branch from c2c18fb to 37393c0 Compare December 23, 2024 18:25
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.

3 participants