refactor location abstractions #3764

tomerd · 2021-09-26T05:19:16Z

motivation: reduce dependency on string location information, in support of registry based dependencies

changes:

add location information to PackageReference::Kind
seperare out sourceControl dependencies to local and remote, where remote carries AbsolutePath and rmeote carries URL
adjust call sites
adjust tests and test utilities

rdar://82954076

motivation: reduce dependency on string location information, in support of registry based dependencies changes: * add location information to PackageReference::Kind, using AbsolutePath and URL where appropriate * seperate out sourceControl dependencies to local and remote, where remote carries AbsolutePath and rmeote carries URL * add Location to RepositorySpecifier with local and remote, where remote carries AbsolutePath and rmeote carries URL * expand IdentityResolver to resolve identity from PackageReference::Kind, AbsolutePath and URL * adjust call sites (tons!) * adjust tests and test utilities (tons!) rdar://82954076

tomerd · 2021-09-26T07:05:19Z

Sources/PackageModel/Manifest/PackageDependencyDescription.swift

+        public enum Location: Equatable, Encodable {
+            case local(AbsolutePath)
+            case remote(Foundation.URL)
+        }


👀 this new enum is another key change

tomerd · 2021-09-26T07:06:10Z

Sources/PackageModel/PackageReference.swift

+        case localSourceControl(AbsolutePath)
+
+        /// A remote source package.
+        case remoteSourceControl(Foundation.URL)


👀 this enum change is the key change

What's the difference between fileSystem and localSourceControl? Do we do anything to ensure remoteSourceControl is indeed remote (e.g., what if URL scheme is file://?), or it doesn't matter?

Is there an opportunity for us to reuse the SourceControl.Location enumeration here?

https://github.com/apple/swift-package-manager/blob/51fe9f947e18fe3daccf3c2aaf6391136ba2b801/Sources/PackageModel/Manifest/PackageDependencyDescription.swift#L41-L44

yes, my plan is to further consolidate / cleanup after this initial but big PR lands.

tomerd · 2021-09-26T07:08:40Z

Sources/SourceControl/Repository.swift

+                return url.absoluteString
+            }
+        }
+    }


👀 this new enum is another key change

tomerd · 2021-09-26T08:16:23Z

@mattt this is the second PR designed to help with landing SE-0292, I think I prefer to take this one even before #3698

wdyt?

tomerd · 2021-09-26T08:18:52Z

@abertelrud @neonichu @elsh @yim-lee this PR includes refactoring we discussed several times with regards to how location is managed, and that is required to land SE-0292. its actually a pretty small change in the model itself but it has huge impact on call sites, especially in tests. given the large number of the lines changed, please focus on the core model changes which are highlighted in comments

note that this and #3698 are key PRs to help land SE-0292

tomerd · 2021-09-26T08:19:10Z

@swift-ci please smoke test

tomerd · 2021-09-26T16:58:41Z

@swift-ci please smoke test

tomerd · 2021-09-26T17:44:24Z

Sources/PackageGraph/RepositoryPackageContainer.swift

