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

Fix #784 Swift Package Manager (Embed & Sign) for dynamic packages. #788

Conversation

alexruperez
Copy link
Contributor

@alexruperez alexruperez commented Feb 25, 2020

Swift Package Manager dynamic dependencies declared like:

.library(name: "MyLib", type: .dynamic, targets: ["MyLib"])

Are embedded (Embed & Sign or Embed Without Signing if codeSign: false) when you declare embed: true in package dependency definition:

Captura de pantalla 2020-02-19 a las 16 35 06

And included here:

Captura de pantalla 2020-02-19 a las 16 35 34

@JesusMartinAlonso
Copy link

It would be great if this PR could be approved soon! Thanks @alexruperez for the contribution! :)

@brentleyjones
Copy link
Collaborator

@alexruperez Thanks for the fix! Can you update the tests and include the updated test fixtures as well? Also, can you add a Changelog entry?

@ghost
Copy link

ghost commented Feb 25, 2020

great work @alexruperez :) thanks for taking time to fix it..

@yonaskolb yonaskolb linked an issue Feb 25, 2020 that may be closed by this pull request
@yonaskolb
Copy link
Owner

Thanks for the PR @alexruperez!
Currently this PR embeds all Swift packages by default which is not I think what we want. You can see this in the test failure. We either need a way to tell if a package library is static or dynamic, or only embed if it’s been defined explicitly in the spec. The current embed property defaults to yes for application targets as can be seen above your change. We can create our own property for swift package dependencies that defaults to false

 let embed = dependency.embed ?? false

Why do you think? I more thorough solution would be to check the framework itself for a good default.

In addition could you please:

Thanks!

@alexruperez
Copy link
Contributor Author

Thanks for the feedback @yonaskolb. Added changelog entry and fixtures updates.

Also SPM dependencies are embedded only when embed: true is declared in the project.yml

Does this fit for you?

Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Thanks @alexruperez! Looks good!

@yonaskolb yonaskolb merged commit bd7e0b7 into yonaskolb:master Feb 29, 2020
@yonaskolb
Copy link
Owner

Released in 2.14.0

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

Successfully merging this pull request may close these issues.

Dependencies: SPM dynamic dependency embed & sign
4 participants