Add PackageIdentity type #3057

mattt · 2020-11-16T22:56:30Z

Motivation:

Modifications:

Replaced PackageReference.PackageIdentity with top-level PackageIdentity type
Replaced existing calls to PackageReference.computeIdentity to initialization of new PackageIdentity type.
Changed access level of PackageReference.computeDefaultName to internal, limiting its use outside of PackageModel module.
Extracted testPackageReference into new, separate test case
Updated package identity to be configurable via top-level _useLegacyIdentities variable. This acts as a setter for a static internal property on PackageIdentity that configures how package identity is calculated via dependency injection.

Result:

This PR shouldn't affect current behavior. Rather, it serves to codify existing determinations of package identity and to tee up subsequent PRs to accommodate the new package identity semantics required for package registry support.

…ty type

Rename PackageIdentityTests to LegacyPackageIdentityTests

mattt · 2020-11-16T22:57:43Z

Sources/PackageModel/PackageIdentity.swift

+import TSCBasic
+import TSCUtility
+
+//@warn_unqualified_access


Aside: I wish @warn_unqualified_access was available for top-level variable declarations (not just functions).

neonichu · 2020-11-17T00:14:08Z

Sources/PackageModel/PackageIdentity.swift

+    }
+}
+
+internal protocol PackageIdentityProvider: LosslessStringConvertible {


I think the identity provider also has to provide equality for package identities, not just an initializer. For the "real" new implementation of identity we will need a staggered comparison that's not just comparing strings.

If we require PackageIdentityProvider to be Equatable, then it can only be used as a generic constraint, because it has Self requirements in ==. Can you think of a reasonable / non-terrible way to work around this?

I would just add a comparison function that's unrelated to Equatable. I think what we want to be able to do is compare two PackageIdentity instances using a comparison operation vended by PackageIdentityProvider.

neonichu

Thanks, this looks great!

I would just make the identity provider a bit more powerful as mentioned.

abertelrud · 2020-11-17T00:47:13Z

Sources/PackageModel/PackageIdentity.swift

+/// in a process called package resolution.
+///
+/// External package dependencies are located by a URL
+/// (which may be an implicit `file://` URL in the form of a file path).


Should this description also mention path-based external dependencies? They do not use file://, which is only used when the local path contains an actual local Git repository and not just an unversioned package. Or is package identity only a thing for actual remote packages (not root or local)?

That's what I was trying to get at — my conceit here is that a file path is actually a file:/// URL with an implicit scheme. Any package dependency, local or remote, is described by a URL: local use file:/// URLs and remote use any other scheme. CanonicalPackageIdentity distinguishes between local from remote by preserving a leading slash (for example, you couldn't shadow a remote URL by having a root directory named /github.com/). Does that all make sense?

Understood, but I think that's too subtle and confusing of a way to represent it. And checking the file system for whether a particular path has a .git isn't a good way to do it either. I think there should be a clear way to represent that a package is either an unversioned take-it-as-given path in the file system or a repository that just happens to be in the local file system. Historically there has been value in using no scheme for the former and file: for the latter. I think it would be a mistake to conflate them, even it's through an extra slash character.

Sources/PackageModel/PackageReference.swift

tomerd

this looks great, thanks @mattt

there are two question @abertelrud brought up that should be answered but I hope they are straight forward by exposing the identity on the ref

abertelrud · 2020-11-17T06:48:16Z

Right, I don't think they are significant issues (since local packages are supported via scheme-less URLs and are distinct from file:// URLs) but since clients will need to adapt, I'm trying to identify what the downstream changes will need to be. From the unit tests it looks as if it can mostly be handled by instantiating a PackageIdentity from the URL strings. I think the only remaining question is how a client that wanted to display a package's display name before fetching that package would do so. For the short term it could show the description of the package identity but since that is an opaque value with unspecified format it might not be visually appealing.

tomerd · 2020-11-17T18:18:48Z

@swift-ci please smoke test

Distinguish between path and URL initializers at call sites

mattt · 2020-11-17T18:52:57Z

