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

Allow library podspec to declare Swift Package Manager dependencies #44627

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mfazekas
Copy link

@mfazekas mfazekas commented May 20, 2024

Summary:

React-Native uses Cocapods for native dependency management on iOS. While CocoaPods is flexible and popular, Apple's Swift Package Manager is the new standard. Currently consuming packages available only via Swift Package Manager is not possible. This change implements a single extension so .podspec files can declare Swift Package Manager dependencies via

ReactNativePodsUtils.spm_dependency(s,  
     url: 'https://github.com/apple/swift-atomics.git', 
     requirement: {kind: 'upToNextMajorVersion', minimumVersion: '1.1.0'}, 
     products: ['Atomics'] 
   ) 

Changelog:

[IOS] [ADDED] - libraries can now declare Swift Package Manager dependencies in their .podspec with ReactNativePodsUtils.spm_dependency

Test Plan:

https://github.com/mfazekas/rn-spm-rfc-poc/

Is a simple demo for the feature:

  1. Podspec declare dependency with:

    if const_defined?(:ReactNativePodsUtils) && ReactNativePodsUtils.respond_to?(:spm_dependency) 
      ReactNativePodsUtils.spm_dependency(s,  
        url: 'https://github.com/apple/swift-atomics.git', 
        requirement: {kind: 'upToNextMajorVersion', minimumVersion: '1.1.0'}, 
        products: ['Atomics'] 
      ) 
    else 
      raise "Please upgrade React Native to >=0.75.0 to use SPM dependencies." 
    end 
  2. import Atomics and ManagedAtomic is used in the code

3.) spm_dependency causes the dependency to be added via post_install hook in the workspace

image

4.) spm_dependecy causes the library to be linked with Atomics library

image

Limitations:
1.) only works USE_FRAMEWORKS=dynamic pod install otherwise the linker fails with known Xcode issue - duplicate link issue
2.) .xcworkspace needs to be reopened after pod install - this could be worked around by not removing/readding spm dependencies

See also:

react-native-community/discussions-and-proposals#587 (comment)
react-native-community/discussions-and-proposals#787

@facebook-github-bot
Copy link
Contributor

Hi @mfazekas!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels May 20, 2024
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Thanks for the amazing PR. 👏
I left some comments in the code to simplify it a little bit and to make it easier to use.

I also have a couple of questions:

1.) only works USE_FRAMEWORKS=dynamic pod install

Does it works with static frameworks?

2.) .xcworkspace needs to be reopened after pod install - this could be worked around by not removing/readding spm dependencies

Do you think there is a way to detect whether dependencies changed so that we can emit a red message to prompt the user to close and reopen the .xcworkspace project?
Alternatively, we can kill Xcode, but if a person has multiple project open, that could be annoying.

packages/react-native/scripts/cocoapods/spm.rb Outdated Show resolved Hide resolved
packages/react-native/scripts/cocoapods/spm.rb Outdated Show resolved Hide resolved
packages/react-native/scripts/cocoapods/utils.rb Outdated Show resolved Hide resolved
Comment on lines 206 to 208
def self.spm_dependency(s, url:, requirement:, products:)
SPM.dependency(s, url: url, requirement: requirement, products: products)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

To simplify the usage for 3rd party dependencies, I'd properly move this function from the utils.rb directly to the react_native_pods.
In this way, instead of the check:

if const_defined?(:ReactNativePodsUtils) && ReactNativePodsUtils.respond_to?(:spm_dependency) 
  ReactNativePodsUtils.spm_dependency(s,  
    url: 'https://github.com/apple/swift-atomics.git', 
    requirement: {kind: 'upToNextMajorVersion', minimumVersion: '1.1.0'}, 
    products: ['Atomics'] 
  ) 
else 
  raise "Please upgrade React Native to >=0.75.0 to use SPM dependencies." 
end 

they can just:

if defined?(:spm_dependency)
  spm_dependency(s,  
    url: 'https://github.com/apple/swift-atomics.git', 
    requirement: {kind: 'upToNextMajorVersion', minimumVersion: '1.1.0'}, 
    products: ['Atomics'] 
  )
