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.

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 use of modulemaps in flutter/packages when using SwiftPM #148572

Open
Tracked by #126005
vashworth opened this issue May 17, 2024 · 4 comments
Open
Tracked by #126005

Remove use of modulemaps in flutter/packages when using SwiftPM #148572

vashworth opened this issue May 17, 2024 · 4 comments
Labels
P2 Important issues not at the top of the work list platform-ios iOS applications specifically platform-mac Building on or for macOS specifically team-ios Owned by iOS platform team triaged-ios Triaged by iOS platform team

Comments

@vashworth
Copy link
Contributor

vashworth commented May 17, 2024

Use case

To support both CocoaPods and Swift Package Manager, dynamic and static linking, two different modulemaps are needed.

CocoaPods modulemap

CocoaPods seems to require the modulemap be a framework module and doesn't use relative paths.

For example,

framework module plugin_name_ios {
  umbrella header "plugin_name_ios-umbrella.h"

  export *
  module * { export * }

  explicit module Test {
    header "PublicHeader_Test.h"
    header "OtherPublicHeader_Test.h"
  }
}

SwiftPM modulemap

SwiftPM requires the modulemap to be named module.modulemap and be located within the include directory. It also doesn't seem to work if defined as a framework module and uses relative paths.

For example,

- framework module plugin_name_ios {
+ module plugin_name_ios {

explicit module Test {
-    header "PublicHeader_Test.h"
+    header "plugin_name_ios/OtherPublicHeader_Test.h"

The problem

The problem is that when you have two modulemaps (most likely also because one is named module.modulemap, which is the implicit modulemap file), this can cause issues:

Proposal

It's been pretty difficult to figure out a way to support both CocoaPods and SwiftPM modulemaps. As a result, we're leaning towards eliminating modulemaps in SwiftPM.


Instead of using a modulemap and umbrella header for the SwiftPM integration of plugins, we simply allow all public headers to be available via @import plugin_name_ios.

@import plugin_name_ios;
- @import plugin_name_ios.Test;

The headers are already public and could be imported individually (like #import <plugin_name_ios/PublicHeader_Test.h>), so making them available through the module isn't much worse.

Modulemap and umbrella header should be excluded from SwiftPM target:

        .target(
            name: "plugin_name_ios",
            dependencies: [],
+           exclude: ["cocoapods_plugin_name_ios.modulemap", "plugin_name_ios-umbrella.h"],

Unit tests can also be kept compatible with both CocoaPods and SwiftPM with something like:

#if __has_include(<plugin_name_ios/plugin_name_ios-umbrella.h>)
  @import plugin_name_ios.Test;
#endif

Prototype

flutter/packages@main...vashworth:packages:camera_avfoundation_remove_swiftpm_modulemap

Re-adding SwiftPM .modulemaps

Once the ecosystem has migrated to SwiftPM, we will remove support for CocoaPods. At this point, we should consider re-adding module.modulemaps to the plugins' SwiftPM integration. This step might not be necessary as we plan to rewrite plugins in Swift.

@vashworth vashworth added P1 High-priority issues at the top of the work list team-ios Owned by iOS platform team triaged-ios Triaged by iOS platform team platform-ios iOS applications specifically platform-mac Building on or for macOS specifically labels May 17, 2024
@vashworth
Copy link
Contributor Author

fyi @jmagman @stuartmorgan

@loic-sharma and I like this approach and are planning to move forward with it. If users want to use modulemaps with SwiftPM, we plan to point them to SwiftPM's documentation about it: https://github.com/apple/swift-package-manager/blob/main/Documentation/Usage.md#creating-c-language-targets

@vashworth vashworth added P2 Important issues not at the top of the work list and removed P1 High-priority issues at the top of the work list labels May 17, 2024
@loic-sharma
Copy link
Member

loic-sharma commented May 17, 2024

If users want to use modulemaps with SwiftPM, we plan to point them to SwiftPM's documentation about it: https://github.com/apple/swift-package-manager/blob/main/Documentation/Usage.md#creating-c-language-targets

I created an experiment that allows for custom module maps with SwiftPM: flutter/packages@main...loic-sharma:flutter-packages:spm_camera_separate_public_file_symlinks

This works by creating separate header directories for CocoaPods (/cocoapods_headers) and Swift Package Manager (/include). To minimize duplication, the CocoaPods header files are symlinks back to the header files in the Swift Package Manager header directory. I verified this works on Xcode 14.2+. Some drawbacks to this approach:

  1. Pigeon generates implementation files that use relative #import "..." paths. This is incompatible with this experiment as the relative path to the header files changes depending on whether you're using CocoaPods or Swift Package Manager. Instead, the Pigeon implementation file should use non-relative imports like #import <my_plugin/MyHeader.h>. Today, this requires manually editing the code generated by Pigeon.
  2. You need to remember to update symlinks if you add/move/delete header files. If we were to use this approach in the flutter/packages repo, we'd likely want tooling/tests to verify CocoaPods/SwiftPM header files are in sync.

Due to these drawbacks we prefer to avoid module maps in SwiftPM if possible.

@jmagman
Copy link
Member

jmagman commented May 17, 2024

In the tests:

@import camera_avfoundation;
@import camera_avfoundation_test;

If we were only in SwiftPM world, could we normally #import the test headers directly? Or was there some problem with the hmap?

@vashworth
Copy link
Contributor Author

vashworth commented May 20, 2024

In the tests:

@import camera_avfoundation;
@import camera_avfoundation_test;

If we were only in SwiftPM world, could we normally #import the test headers directly? Or was there some problem with the hmap?

We could do imports directly.

That actually makes me realize, we can simplify this even further. Since all the headers are public and we're not using a custom umbrella header and modulemap, we don't need to do a separate target or import separate headers. All the headers should be available via the @import camera_avfoundation; since it's no longer limited by the umbrella header. I updated the Proposal and prototype

auto-submit bot pushed a commit to flutter/packages that referenced this issue May 31, 2024
Adds Swift Package Manager compatibility to `image_picker_ios`.

The previous attempt, #6617, was partially reverted due to flutter/flutter#148307. This reland uses the new approach proposed in flutter/flutter#148572: the Swift Package Manager package does not have a `.modulemap`. As a result, the plugin no longer has a `.Test` submodule if using Swift Package Manager.

Fixes flutter/flutter#146919.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Important issues not at the top of the work list platform-ios iOS applications specifically platform-mac Building on or for macOS specifically team-ios Owned by iOS platform team triaged-ios Triaged by iOS platform team
Projects
None yet
Development

No branches or pull requests

3 participants