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] Wrong assumption of Package.revision being non-optional #128

Merged

Conversation

JanBrinker
Copy link
Contributor

My team recently had to move a company internal package to Artifactory and this broke SwiftPackageList parsing the Package.resolved file on consumer side. There was a wrong assumption of the field revision always being set. However this field is optional (see SPM code: https://github.com/swiftlang/swift-package-manager/blob/release/6.0.2/Sources/PackageGraph/PinsStore.swift#L443) and will be empty for registry packages.

I fixed this assumption and made that property optional. I also added a test case with a sample entry on how a registry package looks in thePackage.resolved file.

Furthermore, just to be explicit about what is supported and what isn't, I added a section in the Readme file to note that SwiftPackageList is not able to get license files out of packages received from registries. Registry packages are usually closed source, so it won't matter (usually), but stating it explicitly might save someone quite a lot of headache.

This is not the case for registry packages.
Copy link
Owner

@FelixHerrmann FelixHerrmann 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 contribution, thank you! Will make a release in a bit.

@@ -268,6 +268,10 @@ The Settings Bundle and the UI-components are currently localized in the followi

> If a language has mistakes or is missing, feel free to create an issue or open a pull request.

## Known limitations

- SwiftPackageList won't include license files from packages that are located in a registry like Artifactory.
Copy link
Owner

Choose a reason for hiding this comment

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

I would like to play around with that but there is still not a single public registry I could easily use, right?

public let revision: String

/// Could be `nil` if the package is located in a registry.
public let revision: String?
Copy link
Owner

@FelixHerrmann FelixHerrmann Nov 20, 2024

Choose a reason for hiding this comment

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

This is technically a breaking changes and requires a major version bump but I think we can get it in with a patch release because its more of a bug fix and I don't think that it's utilized often.

@FelixHerrmann FelixHerrmann changed the title Fix wrong assumption of revision field always being set [Fix] Wrong assumption of Package.revision being non-optional Nov 20, 2024
@FelixHerrmann FelixHerrmann merged commit b538985 into FelixHerrmann:master Nov 20, 2024
14 checks passed
@JanBrinker
Copy link
Contributor Author

Wow thanks for the quick response!
Yes, I think there is no real public registry for SPM. In general for Open Source projects it also doesn't make a lot of sense to use a registry. The repo is already publicly available, so why bundle up the code and send it to a registry? It makes a lot more sense for closed source inhouse/enterprise packages and for those the license question will be handled differently.

That is also why it was important for me to add the unit test case, so you could see how a Package.resolved entry for a registry package looks like.

@FelixHerrmann
Copy link
Owner

Gotcha, once there is one I will give it a try. I think it fixes big repository sizes, intercom-ios for example is > 2 GB, current workaround is to have a dedicated repository just for the SPM manifest like intercom-ios-sp.

The test case is very helpful, thanks again!

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.

2 participants