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

Add option for not linking dependencies #109

Merged
merged 2 commits into from
Oct 26, 2017
Merged

Conversation

keith
Copy link
Collaborator

@keith keith commented Oct 25, 2017

This adds a new attribute to Dependency that allows consumers to choose
to not link a dependency. This is useful for if you have this dependency
tree with static libraries:

App -> A -> Shared
App -> B -> Shared

Where A and B both share a static library dependency, that is finally
linked into App. If Shared is added to the link phase of A and B, you
end up with duplicate symbols during the link phase. With this change
consumers could set link: False on A and B's dependency on Shared, this
way Shared will get build before A and B, but not linked.

This adds a new attribute to Dependency that allows consumers to choose
to not link a dependency. This is useful for if you have this dependency
tree with static libraries:

App -> A -> Shared
App -> B -> Shared

Where A and B both share a static library dependency, that is finally
linked into App. If Shared is added to the link phase of A and B, you
end up with duplicate symbols during the link phase. With this change
consumers could set link: False on A and B's dependency on Shared, this
way Shared will get build before A and B, but not linked.
@keith keith requested a review from yonaskolb October 25, 2017 23:06
Copy link

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

This is a great idea!

@yonaskolb
Copy link
Owner

Nice one @keith! Would this option be applicable to the carthage or framework dependency types as well? Then this check should be added to those cases.

Also could you add some documentation in Docs/ProjectSpec.md

@keith
Copy link
Collaborator Author

keith commented Oct 26, 2017

Would this option be applicable to the carthage or framework dependency types as well?

Good question, after thinking about this a bit, I think no, because the reason to have a target dependency for something is to make sure it's built first, where in the case of those already built frameworks if you don't want to link them, they just shouldn't be a dependency of your target. Also since we're specifically talking about the dynamic framework case, whether or not you link them at this phase won't affect the final product, where with static libraries (or static frameworks) this will be an issue because you'll end up with duplicate symbols.

@keith
Copy link
Collaborator Author

keith commented Oct 26, 2017

Added documentation!

@@ -219,6 +219,7 @@ A dependency can be one of a 3 types:
These only applied to `target` and `framework` dependencies.

- ⚪️ **embed**: `Bool` - Whether to embed the dependency. Defaults to true for application target and false for non application targets.
- ⚪️ **link**: `Bool` - Whether to link the dependency. Defaults to true for but only static library and dynamic frameworks are linked.
Copy link
Owner

Choose a reason for hiding this comment

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

for but
And maybe mention that this is only applicable for target dependency types

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated!

@keith keith force-pushed the ks/link-dependency branch from 619377e to d31f9be Compare October 26, 2017 20:14
@yonaskolb yonaskolb merged commit ff73efa into master Oct 26, 2017
@keith keith deleted the ks/link-dependency branch October 26, 2017 20:20
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.

3 participants