-        if package.kind != .remote {
-            return queue.async {
-                let container = LocalPackageContainer(
+        if let repositorySpecifier = package.repositorySpecifier {


@mattt here you would need to add a case for the registry, and probably we can rename RepositoryPackageContainerProvider to something more generic that deals with all PackageContainer types, perhaps rename to DefaultPackageContainerProvider

tomerd · 2021-09-26T17:45:15Z

Sources/PackageLoading/ManifestJSONParser.swift

+                productFilter: .everything
+            )
+        } else {
+            throw StringError("invalid location: \(location)")


with the new model, we can do things here much more safely

tomerd · 2021-09-26T17:47:25Z

Sources/PackageModel/PackageReference.swift

    ///
    /// This could be a remote repository, local repository or local package.
-    public let location: String
+    // FIXME: we should not need this
+    //@available(*, deprecated)


after landing this PR, there will be follow up to clean up call sites that still depend on location string. there are very few now, mostly for display so we are getting there

Great, this will do a lot to clarify the meaning of the code.

tomerd · 2021-09-26T17:52:08Z

Sources/SourceControl/Repository.swift

    /// The URL of the repository.
-    public let url: String
+    // FIXME: transition url to location


this is for follow up PR

tomerd · 2021-09-26T17:53:11Z

Sources/SourceControl/RepositoryManager.swift

@@ -550,9 +550,19 @@ extension RepositoryManager.RepositoryHandle {
        guard let status = Status(rawValue: repository.status) else {
            throw StringError("unknown status :\(repository.status)")
        }
+        // FIXME: encode the type


this is for follow up PR, its not too bad

yim-lee · 2021-09-26T18:14:13Z

Sources/PackageModel/PackageIdentity.swift

+    }
+
+    /// Creates a package identity from a URL.
+    /// - Parameter url: The package's URL.


Suggested change

/// - Parameter url: The package's URL.

/// - Parameter urlString: The package's URL.

yim-lee · 2021-09-26T18:18:19Z

Sources/PackageModel/PackageReference.swift

+        case localSourceControl(AbsolutePath)
+
+        /// A remote source package.
+        case remoteSourceControl(Foundation.URL)


What's the difference between fileSystem and localSourceControl? Do we do anything to ensure remoteSourceControl is indeed remote (e.g., what if URL scheme is file://?), or it doesn't matter?

tomerd · 2021-09-26T18:39:45Z

What's the difference between fileSystem and localSourceControl? Do we do anything to ensure remoteSourceControl is indeed remote (e.g., what if URL scheme is file://?), or it doesn't matter?

this is exactly one of the goals of this PR, fileSystem is a local dependency that is not necessarily a git repo, while localSourceControl must be a git repo. with this distinction the downstream code is much safe. we do not validate the prefix for URLs put into remoteSourceControl, that is a good idea to add

tomerd · 2021-09-26T18:53:21Z

@swift-ci please smoke test

tomerd · 2021-09-26T21:42:37Z

@swift-ci please test package compatibility

mattt · 2021-09-27T13:40:10Z

@mattt this is the second PR designed to help with landing SE-0292, I think I prefer to take this one even before #3698. wdyt?

If #3698 is ready to go, I think it'd be great to lock in our gains with that. But you're closer to the source material, so I defer to you on the best order of operations.

abertelrud · 2021-09-28T19:45:37Z

Sources/Commands/Describe.swift

-                self = .sourceControl(identity: settings.identity, location: settings.location, requirement: settings.requirement)
+                switch settings.location {
+                case .local(let path):
+                    self = .sourceControl(identity: settings.identity, location: path.pathString, requirement: settings.requirement)


Should we do a follow-on to separate out the described location similarly?

yes we could, less critical at this point imo

Yes, absolutely less critical.

abertelrud · 2021-09-28T19:47:34Z

Sources/PackageGraph/LocalPackageContainer.swift

+            case .root(let _path), .fileSystem(let _path):
+                path = _path
+            default:
+                throw InternalError("invalid package type \(package.kind)")


Could we add the package identity to the message here (or is it perhaps already part of the metadata?). That would make it much easier to try to figure out errors coming in from the field that have this message.

sure. this is an internal error so a programmer error and not user one, so less important IMO

abertelrud · 2021-09-28T19:48:30Z

Sources/PackageGraph/LocalPackageContainer.swift

+        case .root, .fileSystem:
+            break
+        default:
+            throw InternalError("invalid package type \(package.kind)")


Same comment as the other message. If it's already part of the context for whatever handler wraps this call, so it will show up as metadata for a diagnostic, then that's even better.

abertelrud · 2021-09-28T19:52:39Z

Sources/PackageModel/Manifest/PackageDependencyDescription.swift

+
+        public enum Location: Equatable {
+            case local(AbsolutePath)
+            case remote(Foundation.URL)


In the past I think we had some issues with Foundation representing git@... URLs, at least on Darwin. I agree that using URL instead of String here makes more sense conceptually, but do we think there might be side effects? We should of course report them if there, but this might be something to watch for.

this PR helps remove the logic that depends on checking if there is a scheme or not, and instead uses the type: localSourceControl vs remoteSourceControl, so ideally this should no longer be an issue regardless

I see. Yes, if SwiftPM is no longer asking those same questions but mainly using the URL as a container of URL-formatted strings, then that might go better.

yes this is the goal - make the concrete location opaque to logic, so it only gets used when we pass it to git, where we use absoluteString to write out the full value

is there a specific use case you recall that we need to add a test for to make sure this works correctly in that sense?

tomerd requested review from abertelrud, friedbunny and neonichu as code owners September 26, 2021 05:19

tomerd force-pushed the refactor/package-location-01 branch 5 times, most recently from c17e07d to d5cd15c Compare September 26, 2021 06:58

tomerd force-pushed the refactor/package-location-01 branch from d5cd15c to 51fe9f9 Compare September 26, 2021 06:59

tomerd commented Sep 26, 2021

View reviewed changes

Sources/SourceControl/Repository.swift

return url.absoluteString

}

}

}

Copy link

Contributor Author

tomerd Sep 26, 2021

Choose a reason for hiding this comment

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

👀 this new enum is another key change

cleanup

de989b6

tomerd force-pushed the refactor/package-location-01 branch from 9943305 to de989b6 Compare September 26, 2021 08:14

tomerd requested review from yim-lee and mattt September 26, 2021 08:15

tomerd self-assigned this Sep 26, 2021

linux

13694a9

tomerd added 2 commits September 26, 2021 10:18

cleanup

9bc5ed1

codable for 5.4

60f2ee7

tomerd commented Sep 26, 2021

View reviewed changes

yim-lee reviewed Sep 26, 2021

View reviewed changes

cleanup, safety check

055cac5

tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Sep 27, 2021

neonichu approved these changes Sep 28, 2021

View reviewed changes

abertelrud reviewed Sep 28, 2021

View reviewed changes

abertelrud self-requested a review September 28, 2021 19:57

abertelrud approved these changes Sep 28, 2021

View reviewed changes

tomerd merged commit 18c097d into swiftlang:main Sep 28, 2021

	/// - Parameter url: The package's URL.
	/// - Parameter urlString: The package's URL.

refactor location abstractions #3764

refactor location abstractions #3764

Uh oh!

Conversation

tomerd commented Sep 26, 2021

Uh oh!

tomerd Sep 26, 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 Sep 26, 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 commented Sep 26, 2021

Uh oh!

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

Uh oh!

Uh oh!

tomerd commented Sep 26, 2021

Uh oh!

tomerd commented Sep 26, 2021

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 Sep 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomerd commented Sep 26, 2021

Uh oh!

tomerd commented Sep 26, 2021

Uh oh!

mattt commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abertelrud Sep 28, 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!

Choose a reason for hiding this comment

Uh oh!

abertelrud Sep 28, 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

tomerd Sep 26, 2021 •

edited

Loading

tomerd Sep 26, 2021 •

edited

Loading

tomerd commented Sep 26, 2021 •

edited

Loading

tomerd commented Sep 26, 2021 •

edited

Loading

mattt commented Sep 27, 2021 •

edited

Loading

abertelrud Sep 28, 2021 •

edited

Loading

abertelrud Sep 28, 2021 •

edited

Loading