-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add support for apple platform command line options #816
Add support for apple platform command line options #816
Conversation
20c1a20
to
cdb95fd
Compare
A step for bazel 7 migration #795 |
cdb95fd
to
33055b5
Compare
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.
Thanks for getting this updated! Left some comments for maintainers to address in the future.
@karim-alweheshy I'll rebase and merge this once #835 lands. |
016fc08
to
11be6f5
Compare
b4c7f97
to
fbea2d5
Compare
f00c6f0
to
aff85fb
Compare
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.
I'll get PRs up to remove the nvm, we don't use those in main, can we just remove them from this PR or is there a reason they were added? From what I can tell rules_apple doesn't use theseapple_compiler
/gre_top
usage
aff85fb
to
58b67a1
Compare
41deaab
to
2c90bd4
Compare
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.
Thank you for iterating on this!
) | ||
for cpu, platform_name in CPU_TO_DEFAULT_PLATFORM_NAME.items() | ||
} | ||
|
||
def _current_apple_platform(apple_fragment, xcode_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.
This now matches rules_apple
, thank you!
rules/transition_support.bzl
Outdated
@@ -74,6 +102,19 @@ def _min_os_version_or_none(attr, attr_platforms, platform, attr_platform_type): | |||
|
|||
return None | |||
|
|||
def _objc_library_transition_values(settings, platform_type): |
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.
nit: im not sure the best name here but since this is related to the transition of apple platform moving to rules vs. defined in Bazel maybe something like this is more clear:
def _objc_library_transition_values(settings, platform_type): | |
def _apple_platforms_base_values(settings, platform_type): |
Alternatively we could try to match: https://github.com/bazelbuild/rules_apple/blob/master/apple/internal/transition_support.bzl#L440-L455
2c90bd4
to
bfdba6a
Compare
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.
Thank you!
Description
Default bazel's transition of native.objc_library was removed in bazel 7. Resulting in the target to be built for the wrong platform. This PR adds the missing transition's command line flags to enable the correct target compilation+linking for the objc_library targets. The transition's code is only added if the host is running on bazel 7 +
Resources
Remove objc_library transition
Bazel changes
rules_apple changes 1
rules_apple changes 2
rules_apple changes 3