Add ability to package an iOS dynamic framework into a format Xcode can consume #928
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
…nd cleaned up some unneeded code
|
@mccorkill1 Hi! We can't really look at this until a CLA is signed. that said, I would like a rule that produces .frameworks for Xcode consumption (we have a SwiftUI integration that requires it).
Personally it would feel odd to me that it only supported some platforms. |
|
@brentleyjones I am working on getting added as an authorized user on my company's corporate account. I will work on adding support for the other two platforms in the mean time. Thank you for the feedback! |
…now be used as a dependency of a swift_library and have the transitives packaged properly. With that in mind, there is no more need to use ios_dynamic_framework as a dependency of a swift_library. It can solely be used to bundle a compiled swift_library into a framework usable by Xcode.
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
Thanks @mccorkill1! I'll take a look at this soon. |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
@googlebot I fixed it. |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
@brentleyjones So I've been working on getting the macos_dynamic_framework rule into shape and here's where I'm at: I've got most of it packaging properly, but I ran into a few snags.
I've pushed my work in progress code to this branch on my fork if you want to take a look: https://github.com/mccorkill1/rules_apple/tree/macos_dynamic_framework I think the best way forward is to remove the support for the macos_dynamic_framework rule from this PR and include it in a separate PR once these issues can be resolved. What do you think? |
Did you hit an issue where this is required for something? I doubt it should be, especially for modules compiled with library evolution (which I guess these may or may not be) |
I agree. Let's pull out macOS for now and make a new PR with your progress. |
My example from Xcode had it. I'll second @keith here, and say it's not needed, but once it is generated via bazelbuild/rules_swift#523 we should add it into these rules. |
…moved off SwiftInfo.transitive_swift* in the swift_dynamic_framework_aspect
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Thanks @mccorkill1 for putting in all the work on this!
@keith @segiddins: Can I get a second pair of eyes on this before we merge it? Thanks!
|
Thank you @brentleyjones for being so responsive and reviewing the PR! I'm really happy with how the code turned out and I'm excited to get it merged in! |
|
|
||
| return [file for file in framework_imports if file.short_path.endswith(".h")] | ||
|
|
||
| def _swift_dynamic_framework_partial_impl(ctx, swift_dynamic_framework_info): |
There was a problem hiding this comment.
i think we're trying to move away from partials taking ctx as a param?
There was a problem hiding this comment.
I matched the format of the other partials and am no longer passing in ctx as a param to the rule when I call it. I added the same TODO that I saw in other partials to Remove ctx from the args when ctx is removed from all partials
When I removed it as a parameter I got Error: _swift_dynamic_framework_partial_impl() got unexpected keyword argument: ctx even though I am no longer manually passing in ctx.
…k partial and updated how the binary_target is selected so it not dependent on the order of the deps_list
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
… when it is removed from all partials and fixed a comment
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@keith @segiddins Can you take a look this? I think it's about ready to be merged in, but I need at least one more approval. |
|
need to make sure to squash and merge this one |
| def _get_header_imports(framework_imports): | ||
| """Get the header files from the list of framework imports""" | ||
|
|
||
| return [file for file in framework_imports if file.short_path.endswith(".h")] |
There was a problem hiding this comment.
the extensions here should match what objc_library or cc_library allow, but this can be addressed in a follow-up
|
Thanks again @mccorkill1! |
Background:
This scope of work was to develop a rule that can be used with Bazel that will output a dynamic framework which can be imported and used in Xcode. The existing ios_framework rule did not meet this requirement because it doesn’t include the headers, modulemap, or swiftmodule, and swiftdoc files bundled into the packaged .framework file. Therefore, we wrote the ios_dynamic_framework and watchos_dynamic_framework rules that output a .framework file with the compiled dynamic framework, Info.plist, Headers folder with the generated headers file, and Modules folder with the swiftmodule and modulemap files.
Approach:
Copy the ios_framework rule - it already did the packaging of the .framework file and the compiling of the framework
Copy the swift_static_framework_aspect - This aspect generates the swiftdoc, headers, and modulemap files, but it has limitations because static frameworks can’t have transitive dependencies. We removed the if statements that enforced that there be no transitive dependencies, used the SwiftInfo provider to get the relevant files for the transitive dependencies, including the swiftmodule file, and returned the references to the files in a new provider called SwiftDynamicFrameworkInfo.
Copy swift_static_framework partial - this partial uses the files returned by the SwiftDynamicFrameworkInfo provider and bundles them into the packaged .framework file. It also returns an incomplete CcInfo provider which was needed for the transitive dependencies to work.
Generate an objc_provider - In order for the ios_dynamic_framework rule to be used as a dependency of swift_library, it needed to pass an objc_provider that has a reference to the dynamic_framework_file. The dynamic_framework_file is is processed in the framework_provider_partial and returned in the AppleDynamicFramework provider. So after the partials are processed, the objc_provider is made and added to the list of returned providers. The _ios_dynamic_framework_impl felt like the wrong place to do this logic, but it's the only place we had access to all the data
Outstanding questions and callouts:
We added the swift_dynamic_framework_aspect to all product_type == framework. This shouldn’t impact anything since the aspect only returns additional information, but wanted to confirm this is the approach was acceptable.
We built this rule for ios and watchos. Are tvos and macos a priority for this use case? If so, can they go in a separate PR?
Edit: We added support for tvos and macos
We added framework as a supported type for watchos in rule_factory.bzl and rule_support.bzl
Currently the rule only supports swift_library dependencies, and those libraries can only have swift_library dependencies. There is an issue with swift_libraries that have objc_library dependencies that we are aware is in the backlog to add support for.