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 Support to --package path/to/Package.swift Parameter #7

Merged
merged 13 commits into from Sep 5, 2021

Conversation

rogerluan
Copy link
Collaborator

@rogerluan rogerluan commented Aug 29, 2021

Description

This PR picks up the work that was initiated here d04da28 and aims to bring support for the --package argument, which receives a path to a Package.swift file, and plots a graph for the dependencies of the given Swift Package.

The heavy lifting was really done by @maxchuquimia. What I did was finish up the TODO left here d04da28#diff-153d5429d399c18d0a6dd32f593f6d1411fd0c8e9df19e62701497835913edb9R31 and fix the unit tests. I also added a new dependency to the sample SPM project to make the resulting graph look more interesting (where 2 packages (SomePackage and Moya) depends on the same dependency (Alamofire).

Resolves #4

Note: this PR was branched off of #6 so please review & possibly merge it first, before reviewing & merging this one. As I'm working off of a GitHub Fork, I can't point this PR against my other branch 😬

Demo

image

@rogerluan rogerluan mentioned this pull request Aug 29, 2021
Copy link
Owner

@maxchuquimia maxchuquimia 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 this! I'm surprised the swift build stuff actually works, was worried there might need to be handling around the OSes that the package supports.

It's a little difficult to read through the code whilst the other PR is still open, so I'll want to go through it again once it is merged - but looking good!

And, we will need new tests that specifically target a package as a starting point I reckon.

Sources/XCGrapherLib/XCGrapher.swift Show resolved Hide resolved
@@ -76,6 +76,7 @@ private enum KnownEdges {
("SomePackage", "Kingfisher"),
("SomePackage", "Moya"),
("Moya", "Alamofire"),
("SomePackage", "Alamofire"),
Copy link
Owner

Choose a reason for hiding this comment

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

I know you said you did this to make the tests graph more interesting - but I specifically chose Moya because I knew it was dependent on Alamofire and that way we could properly test that dependencies of dependencies are discovered. Would prefer to revert it back to the way it was as in this case we could introduce a bug where only direct dependencies of the starting point are discovered!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm happy to remove Alamofire direct dependency, but I'd like to introduce enough dependencies to replicate this behavior:

image

(2 arrows coming to the same dependency)
Can you think of other packages that we could achieve this? 😁

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we could simply add https://github.com/apple/swift-algorithms? It seems to be dependent on https://github.com/apple/swift-numerics - then we could just keep your changes of adding Alamofire, easy peasy

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we re-evaluate this on (or after) #10? 🙏 I've changed quite a lot of things between this PR and that one, so fixing in both would be kind of a mess. I reckon I could've fixed it earlier, but I've been working on that branch for quite some time already 😅 Hope you're cool with that 👍

Copy link
Owner

Choose a reason for hiding this comment

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

Yup we can - should we hold off merging until then too, or do you prefer it to merge before #10?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah now I see in #10 that you would like this merged first 🎉

@maxchuquimia maxchuquimia merged commit 7ff6772 into maxchuquimia:master Sep 5, 2021
@rogerluan rogerluan deleted the feature/DirectSPMSupport branch September 5, 2021 09:57
@rogerluan
Copy link
Collaborator Author

Thanks for merging this in @maxchuquimia ! 🎉 🚀🚀

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.

Support for other package managers?
2 participants