else 
  raise "Please upgrade React Native to >=0.75.0 to use SPM dependencies." 
end 

I would also consider backporting this to all the RN versions in the supported window (right now: 0.72, 0.73, 0.74) so that they don't even need to if-else.

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Added as spm_dependency.

As noted #44627 (comment) this approach does depend on what linking (static/dynamic) the RN framework uses and what linking does the SPM product uses. This is something to consider with backporting, as it might require some further changes to be usefull in all scenarios

@mfazekas
Copy link
Author

mfazekas commented May 21, 2024

Thanks for the amazing PR. 👏 I left some comments in the code to simplify it a little bit and to make it easier to use.

I also have a couple of questions:

1.) only works USE_FRAMEWORKS=dynamic pod install

Does it works with static frameworks?

It's a bit complicated. I've tested with Alamofire Swift Package Manager package that has a dynamic and static product.

Alamofire - static AlamofireDynamic
USE_FRAMEWORKS=dynamic pod install works works
USE_FRAMEWORKS=static pod install duplicate symbols undefined symbol*
pod install duplicate symbols undefined symbol*

Now undefined symbols can be fixed by adding AlmofireDynamic to the user project. But modifying the user project is less than ideal as unlike Pods project which get regenerated with each pod install, the user project is not regenerated.

2.) .xcworkspace needs to be reopened after pod install - this could be worked around by not removing/readding spm dependencies

Do you think there is a way to detect whether dependencies changed so that we can emit a red message to prompt the user to close and reopen the .xcworkspace project? Alternatively, we can kill Xcode, but if a person has multiple project open, that could be annoying.

According to testing in Xcode 15.4, if there is a single swift package manager dependency, the regenerating the project might loose the dependency in the UI. (Not 100% reproducible)

spm-reload.mov

We can write an osascript that reloads the project if it's open, but it's a bit hacky

#!/usr/bin/osascript -l JavaScript

function main() {
  const app = Application("Xcode")
  const active = app.activeWorkspaceDocument();

  console.log("Active", app.activeWorkspaceDocument().file());

  const worspacePath = "/Users/boga/Work/OSS/react-native-sim/rn-spm-rfc-poc/example/ios/RnSpmRfcPocExample.xcworkspace"

  if (app.activeWorkspaceDocument().file() == filePath) {
	  app.activeWorkspaceDocument().close();
	  app.open(filePath);
  } 
}

main()

@cipolleschi
Copy link
Contributor

Sorry for the late reply. I'm still catching up after the conferences of the past weeks.

One thing we can add is a big warning message that runs only if SPM dependencies are found.
The message can

  • ask the user to close and reopen the XCWorkspace
  • ask the user to run with dynamic frameworks (if it has not done that)

What do you think?

@mfazekas
Copy link
Author

mfazekas commented Jun 12, 2024

One thing we can add is a big warning message that runs only if SPM dependencies are found. The message can

  • ask the user to close and reopen the XCWorkspace

I've tested this with new Xcode 16.0 beta and close/reopen issue is solved there. So likely some warning that

If you're using Xcode 15 or earlier you might need to reload the Xcode workspace

  • ask the user to run with dynamic frameworks (if it has not done that)

The SPM might be a simple Swift Macro and in that case it works with all linking. If it's a static library then static linking causes link errors with duplicate symbols. If it's a dyanamic library, then static linking only works if the app also links with the dynamic library, otherwise they'll see undefined symbols.

So not sure what's the good message here.

Warning: Pod Foo using swift package Bar with static linking, this might cause linker errors. Consider using USE_FRAMEWORKS=dynamic, see https://github.com/facebook/react-native/pull/44627 for more information

Maybe we also add a url to this issue, or a new one created just for explaining the potential issues with SPM regarding linking?

What do you think?

Yes given that the reload issue seems to get's solved by new Xcode version, it don't think it's worth to mess with osascript workaround, so message is good there. The linking part is a bit complicated, not sure what's a good message there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants