swiftlang / swift-package-manager

The Package Manager for the Swift Programming Language
Apache License 2.0
9.72k stars 1.34k forks source link

Allow relative path with `swift package add-dependency` command #7871

Open hi2gage opened 1 month ago

hi2gage commented 1 month ago

Enable passing relative path into swift package add-dependency

Motivation:

Currently only AbsolutePaths are supported with --type path flag

swift package add-dependency /packagePath --type path

There is a need to support RelativePath because it's not uncommon to have a package structure inside of an xcodeproj as shown below.

LocalPackages
├── ChildPackage
│   ├── .gitignore
│   ├── .swiftpm
│   ├── Package.swift
│   ├── Sources
│   └── Tests
└── ParentPackage
    ├── .build
    ├── .gitignore
    ├── .swiftpm
    ├── Package.swift
    ├── Sources
    └── Tests

If we want to open ParentPackage by itself, it will not be able to resolve that package.

This pr allows for the user to add a package with a RelativePath path via the swift package add-dependency --type path command.

Access level for

were upgraded from private to package allowing access to these functions from the PackageCommands module.

Modifications:

Result:

Both of the following commands are valid

swift package add-dependency ../relative --type path
swift package add-dependency /absolute --type path
hi2gage commented 1 month ago

@bnbarham Just wanted to bump this.

hi2gage commented 3 weeks ago

Thanks @hi2gage! Definitely valuable to support relative paths here.

Due to access level and not wanting to make certain enums public, I opted to use MappablePackageDependency instead of Package.Dependency to represent the values coming from the CLI command.

Which enums would those be? Ideally MappablePackageDependency would not be public, it really shouldn't be visible to commands.

@bnbarham No problem!

I thought I would need to make Package.Dependency.SourceControlRequirement and Package.Dependency.RegistryRequirement public, but I avoided doing so because of these comments:

// intentionally private to hide enum detail
private static func package(
        name: String? = nil,
        url: String,
        requirement: Package.Dependency.SourceControlRequirement
    ) -> Package.Dependency {
        return .init(name: name, location: url, requirement: requirement, traits: nil)
    }

but looking back over this again I realized this was written prior to the package access level being introduced. Any reason to not move these 3 functions from private -> package?

It would really simplify this PR to not use MappablePackageDependency as you mentioned.

bnbarham commented 2 weeks ago

I thought I would need to make Package.Dependency.SourceControlRequirement and Package.Dependency.RegistryRequirement public, but I avoided doing so because of these comments:

Aren't they already 🤔? I assume the issue was that there's no way to create a Dependency with them today, but couldn't you call the existing initializers (which end up mapping to these)?

Any reason to not move these 3 functions from private -> package?

If they need to be (depending on the above) - they just can't be public as this is the package manifest API.

hi2gage commented 2 weeks ago

Hey @bnbarham I just pushed up the changes to use Package.Dependency so we can talk specifics.

I thought I would need to make Package.Dependency.SourceControlRequirement and Package.Dependency.RegistryRequirement public, but I avoided doing so because of these comments:

Aren't they already 🤔?

Yes they are public 🤦I meant to say: I thought I would need to make the Dependency initializers that contain those enums public. Not the enums themselves public.

I assume the issue was that there's no way to create a Dependency with them today, but couldn't you call the existing initializers (which end up mapping to these)?

Technically I could call the existing initializers, but it requires some funky switching. The changes I just pushed uses the existing initializers. So let me know what you think about that.

This being said if I made those initializers package access level I could take this code:

let packageDependency: PackageDescription.Package.Dependency
switch (firstRequirement, to) {
case (let .exact(version), nil):
    packageDependency = .package(url: self.dependency, exact: version)
case (let .range(range), let to?):
    packageDependency = .package(url: self.dependency, (range.lowerBound ..< to))
case (let .range(range), nil):
    packageDependency = .package(url: self.dependency, range)
case (let .revision(revision), nil):
    packageDependency = .package(url: self.dependency, revision: revision)
case (let .branch(branch), nil):
    packageDependency = .package(url: self.dependency, branch: branch)
case (.branch, _?), (.revision, _?), (.exact, _?):
    throw StringError("--to can only be specified with --from or --up-to-next-minor-from")
case (_, _):
    throw StringError("unknown requirement")
}

->

let packageDependency2: PackageDescription.Package.Dependency = .package(
    url: self.dependency,
    requirement: requirement,
    traits: []
)

Any reason to not move these 3 functions from private -> package?

If they need to be (depending on the above) - they just can't be public as this is the package manifest API.

Agreed, they can't be public, making them package would be helpful but we can get around it if you want to keep those initializers locked down.

bnbarham commented 1 week ago

I'd be fine making them package, especially to avoid that switch 😅 . @dschaefer2 / @plemarquand any opinions?

hi2gage commented 1 day ago

I'd be fine making them package, especially to avoid that switch 😅 . @dschaefer2 / @plemarquand any opinions?

Great! I just pushed up changes making those package.