improve correctness by using identity more broadly throughout the code #3244

tomerd · 2021-02-08T03:51:24Z

motivation: improve correctness by using identity more broadly throughout the code, prepare to package registry

changes:

refector as PackageDependencyDescription with "scm" and "local" cases, this reflects better the true meaning and associated data with the two options and prepares us to adding a third option for registry
create new abstraction IdentityResolver, with default implementation that follows existing identity resolution logic
move mirror logic into IdentityResolver, and simplify code that used mirrors
move all logic to generate identity, apply mirrors to PackageDependencyDescription so it is handles as soon as possible in the call stack
adjust call-sites
adjust and add tests for mirror

builds on #3239 and #3240 so lets merge those first

tomerd · 2021-02-08T04:37:09Z

@swift-ci please smoke test

tomerd · 2021-02-08T04:37:52Z

Sources/PackageLoading/IdentityResolver.swift

+import TSCBasic
+import PackageModel
+
+// TODO: refactor this when adding registry support


tomerd · 2021-02-08T04:40:02Z

Sources/PackageLoading/PackageDescription4Loader.swift

-        let isBaseURLRemote = URL.scheme(packageLocation) != nil
-
-        func fixLocation(_ location: String, requirement: Requirement) throws -> String {
+    fileprivate init(


👀 this is the key change, the logic to address identity resolution d location remapping (aka mirrors) moved to the constructor of the PackageDependencyDescription which is the earliest time and makes the rest of the code simpler can it assume the dependency identity and location is correct

tomerd · 2021-02-08T04:41:44Z

Sources/PackageModel/Manifest/PackageDependencyDescription.swift

@@ -11,15 +11,33 @@
 import TSCBasic

 /// Represents a package dependency.
-public struct PackageDependencyDescription: Equatable, Codable, Hashable {
+public enum PackageDependencyDescription: Equatable {


👀 this is the other key change: PackageDependencyDescription is now and enum with two cases: local and git. the idea is to model the data more accurately instead of the a struct that has fields that are reused with different meaning and make reasoning in downstream harder. As we get closer with the registry, we will add a 3rd option registry with its own payload.

The one drawback (since enums can't have stored properties) is that common properties have to be replicated across the payloads. I don't think this is a problem but another way that might make sense would be to separate out the reference type as an enum (path, git repo, registry), which would also make clear what is common to all packages (e.g. identity and filter) but leaving the specifics of the reference type to the enum. Just a thought.

yes, this is the tradeoff here. i went with creating computed properties for the shared properties since its not a big price to pay, and the price is paid only once in PackageDependencyDescription itself, keeping the call-sites nice and clean. if we split the properties across the associated values and and some "type" enum, the call-sites would have to know how to access the properties so the code in PackageDependencyDescription would be cleaner but the call-sites "uglier"

Agreed. Overall it's a good tradeoff. I expect it to be very rare to have to add enum cases here.

my thoughts exactly

tomerd · 2021-02-08T04:43:39Z

Sources/PackageModel/Manifest/PackageDependencyDescription.swift

-    public let name: String
+    // FIXME: we should simplify target based dependencies such that this is no longer required
+    // A name to be used *only* for target dependencies resolution
+    public var nameForTargetDependencyResolutionOnly: String {


renamed this so its obvious what its only fo

tomerd · 2021-02-08T04:45:10Z

Sources/PackageModel/Manifest/PackageDependencyDescription.swift


    /// The dependency requirement.
    public enum Requirement: Equatable, Hashable {
        case exact(Version)
        case range(Range<Version>)
        case revision(String)
        case branch(String)
-        case localPackage


a nice side effect of the move to enum is that we can get rid of localPackage as kind of requirement which seems unnatural

Agreed, that was always odd.

tomerd · 2021-02-08T04:47:37Z

Sources/Workspace/Workspace.swift

@@ -1309,6 +1309,7 @@ extension Workspace {
    /// This will load the manifests for the root package as well as all the
    /// current dependencies from the working checkouts.
    // @testable internal
+    // FIXME: this logic should be changed to use identity instead of location once identity is unique


this is the next step in the identity cleanup journey, but before its unique we need to keep using locations here :(

tomerd · 2021-02-08T04:53:32Z

Tests/FunctionalTests/DependencyResolutionTests.swift

@@ -65,4 +65,62 @@ class DependencyResolutionTests: XCTestCase {
            XCTAssertEqual(output, "♣︎K\n♣︎Q\n♣︎J\n♣︎10\n♣︎9\n♣︎8\n♣︎7\n♣︎6\n♣︎5\n♣︎4\n")
        }
    }
+
+    func testMirrors() {


Hmm, was there no test at all before?!

there was a test that uses all mocks, no end-to-end test

tomerd · 2021-02-08T04:58:41Z

@swift-ci please smoke test

abertelrud

This is great! Just had a question/suggestion for PackageDependencyDescription but this looks good.

abertelrud · 2021-02-08T20:17:31Z

Sources/PackageLoading/IdentityResolver.swift

+public protocol IdentityResolver {
+    func resolveIdentity(for location: String) -> PackageIdentity
+    func resolveIdentity(for path: AbsolutePath) -> PackageIdentity
+    func resolveLocation(from location: String) -> String


Is this String because it could be either path or URL or identity?

yes, I was contemplating moving to URL as well but wanted to minimize changes so kept String for now. We could/should revisiting this in follow-up PRs

Great, that seems like a good choice (keeping PRs incremental)

abertelrud · 2021-02-08T20:24:37Z

Sources/PackageLoading/PackageDescription4Loader.swift

+                // If the dependency URL starts with '~/', try to expand it.
+                return fileSystem.homeDirectory.appending(RelativePath(String(dependencyLocation.dropFirst(2)))).pathString
+            } else if dependencyLocation.hasPrefix(filePrefix) {
+                // FIXME: SwiftPM can't handle file locations with file:// scheme so we need to


Right, in particular, file:// is treated as a remote package whose repo just happens to be in the local file system.

abertelrud · 2021-02-08T20:27:29Z

Sources/PackageModel/Manifest/PackageDependencyDescription.swift

+    public struct Git: Equatable, Codable {
+        public let identity: PackageIdentity
+        public let name: String?
+        public let location: String


Is this always a URL in this case? Would it make sense to use URL, or rather keep as String?

abertelrud · 2021-02-08T20:28:06Z

Sources/PackageModel/Manifest/PackageDependencyDescription.swift


    /// The dependency requirement.
    public enum Requirement: Equatable, Hashable {
        case exact(Version)
        case range(Range<Version>)
        case revision(String)
        case branch(String)
-        case localPackage


Agreed, that was always odd.

abertelrud · 2021-02-08T20:32:47Z

Sources/PackageModel/Manifest/PackageDependencyDescription.swift

@@ -11,15 +11,33 @@
 import TSCBasic

 /// Represents a package dependency.
-public struct PackageDependencyDescription: Equatable, Codable, Hashable {
+public enum PackageDependencyDescription: Equatable {


The one drawback (since enums can't have stored properties) is that common properties have to be replicated across the payloads. I don't think this is a problem but another way that might make sense would be to separate out the reference type as an enum (path, git repo, registry), which would also make clear what is common to all packages (e.g. identity and filter) but leaving the specifics of the reference type to the enum. Just a thought.

abertelrud · 2021-02-08T20:33:31Z

Sources/PackageModel/Manifest/PackageDependencyDescription.swift

-    /// - SeeAlso: `explicitName`
-    /// - SeeAlso: `url`
-    public let name: String
+    // FIXME: we should simplify target based dependencies such that this is no longer required


abertelrud · 2021-02-08T20:35:42Z

Tests/FunctionalTests/DependencyResolutionTests.swift

@@ -65,4 +65,62 @@ class DependencyResolutionTests: XCTestCase {
            XCTAssertEqual(output, "♣︎K\n♣︎Q\n♣︎J\n♣︎10\n♣︎9\n♣︎8\n♣︎7\n♣︎6\n♣︎5\n♣︎4\n")
        }
    }
+
+    func testMirrors() {


Hmm, was there no test at all before?!

tomerd · 2021-02-09T02:10:48Z

@swift-ci please smoke test

tomerd · 2021-02-09T02:13:36Z

@swift-ci please smoke test

tomerd · 2021-02-09T07:44:08Z

@neonichu lmk what you think

motivation: improve correctness by using identity more broadly throughout the code, prepare to package registry changes: * refector as PackageDependencyDescription with "scm" and "local" cases, this reflects better the true meaning and associated data with the two options and prepares us to adding a third option for registry * create new abstraction IdentityResolver, with default implementation that follows existing identity resolution logic * move mirror logic into IdentityResolver, and simplify code that used mirrors * move all logic to generate identity, apply mirrors to PackageDependencyDescription so it is handles as soon as possible in the call stack * adjust call-sites * adjust and add tests for mirror

tomerd · 2021-02-11T21:36:56Z

@swift-ci please smoke test

tomerd · 2021-02-11T21:37:22Z

@neonichu renamed git to scm as per your feedback

tomerd · 2021-02-11T23:26:42Z

@swift-ci please smoke test macOS

tomerd · 2021-02-12T03:44:02Z

@swift-ci please smoke test macOS

tomerd requested review from abertelrud, friedbunny and neonichu as code owners February 8, 2021 03:51

tomerd force-pushed the refactor/identity04 branch 2 times, most recently from eea65aa to ad5a33c Compare February 8, 2021 04:36

tomerd commented Feb 8, 2021

View reviewed changes

tomerd self-assigned this Feb 8, 2021

abertelrud approved these changes Feb 8, 2021

View reviewed changes

tomerd mentioned this pull request Feb 8, 2021

improve correctness by using identity more broadly throughout the code #3152

Closed

abertelrud mentioned this pull request Feb 9, 2021

refactor PackageDependencyDescription towards identity changes #3239

Merged

tomerd force-pushed the refactor/identity04 branch from 05ca1e7 to c613001 Compare February 9, 2021 02:08

tomerd force-pushed the refactor/identity04 branch from c613001 to 5965575 Compare February 9, 2021 02:13

tomerd changed the title ~~Refactor/identity04~~ improve correctness by using identity more broadly throughout the code Feb 9, 2021

tomerd changed the title ~~improve correctness by using identity more broadly throughout the code~~ improve correctness by using identity more broadly throughout the code Feb 9, 2021

tomerd force-pushed the refactor/identity04 branch from 5965575 to 9aef406 Compare February 11, 2021 21:31

tomerd force-pushed the refactor/identity04 branch from 9aef406 to e77f4ae Compare February 11, 2021 21:33

tomerd merged commit 5667965 into swiftlang:main Feb 12, 2021

improve correctness by using identity more broadly throughout the code #3244

improve correctness by using identity more broadly throughout the code #3244

Uh oh!

Conversation

tomerd commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomerd commented Feb 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Feb 8, 2021 • 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!

tomerd Feb 8, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abertelrud Feb 9, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Feb 8, 2021 • 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!

tomerd Feb 8, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Feb 8, 2021

Uh oh!

abertelrud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Feb 9, 2021

Uh oh!

tomerd commented Feb 9, 2021

Uh oh!

tomerd commented Feb 9, 2021

Uh oh!

tomerd commented Feb 11, 2021

Uh oh!

tomerd commented Feb 11, 2021

Uh oh!

tomerd commented Feb 8, 2021 •

edited

Loading

tomerd Feb 8, 2021 •

edited

Loading

tomerd Feb 8, 2021 •

edited

Loading

abertelrud Feb 9, 2021 •

edited

Loading

tomerd Feb 8, 2021 •

edited

Loading

tomerd Feb 8, 2021 •

edited

Loading