[Retry] Fix transitive framework import in opt build #718
Conversation
…aging.transitive_deps (#3) (bazel-ios#570)" (bazel-ios#715)" This reverts commit 5d32bd1.
qyang-nj
force-pushed
the
qing--retry-framework-aspect
branch
from
e368674
to
a85465c
Compare
qyang-nj
changed the title
| cc_info = objc_provider_utils.merge_cc_info_providers( | ||
| cc_info_providers = [dep[CcInfo] for dep in ctx.attr.deps if CcInfo in dep], | ||
| ) | ||
| additional_providers.append(cc_info) |
There was a problem hiding this comment.
This custom merging logic caused a problematic modulemap is generated. I'm not sure why we are merging CcInfo differently depending on whether VFS is used, but all CI jobs are passed.
There was a problem hiding this comment.
I'll be able to look at this change more in depth a bit later in the week or early next; a lot of times we can't rely on "green" CI status for most of this logic. There was some reasons to do this on Bazel 4; which I'm not sure exist still..
Is there a way that you'd add an example of how you're using a transitive import that wasn't working incase we need to fix that in a different way?
There was a problem hiding this comment.
We should be able to though right? If we can't rely on CI being 🟢 here then I think we should look into it and make CI start checking for whatever is necessary?
There was a problem hiding this comment.
SGTM! Yeah it'd be nice to improve the coverage for the things we can, I can dig more into this early next week.
|
Running an integration test in our repo now. Do you mind doing the same when you have time, @thiagohmcruz? |
| @@ -55,29 +55,9 @@ def _merge_dynamic_framework_providers(dynamic_framework_providers): | |||
|
|
|||
| return apple_common.new_dynamic_framework_provider(**fields) | |||
|
|
|||
| def _merge_cc_info_providers(cc_info_providers, merge_keys = [ | |||
There was a problem hiding this comment.
Actually this is not used anywhere else.
There was a problem hiding this comment.
Ok I just found out we use this internally and this removal broke things for us. Was it necessary to remove it to unblock your intended change? If not can we put it back for now while we discuss where this code should live?
Ideally we would add a test that exercises this then, maybe this shouldn't be an internal util if the intent is to load it on the consumer side.
cc @jerrymarino I see you added it originally, would you be able to add a test case that exercises this code path specifically?
There was a problem hiding this comment.
Overall this is "internal" but this overall utility module clearly has many useful functions none the less - we should clarify API contracts with this code.
|
We can merge this and revert if Block folks have any issues. Thanks for fixing the previous problem, @qyang-nj! |
| cc_info = objc_provider_utils.merge_cc_info_providers( | ||
| cc_info_providers = [dep[CcInfo] for dep in ctx.attr.deps if CcInfo in dep], | ||
| ) | ||
| additional_providers.append(cc_info) |
There was a problem hiding this comment.
I'll be able to look at this change more in depth a bit later in the week or early next; a lot of times we can't rely on "green" CI status for most of this logic. There was some reasons to do this on Bazel 4; which I'm not sure exist still..
Is there a way that you'd add an example of how you're using a transitive import that wasn't working incase we need to fix that in a different way?
Retry merging PR #570, which solves issue #569. The original PR got reverted because of issue #712.
Original description
apple_*_(xc)framework_importprovides_SwiftInteropInfoinstead ofSwiftInfo. Inapple_framework_packaging, when we merge the SwiftInfo, the info from the framework import get lost there.To fix the problem, we can add
swift_clang_module_aspectto thetransitive_depsattribute ofapple_framework_packaging, which converts_SwiftInteropInfotoSwiftInfo.swift_librarydoes this fordepsas well.@mattrobmattrob @thiagohmcruz