Skip to content

Replace PackageGraphRoot.PackageDependency with PackageDependencyDescription #2997

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

Merged
merged 6 commits into from
Oct 26, 2020

Conversation

mattt
Copy link
Contributor

@mattt mattt commented Oct 23, 2020

As discussed with @neonichu and @aciidb0mb3r (see this comment), this PR replaces all use of PackageGraphRoot.PackageDependency with PackageDependencyDescription. This gets us one step closer to having a single, canonical type capable of representing package dependencies to the extent required to implement the package registry (#2967).

This PR also does away with FilteredDependencyDescription, which now has an equivalent representation by way of the productFilter property added to PackageDependencyDescription.

@mattt

This comment has been minimized.

Copy link
Contributor

@abertelrud abertelrud 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 cleanup! I had one question about what looks as if it might be a semantic difference, but I might of course be reading the code wrong. Apart from that this looks good.

return PackageReference(
identity: PackageReference.computeIdentity(packageURL: effectiveURL),
identity: explicitName?.lowercased() ?? PackageReference.computeIdentity(packageURL: effectiveURL),
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a change in semantics. Are there any risks here?

Copy link
Contributor Author

@mattt mattt Oct 26, 2020

Choose a reason for hiding this comment

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

Good catch. If you remove the left-hand side of that ?? expression, tests will fail, with reports of mismatched dependency names. So although this looks like a semantic change, it's merely a downstream effect of consolidating PackageGraphRoot.PackageDependency into PackageDependencyDescription.

Later on, we will be changing the semantics so that we only use the package URL (normalized for scheme, case, etc.) for identity. Any explicit names would then become convenient labels rather than something load-bearing — more like a variable name within a block than a memory address. But to be clear: that's not what this change is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see; thanks for the explanation. If this doesn't have any actual effect, it would be great if there were a comment explaining what's going on. If this will go away when the notion of package identity changes, then this seems OK for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Added with 0ccdeaf.

Copy link
Contributor

@neonichu neonichu left a comment

Choose a reason for hiding this comment

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

LGTM as well, no other comments than the ones Anders already made.

@mattt
Copy link
Contributor Author

mattt commented Oct 26, 2020

FYI — Currently getting a 502 Proxy Error for the details on the failed smoke test.

Proxy Error

The proxy server received an invalid response from an upstream server.
The proxy server could not handle the request GET /job/swift-package-manager-Linux-smoke-test/2219/.

Reason: Error reading from remote server

@mattt

This comment has been minimized.

@mattt
Copy link
Contributor Author

mattt commented Oct 26, 2020

@swift-ci please smoke test

@mattt
Copy link
Contributor Author

mattt commented Oct 26, 2020

Getting 502 proxy errors again (see #2997 (comment))

@mattt
Copy link
Contributor Author

mattt commented Oct 26, 2020

/Users/buildnode/jenkins/workspace/swift-package-manager-PR-osx-smoke-test/branch-main/llvm-project/llvm/lib/Target/X86/X86.td:1349:1: error: Primary decode conflict
07:13:51 def X86 : Target {

^ Seems like an unrelated one-off. Trying again.

@mattt
Copy link
Contributor Author

mattt commented Oct 26, 2020

@swift-ci please smoke test

@mattt
Copy link
Contributor Author

mattt commented Oct 26, 2020

@abertelrud Any more requested changes? Or is this ready to be squashed and merged?

@abertelrud
Copy link
Contributor

abertelrud commented Oct 26, 2020

This looks good to me at this point. Thanks!

Are you able to merge it or should I do it?

@abertelrud abertelrud self-assigned this Oct 26, 2020
@mattt
Copy link
Contributor Author

mattt commented Oct 26, 2020

@abertelrud It looks like I'm able to do it, so I'll go ahead and merge now.

@mattt mattt merged commit df81b0e into swiftlang:main Oct 26, 2020
@mattt mattt deleted the package-dependency-description branch October 26, 2020 21:12
federicobucchi pushed a commit to federicobucchi/swift-package-manager that referenced this pull request Jan 6, 2021
…ription (swiftlang#2997)

* Replace PackageGraphRoot.PackageDependency with PackageDependencyDescription

* Add documentation comment for nothing type property

* Update CMakeLists.txt for PackageModel module

* Replace reference to FilteredDependencyDescription

* Add comment about how package identity is determined
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