-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Fix objc transition for cc_binary -- simplifying change #19236
Conversation
…tic archives are in bin
if platform_type == "macos": | ||
return "darwin_{}".format(apple_split_cpu) | ||
return "{}_{}".format(platform_type, apple_split_cpu) | ||
return settings["//command_line_option:cpu"] |
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.
erm wouldn't removing all the other platforms here mean you couldn't build an objc library directly for iOS anymore? I feel like this change would likely be about the same as removing the transition all together?
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.
(Probably best to look at #19235 (comment) first, just to make sure we're on the same page. Thanks for thinking about it with me!)
To clarify: Not removing all the others; instead unifying a separate condition. (Note that despite how the diff preview decides to align, I've moved this code down from above, not replaced all the platform-specific cases with that macOS if statement. It's all the platform-specific, non-fat-binary-because-no-rules_apple-binary fallback cases that are unified into the last line, looking at CPU. I realize I still need to make the case to you that that's the right, consistent analysis, hence attempting to order the other comment first.)
To build an objc_library directly for iOS (rather than the default, Mac) one would now just specify --cpu (as elsewhere). Previously, (I think?) one couldn't build an objc_library directly for iOS with public APIs, since --apple_platform_type is undocumented (and explicitly labeled as not for direct use), and you'd have to specify it to hit the iOS case when building objc_library directly.
A quick cut at a change that would fix #19204.
An alternative to #19235 that attempts to simplify.
Please see #19204 for context.