@abertelrud @tomerd @neonichu Thanks for reviewing my changes. I just pushed 766ea50, which is responsive to this discussion thread. This commit takes the additional step to distinguish between PackageIdentity values constructed from absolute paths and URLs (per @abertelrud 's comment). Right now, the url version takes a String, but that could be migrated to Foundation.URL or some other URL type in the future.

Does this seem like the right change for this PR?

mattt · 2020-11-17T19:03:31Z

@swift-ci please smoke test

abertelrud · 2020-11-17T19:14:05Z

Thanks, @mattt! I'll take a look at the new diffs right away, but that sounds like the right API from a client's perspective.

abertelrud

Thanks @mattt, this looks good to me know. I realize that separate URL and AbsolutePath initializers for package identity are maybe tedious, but I think it will really help clients keep things straight. In one client I worked on, this confusion, where everything was just a string, was a source of at least one bug.

abertelrud · 2020-11-17T20:54:37Z

Thanks again for these diffs, @mattt. I'm adapting some of the downstream dependencies to it and will do some testing to avoid regressions. I plan to wrap that up by tonight. Are you OK with holding off until tomorrow to merge?

mattt · 2020-11-17T21:28:04Z

Absolutely. Thanks for taking a look at the downstream consumers. Let me know if anything comes up in your review.

abertelrud · 2020-11-17T22:51:17Z

Great, thanks @mattt. The changes that are technically needed to keep compiling turn out to be fairly minimal, and similar to unit test changes in this PR... the later changes to adapt to the change in package identity will be more interesting (but of course very useful!). But that's for after this PR lands.

abertelrud · 2020-11-18T18:38:31Z

@mattt this seems to work fine downstream, after adapting some of the client code. All ready to merge as far as you're concerned?

mattt · 2020-11-18T18:47:19Z

@abertelrud I just gave everything another once-over, and I think this is ready to merge. Feel free to do that whenever you're ready.

I'd like to continue our conversation from https://github.com/apple/swift-package-manager/pull/3057/files#r525394175, but that's not a blocker here.

abertelrud · 2020-11-18T19:26:50Z

Great, thanks! Merging.

tomerd · 2020-11-18T20:41:40Z

🥳 🎊

* Replace PackageReference.PackageIdentity with top-level PackageIdentity type * Change access level of PackageReference.computeDefaultName to internal * Extract testPackageReference into new PackageIdentityTests file * Allow package identity to be configurable via dependency injection Rename PackageIdentityTests to LegacyPackageIdentityTests * Revert change to access level of computeDefaultName, making it public * Remove LosslessStringConvertible conformance * Add documentation comment for provider type property * Add initializer that takes an AbsolutePath parameter * Remove commented @warn_unqualified_access attribute * Add argument label to public PackageIdentity initializers Distinguish between path and URL initializers at call sites

mattt added 4 commits November 16, 2020 13:37

Replace PackageReference.PackageIdentity with top-level PackageIdenti…

cbc2e3e

…ty type

Change access level of PackageReference.computeDefaultName to internal

f2c7c21

Extract testPackageReference into new PackageIdentityTests file

63481c7

Allow package identity to be configurable via dependency injection

d7f8408

Rename PackageIdentityTests to LegacyPackageIdentityTests

mattt requested review from neonichu, tomerd and abertelrud November 16, 2020 22:56

mattt requested review from aciidgh and friedbunny as code owners November 16, 2020 22:56

mattt commented Nov 16, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

tomerd assigned neonichu Nov 16, 2020

neonichu reviewed Nov 17, 2020

View reviewed changes

neonichu approved these changes Nov 17, 2020

View reviewed changes

abertelrud reviewed Nov 17, 2020

View reviewed changes

Sources/PackageModel/PackageReference.swift Outdated Show resolved Hide resolved

abertelrud reviewed Nov 17, 2020

View reviewed changes

Sources/PackageModel/PackageReference.swift Show resolved Hide resolved

tomerd approved these changes Nov 17, 2020

View reviewed changes

abertelrud approved these changes Nov 17, 2020

View reviewed changes

mattt added 5 commits November 17, 2020 09:39

Revert change to access level of computeDefaultName, making it public

566a08e

Remove LosslessStringConvertible conformance

912cfbf

Add documentation comment for provider type property

b33a963

Add initializer that takes an AbsolutePath parameter

23df82a

Remove commented @warn_unqualified_access attribute

336730b

Add argument label to public PackageIdentity initializers

766ea50

Distinguish between path and URL initializers at call sites

mattt requested a review from abertelrud November 17, 2020 19:03

mattt requested review from tomerd and neonichu November 17, 2020 19:03

abertelrud approved these changes Nov 17, 2020

View reviewed changes

abertelrud self-assigned this Nov 18, 2020

abertelrud merged commit e887924 into swiftlang:main Nov 18, 2020

mattt deleted the package-identity branch November 18, 2020 20:49

mattt mentioned this pull request Nov 21, 2020

Add package registry implementation #3023

Closed

Add PackageIdentity type #3057

Add PackageIdentity type #3057

Uh oh!

Conversation

mattt commented Nov 16, 2020

Motivation:

Modifications:

Result:

Uh oh!

mattt Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

neonichu Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

mattt Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neonichu Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

neonichu left a comment

Choose a reason for hiding this comment

Uh oh!

abertelrud Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

mattt Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

abertelrud Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tomerd left a comment

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomerd commented Nov 17, 2020

Uh oh!

mattt commented Nov 17, 2020

Uh oh!

mattt commented Nov 17, 2020

Uh oh!

abertelrud commented Nov 17, 2020

Uh oh!

abertelrud left a comment

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Nov 17, 2020

Uh oh!

mattt commented Nov 17, 2020

Uh oh!

abertelrud commented Nov 17, 2020

Uh oh!

abertelrud commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattt commented Nov 18, 2020

Uh oh!

abertelrud commented Nov 18, 2020

Uh oh!

tomerd commented Nov 18, 2020

Uh oh!

Uh oh!

mattt Nov 17, 2020 •

edited

Loading

abertelrud commented Nov 17, 2020 •

edited

Loading

abertelrud commented Nov 18, 2020 •

edited

Loading