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

hi2gage commented 3 months ago

Is it reproducible with SwiftPM command-line tools: `swift build`, `swift test`, `swift package` etc?

[X] Confirmed reproduction steps with SwiftPM CLI. The description text must include reproduction steps with either of command-line SwiftPM commands, swift build, swift test, swift package etc.

Description

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

If the user passes a valid path (AbsolutePath) The add-dependencycommand forces the user to add one of these options

When attempting to add a .package(path:) dependency to a Package.swift using the swift package add-dependency command, the command requires one of the following options:

--exact
--branch
--revision
--from
--up-to-next-minor-from

This is fine while trying to add a .package(url:from:) or .package(url:_:) but not for .package(path:) because no additional arguments are required.

If the user passes a valid AbsolutePath it will add a PackageDependency.SourceControl.Requirement to the .package(path:) making it an invalid Package.Dependency declaration

Original Swift-Evolution Proposal:

While investigating this bug I read through SE-0301 "Package Editing Commands" which includes the following information on this command:

swift package add-dependency <dependency> [--exact <version>] [--revision <revision>] [--branch <branch>] [--from <version>] [--up-to-next-minor-from <version>]

dependency: This may be the URL of a remote package, the path to a local package, or the name of a package in one of the user's package collections.

The following options can be used to specify a package dependency requirement:

--exact : Specifies a .exact() requirement in the manifest.
--revision : Specifies a .revision() requirement in the manifest.
--branch : Specifies a .branch() requirement in the manifest.
--up-to-next-minor-from : Specifies a .upToNextMinor() requirement in the manifest.
--from : Specifies a .upToNextMajor() requirement in the manifest when it appears alone. Optionally, --to may be added to specify a custom range requirement, or --through may be added to specify a custom closed range requirement.

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

Expected behavior

Expected to be able to pass in a path without having to add extra options in the CLI

The swift code generated should be

    dependencies: [
        .package(path: "/ChildPackage"),
    ],

Actual behavior

Forces user to selectiona additional options, then adds those options to the end of .package(path:)

    dependencies: [
        .package(path: "/ChildPackage", exact: "1.0.0"),
    ],

this breaks the Package.swift file

Steps to reproduce

clone the Xcode Project

git clone https://github.com/hi2gage/swift-package-manager-add-dependency-sample-project.git

Enter First Package Directory

cd swift-package-manager-add-dependency-sample-project/swift-package-manager-add-dependency-sample-project/LocalPackages/ParentPackage/

Now we want to add ChildPackage as a local package dependency to ParentPackage using the .package(path:)

swift package add-dependency /ChildPackage
> error: must specify one of --exact, --branch, --revision, --from, or --up-to-next-minor-from

So we pass in one of the options and see what happens

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

Open Package.swift Find that .package(path:exact:) is not a valid symbol

    dependencies: [
        .package(path: "/ChildPackage", exact: "1.0.0"),
    ],

Swift Package Manager version/commit hash

bf0272ca3

Swift & OS version (output of `swift --version ; uname -a`)

swift-driver version: 1.110 Apple Swift version 6.0 (swiftlang-6.0.0.4.52 clang-1600.0.21.1.3)
Target: arm64-apple-macosx14.0
Darwin MBP-M2 23.5.0 Darwin Kernel Version 23.5.0: Wed May  1 20:14:38 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T6020 arm64

Possible Solutions:

I spent some time working on fixing this issue and came up with two options.

Option 1:

Make the user decide what type of dependency they are creating: URL of a remote package, the path to a local package, or the name of a package in one of the user's package collections (implemented in the future for collections).

swift package add-dependency [--url <url>] [--path <path>] [--exact <version>] [--revision <revision>] [--branch <branch>] [--from <version>] [--up-to-next-minor-from <version>]

This allows for the user to be explicit with what type of dependency they want to add. This would make the command give better errors explaining why the user's intention is not possible:

Throws an error if neither --url nor --path is passed into the command
- Dependency must have a source
Throws an error if both --url and --path are passed into the command
- Dependency cannot have multiple sources
Throws an error if --url is passed with an invalid URL
- Remote URLs must be valid
Throws an error if --path is passed with an invalid AbsolutePath
- Local Paths must be valid

Option 2:

Keep the command interface the same but only require additional options if the <dependency> is not a valid AbsolutePath

swift package add-dependency <dependency> [--exact <version>] [--revision <revision>] [--branch <branch>] [--from <version>] [--up-to-next-minor-from <version>]

This would allow for the command interface to remain as it currently is and requires fewer options to be passed into the command.

Recommendation:

There are pros and cons to both solutions, but I find the verbosity of Option 1 to be superior even if the command interface needs to change slightly. By forcing the user to decide what type of dependency they want to add to the Package.swift, the command is able to parse the values more safely and throw dependency-specific errors such as invalid path or invalid URL.

If we went with Option 2, the user needs to understand that a path must be a AbsolutePath which always starts with a / character. It could be extremely confusing if the user wants to pass in a relative path instead of an absolute path and they keep getting a .package(url:_:) with the relative path.

Thanks for reading and hope this helps!

hi2gage commented 3 months ago

@DougGregor

Hey Doug thanks for all of the work on this feature I would love to get some feedback from you on this issue. I almost have PR ready for Option 1 so let me know how I should proceed.

hi2gage commented 3 months ago

Side note: It would also be awesome to be able to pass in RelativePath or AbsolutePath. Not sure if this is the time to add that functionality or not.

DougGregor commented 3 months ago

My inclination would be something more like Option 2, since it follows most closely with the accepted proposal and is (I think?) more intuitive. We can detect a URL using the URL initializer (returns nil if it's not a URL), and detect a path by the presence of / or \, and anything else would be a package in the collection. Am I missing some reason why that wouldn't work? @MaxDesiatov do you have an opinion on the interface here?

hi2gage commented 2 months ago

Yeah I agree that it follows the proposal better. I went ahead and implemented Option 2. But I found that URL?(string: String) is not strict enough. URL?(string: "/coolPackage" returns a valid URL so we can't use it to determine if it's a valid URL.

I instead opted for using SourceControlURL see #7769

Also I didn't see anything in the proposal regarding package collections. Is that something that should be addressed here?

swiftlang / swift-package-manager