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

Unused dependency in Package.swift? #21

Closed
JRHeaton opened this issue Mar 20, 2019 · 6 comments
Closed

Unused dependency in Package.swift? #21

JRHeaton opened this issue Mar 20, 2019 · 6 comments

Comments

@JRHeaton
Copy link

JRHeaton commented Mar 20, 2019

Hi there 👋

I was noticing that since the merge of #10, my projects using Tagged have more transitive dependencies.
image

This seems due to this dependency specifier in Package.swift:

.package(url: "https://github.com/yonaskolb/XcodeGen.git", from: "2.2.0"),

From what I can see in the rest of Package.swift, none of the library or test targets depend on any targets from XcodeGen. I'm curious if/why this is needed, or if it's something that can be removed.

Thanks!

@stephencelis
Copy link
Member

Hey @JRHeaton! We use XcodeGen to generate Xcode projects for most of our libraries, mainly to support Carthage. While we were originally running un-vendored versions of xcodegen, our versions were going out of sync and we were seeing some back-and-forth churn in the project file format. Ideally SwiftPM would have the notion of developmentDependencies, or allow for something like Package.development.swift, but I don't think any such thing exists right now :/ The extra dependencies are definitely annoying, but it's a quirk of how SwiftPM works right now and shouldn't have a noticeable impact on your project. Do you have any specific concerns that these packages are being downloaded?

@stephencelis
Copy link
Member

@JRHeaton I chatted with the SPM team and it looks like this will be fixed in the future: https://github.com/apple/swift-evolution/blob/60266bb8d125dee56f2d270b3fd25aa9eeeb63c8/proposals/0226-package-manager-target-based-dep-resolution.md

@JRHeaton
Copy link
Author

JRHeaton commented Mar 20, 2019

@stephencelis Right, and while I'm happy to see that in the future unused dependencies won't be built per that proposal, it's the fact that it's in the Package.swift in this case at all that is confusing. XcodeGen isn't needed for any part of the actual project itself; it's (if I understand correctly) just for maintenance purposes during development to, as you said, generate projects, etc.

So essentially my concern is that now every project that I use with Tagged(which is most if not all) now has to download and build 13 new targets, most of which are library dependencies for an executable that again, is for dev/maintenance, not usage of the library. This is a timely I'm sure, especially on CI.

@stephencelis
Copy link
Member

@JRHeaton While every dependency needs to be downloaded these days to resolve the current graph, only the dependencies your project uses will be built. You shouldn't see much of an increase in CI time because the downloads are quite fast. Downloading and resolving the Tagged graph takes about 11 seconds for us: https://travis-ci.org/pointfreeco/swift-tagged

Many CI services also offer the ability to cache directories between builds to speed things up. If you use one of these features, you should see no increase in CI time.

It's becoming more and more common to use Package.swift as a way of managing Swift development dependencies as described here: https://artsy.github.io/blog/2019/01/05/its-time-to-use-spm/

And it's been common in the past for folks to use CocoaPods to manage a development dependency on, say, SwiftLint. The main issue seems to be that SPM hasn't caught up quite yet.

Brandon and I are open to revisiting things, but want to better understand the concern.

@JRHeaton
Copy link
Author

@stephencelis Thanks for the correction of my assumption that SPM would build those unreferenced targets. I guess this means the behavior is benign until now until SPM is upgraded; I just wanted to raise this to make sure it was not a leftover artifact from the time/money PR as we noticed these new dependencies in our Package.resolved in code review at work and it led me to Tagged 😇

I'll go ahead and close this. Thank you again for the clarification and the links!

@stephencelis
Copy link
Member

@JRHeaton Just to follow up, it's been brought to our attention that we can scope development dependencies using environment variables (thanks @inamiy), so we'll avoid these excess downloads soon! See 77bdcc1

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

No branches or pull requests

2 participants