Add `AbsolutePath` and `SourceControlURL` parsing to `swift package add-dependency` #7769

hi2gage · 2024-07-11T04:01:48Z

Fix issue #7738
Allow user to add .package(path:) dependencies usingswift package add-dependency`

Motivation:

I wanted to be able to add .package(path:) using the command line and was unable to do so with the given parsing.

This PR updates the parsing logic to check if the dependency argument is a valid SourceControlURL or AbsolutePath.

Modifications:

Introduces a failable initializer to SourceControlURL allowing https://domain or git@domain URLs
Changes AddDependency to only require additional options if the dependency is the SourceControlURL format
Runs SwiftFormat in the files that were edited

Result:

User can run the following commands

`AbsolutePath`

swift package add-dependency /ChildPackage
> Updating package manifest at Package.swift... done.

resulting in

.package(path: "/ChildPackage")

`SourceControlURL`

swift-package add-dependency https://github.com/swiftlang/swift-syntax.git --branch main
> Updating package manifest at Package.swift... done.

resulting in

.package(url: "https://github.com/swiftlang/swift-syntax.git", branch: "main")

`RelativePath`

swift-package add-dependency ../ChildPackage
> error: relative paths not supported

Switched back to dependency Argument

bnbarham

Thank you @hi2gage! This LGTM generally, only have a small suggestion.

bnbarham · 2024-07-12T00:01:53Z

Sources/Commands/PackageCommands/AddDependency.swift

-                manifestContents = try fileSystem.readFileContents(manifestPath)
-            } catch {
-                throw StringError("cannot find package manifest in \(manifestPath)")
+            if let url = SourceControlURL(absoluteString: dependency) {


We also have a CanonicalPackageURL that @francescomikulis is changing a little in #7741. What if instead of the new SourceControlURL here, we use CanonicalPackageURL and then check if the scheme is either file or nil?

neonichu · 2024-07-12T00:11:19Z

I would very much caution against trying to parse URLs here and just let the user explicitly choose between URL- and path-based dependencies via arguments.

hi2gage · 2024-07-12T23:38:29Z

I would very much caution against trying to parse URLs here and just let the user explicitly choose between URL- and path-based dependencies via arguments.

@neonichu I brought that up as option 1 in the original issue #7738. I agree letting the user explicitly decide is better. There were concerns changing the command interface would not be following the accepted proposal (SE-0301 "Package Editing Commands")

More than willing to go through the proposal process if needed

jakepetroules · 2024-07-13T05:42:41Z

Sources/Basics/SourceControlURL.swift

+    /// e.g. https://github.com/apple/swift
+    /// e.g. git@github.com:apple/swift
+    public init?(absoluteString url: String) {
+        guard let regex = try? NSRegularExpression(pattern: "^(?:https://|git@)", options: .caseInsensitive) else {


Can this use Swift regex literals instead?

neonichu · 2024-07-15T16:00:29Z

More than willing to go through the proposal process if needed

That seems preferable since otherwise we're cutting off legitimate uses through the CLI, e.g.

.package(url: "/abcd", from: "1.0.0")

is a valid dependency.

bnbarham · 2024-07-15T17:56:36Z

Given:

That seems preferable since otherwise we're cutting off legitimate uses through the CLI, e.g.

And that the proposal states (not currently implemented):

If no requirement is specified, the command will default to a .upToNextMajor requirement on the latest version of the package

My vote would be for another argument that specifies adding it as a path dependency instead (this seems pretty inline with all the other version arguments, you could reasonably think of "path" as specifying the version, ie. "use local as is").

hi2gage · 2024-07-23T05:46:29Z

Finally getting back around to this PR. Sorry for the delay.

My vote would be for another argument that specifies adding it as a path dependency instead (this seems pretty inline with all the other version arguments, you could reasonably think of "path" as specifying the version, ie. "use local as is").

@bnbarham, Are you suggesting a boolean --path flag argument?
or a --type argument that accepts url, path, id similar to swift package add-target?

bnbarham · 2024-07-23T21:42:25Z

@bnbarham, Are you suggesting a boolean --path flag argument?
or a --type argument that accepts url, path, id similar to swift package add-target

I was suggesting a boolean and treating it in the same "group" as specifying a version. I forgot about the id case though, so maybe type does make more sense given that.

hi2gage · 2024-07-23T23:03:14Z

@bnbarham, Are you suggesting a boolean --path flag argument?
or a --type argument that accepts url, path, id similar to swift package add-target

I was suggesting a boolean and treating it in the same "group" as specifying a version. I forgot about the id case though, so maybe type does make more sense given that.

ha yeah I guess we've almost come full circle.
My personal vote would be instead of an additional type argument we should just have multiple arguments as shown below:
swift package add-dependency [--url <url>] [--path <path>] [--id <id>] [--exact <version>] [--revision <revision>] [--branch <branch>] [--from <version>] [--up-to-next-minor-from <version>]

Another benefit is that the command pattern follows the same pattern that the user would interact with using one of the
static func package(...) -> Package.Dependency inside of the Package.swift file.

This structure would allow the following commands:

swift package add-dependency path /ChildPackage
> Updating package manifest at Package.swift... done.

swift package add-dependency url /ChildPackage --from 1.0.0
> Updating package manifest at Package.swift... done.

swift package add-dependency url https://github.com/swiftlang/swift-syntax.git --branch main
> Updating package manifest at Package.swift... done.

swift package add-dependency id scope.name --from: 1.2.3
> Updating package manifest at Package.swift... done.

More than happy to discuss this offline to get alignment here

bnbarham · 2024-07-23T23:21:23Z

My personal vote would be instead of an additional type argument we should just have multiple arguments as shown below:

IMO the downside here is that it makes the 99% case more difficult (ie. URL).

More than happy to discuss this offline to get alignment here

Given all the different opinions, I definitely think this is worth bringing up on the forums for discussion.

hi2gage · 2024-07-24T00:51:04Z

IMO the downside here is that it makes the 99% case more difficult (ie. URL).

Yeah that's true. Plus a quick search through GitHub shows that the registry (id) adoption really isn't that high for public repos either.

I would be ok with this for the id

swift package add-dependency scope.name --from: 1.2.3 --type id
> Updating package manifest at Package.swift... done.

and

for path dependencies

swift package add-dependency /abcd --type path
> Updating package manifest at Package.swift... done.

I'll give this a go this evening. Given all of this dialog does it make sense to close this PR and create new one for this work?

bnbarham · 2024-07-24T00:53:06Z

Given all of this dialog does it make sense to close this PR and create new one for this work?

Either is okay. If you do make another PR, I'd reference this one.

hi2gage · 2024-07-24T04:51:30Z

Implemented changes in #7815

Fix swift package add-dependency for path dependency

ce71f81

Switched back to dependency Argument

hi2gage requested review from bnbarham, MaxDesiatov, jakepetroules, francescomikulis and xedin as code owners July 11, 2024 04:01

hi2gage mentioned this pull request Jul 11, 2024

Unable to add a valid .package(path:) dependency to Package.Swift via swift package add-dependency command #7738

Closed

1 task

Run SwiftFormat on changes

cf280c3

hi2gage force-pushed the main branch from 95e3d30 to cf280c3 Compare July 11, 2024 04:06

bnbarham reviewed Jul 12, 2024

View reviewed changes

jakepetroules reviewed Jul 13, 2024

View reviewed changes

Merge branch 'swiftlang:main' into main

d099c69

hi2gage mentioned this pull request Jul 24, 2024

Add --type argument to swift package add-dependency #7815

Merged

hi2gage closed this Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `AbsolutePath` and `SourceControlURL` parsing to `swift package add-dependency` #7769

Add `AbsolutePath` and `SourceControlURL` parsing to `swift package add-dependency` #7769

hi2gage commented Jul 11, 2024

bnbarham left a comment

bnbarham Jul 12, 2024

neonichu commented Jul 12, 2024

hi2gage commented Jul 12, 2024

jakepetroules Jul 13, 2024

neonichu commented Jul 15, 2024

bnbarham commented Jul 15, 2024

hi2gage commented Jul 23, 2024

bnbarham commented Jul 23, 2024

hi2gage commented Jul 23, 2024

bnbarham commented Jul 23, 2024

hi2gage commented Jul 24, 2024

bnbarham commented Jul 24, 2024

hi2gage commented Jul 24, 2024

Add AbsolutePath and SourceControlURL parsing to swift package add-dependency #7769

Add AbsolutePath and SourceControlURL parsing to swift package add-dependency #7769

Conversation

hi2gage commented Jul 11, 2024

Motivation:

Modifications:

Result:

AbsolutePath

SourceControlURL

RelativePath

bnbarham left a comment

Choose a reason for hiding this comment

bnbarham Jul 12, 2024

Choose a reason for hiding this comment

neonichu commented Jul 12, 2024

hi2gage commented Jul 12, 2024

jakepetroules Jul 13, 2024

Choose a reason for hiding this comment

neonichu commented Jul 15, 2024

bnbarham commented Jul 15, 2024

hi2gage commented Jul 23, 2024

bnbarham commented Jul 23, 2024

hi2gage commented Jul 23, 2024

bnbarham commented Jul 23, 2024

hi2gage commented Jul 24, 2024

bnbarham commented Jul 24, 2024

hi2gage commented Jul 24, 2024

Add `AbsolutePath` and `SourceControlURL` parsing to `swift package add-dependency` #7769

Add `AbsolutePath` and `SourceControlURL` parsing to `swift package add-dependency` #7769

`AbsolutePath`

`SourceControlURL`

`RelativePath`