-
Notifications
You must be signed in to change notification settings - Fork 332
Prepartation for Bazel 9: get xcode version from @bazel_tools//tools/cpp:host_xcodes #8003
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
Prepartation for Bazel 9: get xcode version from @bazel_tools//tools/cpp:host_xcodes #8003
Conversation
|
Preparation for: #8004 |
| "#!/bin/bash", | ||
| "__BAZEL_BIN__ cquery \\", | ||
| " 'deps(\"@bazel_tools//tools/osx:current_xcode_config\")' \\", | ||
| " 'deps(\"@bazel_tools//tools/cpp:host_xcodes\")' \\", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, but I have a feeling that item on the left has current config, while rhs lists all
https://bazel.build/reference/command-line-reference#flag--xcode_version_config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So apparently host_xcodes is the default value for --xcode_version_config: https://github.com/bazelbuild/bazel/blob/f57b39513a30d0ebd46e8888b2445313b3198de1/tools/cpp/BUILD.tools#L125-L128
Where as current_xcode_config was the current value of --xcode_version_config: https://github.com/bazelbuild/bazel/blob/3b1ed466d08107049dc1d232f43a7018997a2505/src/main/starlark/builtins_bzl/common/xcode/xcode_config_alias.bzl
But this rule has been removed in bazelbuild/bazel@873b036 and is no longer in the builtins. Idk where it moved to.
As shown in the new aspect, it is possible to move the xcode version collection to the aspect finally. I could port this code and we could use this for Bazel 8+.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need some short term solution here as bazel 9 release is kinda close and full migration to the new aspect is a bit risky? Or is it ok for us to depend on aspect change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I'm a bit puzzled about the real need in this code. We have it to set DEVELOPER_DIR and SDKROOT for compiler script, but we never had such a need in CMake support. Is it so that we rely on default xcode selection, while bazel can override it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this code is incorrectly placed, this has to be somewhat related closer to toolchain info rather than just "random" Xcode-related target query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. By porting I meant only taking the code I wrote for xcode info collection and adding it to the old aspect. If this is possible.
|
As agreed, we'll proceed as follows. Only use the With the new aspect we'll receive the correct xcode version from the aspect and can deprecate this entire cquery setup. |
086f413 to
28acd21
Compare
The previous target (@bazel_tools//tools/osx:current_xcode_config) which we used in the cquery no longer exists in Bazel 9. However, the actual target which provides the version (@bazel_tools//tools/cpp:host_xcodes) exists in Bazel 9 and all currently supported versions (6-7).