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.

Sign up for GitHub

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

Jump to bottom

Add support for apple platform command line options #816

Merged
merged 6 commits into from
Mar 4, 2024
Merged

Add support for apple platform command line options #816

merged 6 commits into from
Mar 4, 2024

Conversation

karim-alweheshy
Copy link
Contributor

@karim-alweheshy karim-alweheshy commented Dec 25, 2023 •
edited
Loading

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

@karim-alweheshy karim-alweheshy mentioned this pull request Dec 25, 2023
Closed
@karim-alweheshy karim-alweheshy force-pushed the karim/fix-objc_library-toolchain-resolution-transition branch 8 times, most recently from 20c1a20 to cdb95fd Compare December 28, 2023 13:18
@karim-alweheshy karim-alweheshy marked this pull request as ready for review December 28, 2023 13:19
@karim-alweheshy
Copy link
Contributor Author

A step for bazel 7 migration #795

@karim-alweheshy karim-alweheshy force-pushed the karim/fix-objc_library-toolchain-resolution-transition branch from cdb95fd to 33055b5 Compare December 28, 2023 13:31
@mattrobmattrob mattrobmattrob changed the title Karim/fix objc library toolchain resolution transition Fix ObjC library toolchain resolution transition Dec 28, 2023
luispadron
luispadron approved these changes Feb 5, 2024
View reviewed changes
Copy link
Collaborator

@luispadron luispadron left a 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.

rules/transition_support.bzl Outdated Show resolved Hide resolved
@luispadron
Copy link
Collaborator

@karim-alweheshy I'll rebase and merge this once #835 lands.

@karim-alweheshy karim-alweheshy force-pushed the karim/fix-objc_library-toolchain-resolution-transition branch 3 times, most recently from 016fc08 to 11be6f5 Compare March 1, 2024 00:21
luispadron
luispadron reviewed Mar 1, 2024
View reviewed changes
MODULE.bazel Outdated Show resolved Hide resolved
@luispadron luispadron requested review from luispadron and removed request for jerrymarino March 1, 2024 00:42
@karim-alweheshy karim-alweheshy force-pushed the karim/fix-objc_library-toolchain-resolution-transition branch from b4c7f97 to fbea2d5 Compare March 1, 2024 11:57
@karim-alweheshy karim-alweheshy force-pushed the karim/fix-objc_library-toolchain-resolution-transition branch 2 times, most recently from f00c6f0 to aff85fb Compare March 3, 2024 13:27
luispadron
luispadron reviewed Mar 3, 2024
View reviewed changes
Copy link
Collaborator

@luispadron luispadron left a comment •
edited
Loading

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 apple_compiler/gre_top usage 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 these

@karim-alweheshy karim-alweheshy force-pushed the karim/fix-objc_library-toolchain-resolution-transition branch from aff85fb to 58b67a1 Compare March 3, 2024 18:23
@luispadron luispadron self-requested a review March 3, 2024 18:26
@karim-alweheshy karim-alweheshy force-pushed the karim/fix-objc_library-toolchain-resolution-transition branch 3 times, most recently from 41deaab to 2c90bd4 Compare March 3, 2024 23:57
luispadron
luispadron approved these changes Mar 4, 2024
View reviewed changes
Copy link
Collaborator

@luispadron luispadron left a 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!

rules/transition_support.bzl
)
for cpu, platform_name in CPU_TO_DEFAULT_PLATFORM_NAME.items()
}

def _current_apple_platform(apple_fragment, xcode_config):
Copy link
Collaborator

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):
Copy link
Collaborator

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:

Suggested change
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

@luispadron luispadron changed the title Fix ObjC library toolchain resolution transition Add support for apple platform command line options Mar 4, 2024
@karim-alweheshy karim-alweheshy force-pushed the karim/fix-objc_library-toolchain-resolution-transition branch from 2c90bd4 to bfdba6a Compare March 4, 2024 11:25
jszumski
jszumski approved these changes Mar 4, 2024
View reviewed changes
Copy link
Collaborator

@jszumski jszumski left a comment

Choose a reason for hiding this comment

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

Thank you!

@luispadron luispadron merged commit 349ca62 into bazel-ios:master Mar 4, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jszumski jszumski jszumski approved these changes

@luispadron luispadron luispadron approved these changes

@thiagohmcruz thiagohmcruz Awaiting requested review from thiagohmcruz

@mattrobmattrob mattrobmattrob Awaiting requested review from mattrobmattrob

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@karim-alweheshy @luispadron @jszumski @thiagohmcruz