-
Notifications
You must be signed in to change notification settings - Fork 85
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 invalid product type #744
Conversation
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.
At a high level LGTM, only one concern. Are we confident this is not changing behaviour by potentially bundling things with different names in some scenarios that might not be covered on CI?
Specifically these values now map from
bundle_name, bundle_extension and executable_name <= bundling_support internal logic
to
bundle_name <= ctx.attr.framework_name
bundle_extension <= ".framework"
executable_name <= bundle_name
How to we know these names match before and after?
@@ -582,7 +582,7 @@ def _merge_root_infoplists(ctx): | |||
current_apple_platform = transition_support.current_apple_platform(apple_fragment = ctx.fragments.apple, xcode_config = ctx.attr._xcode_config) | |||
platform_type = str(current_apple_platform.platform.platform_type) | |||
apple_mac_toolchain_info = ctx.attr._toolchain[AppleMacToolsToolchainInfo] | |||
rule_descriptor = rule_support.rule_descriptor(ctx) | |||
rule_descriptor = rule_support.rule_descriptor_no_ctx(platform_type, apple_product_type.static_framework) |
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.
Right, in my other PR I could've passed the attribute we need directly like this 🤦, nice one
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.
NP @thiagohmcruz it just seemed like a small thing to fix up and make a bit easier
It'd be a decent amount of studying the code in general, but this is a lot more explicit and removes possible values for it which is a beneficial part of it too. |
It was previously hardcoded as as static library and led to confusion even though it was mainly unused at the time. Fixes #742
bdd8d7c
to
9523862
Compare
It was previously hardcoded as as static library and led to confusion even though it was mainly unused at the time.
Fixes #742