Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[quick_actions]remove custom modulemap and private headers #6229

Conversation

hellohuanlin
Copy link
Contributor

@hellohuanlin hellohuanlin commented Aug 11, 2022

I do not like this solution since it makes private headers public. However, I wasn't able to find a better solution after lots of research on this topic.

Below is a summary of what I found so far, in case it's useful searching for a better solution in the future:

  1. Cocoapods does not support a custom modulemap file for static libraries that contains Swift:
[!] Using Swift static libraries with custom module maps is currently not supported. Please build `quick_actions_ios` as a framework or remove the custom module map.

This limitation is the culprit of everything. If we could use a custom modulemap, we could easily define a submodule (aka we are currently doing) or use a private modulemap to map the private headers, like this.

  1. Adding use_frameworks! to the Podfile also works, but we can't force developers to choose frameworks and not static libraries.

  2. There seems to be some effort fixing this problem in Cocoapods (this and this), but there isn't much update since then.

  3. What about removing clang modules at all? In pure ObjC it's fine, but Swift cannot directly import headers - it can only interact with modules.

  4. The auto-generated default modulemap simply exports everything from the umbrella header. It defines a submodule for each of the header import, and re-export them to the main module. Only public headers are allowed in this process. Something like this:

module ModuleName {
  umbrella header "ModuleName-umbrella.h"
  export * 
  module * { export * }
}

This is only a temporary workaround during migration, but the bad state could last a long time for larger plugins. Though on the bright side, consider that private headers aren't that "private" at all - they can be imported by clients just like public headers. it's only that they are put under a separate directory named "PrivateHeaders" to warn the clients that it's risky to import them, but nothing can technically stop them from doing so.

List which issues are fixed by this PR. You must list at least one issue.

flutter/flutter#108750

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@hellohuanlin hellohuanlin marked this pull request as ready for review August 11, 2022 19:26
Comment on lines 24 to 25
'LIBRARY_SEARCH_PATHS' => '$(TOOLCHAIN_DIR)/usr/lib/swift/$(PLATFORM_NAME)/ $(SDKROOT)/usr/lib/swift',
'LD_RUNPATH_SEARCH_PATHS' => '/usr/lib/swift',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hit similar issues in flutter/flutter#16049 (comment). But I don't think you'll need this once you add your first Swift file to the framework, right? Instead of making this modulemap change now can you instead do it when you add the first Swift file so we don't need this frankly sketch workaround?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed swift related updates since we don't have swift files yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of making this modulemap changes a standalone PR can you instead include it in the PR that actually adds a Swift file? It doesn't seem worth a version bump, and I can't check it out and play with it with the Swift tests you write to see what else can be done instead.

@jmagman jmagman marked this pull request as draft August 24, 2022 21:02
@hellohuanlin
Copy link
Contributor Author

Since I can't change the source branch in GitHub in the same PR, I will close this PR in favor of a new one.

@jmagman
Copy link
Member

jmagman commented Sep 7, 2022

Since I can't change the source branch in GitHub in the same PR, I will close this PR in favor of a new one.

What branch were you trying to update to? You can rebase or whatever you need to in this PR.

@hellohuanlin
Copy link
Contributor Author

@jmagman Because the original branch name quick_actions_remove_custom_module_map is not accurate anymore. It seems it is not possible to update the source branch after creating a Pull Request.

@jmagman
Copy link
Member

jmagman commented Sep 7, 2022

Oh, the branch name changed on your fork.

@hellohuanlin
Copy link
Contributor Author

@jmagman Yep, i know it is a weird obsession to keep things organized

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants