Allow relative path with swift package add-dependency command

[hi2gage]

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

• .package(name:url:requirement:traits:)
• .package(name:url:requirement:)
• .package(id:requirement:traits:)

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

Modifications:

• Enable passing relative path into swift package add-dependency
• Add unit test coverage

Result:

Both of the following commands are valid

swift package add-dependency ../relative --type path

swift package add-dependency /absolute --type path

[hi2gage]

@bnbarham Just wanted to bump this.

[bnbarham]

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.

[hi2gage]

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]

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]

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]

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

[hi2gage]

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.

[hi2gage]

@bnbarham Just wanted to bump this, let me know if there is anything I can to help move this PR along.

[dschaefer2]

@swift-ci please test

[dschaefer2]

@swift-ci please test windows

[dschaefer2]

@hi2gage that did the trick. Running the tests now. I'm a little worried about adding the PackageDescription dependency since it was really only intended for the package manifest processing, but it actually makes sense how you're using it.

This is actually pretty timely. We've been working at improving support for monorepos where relative packages are common. This will help setting them up.

[dschaefer2]

Hmm, the toolchain build in the smoke tests and the Windows builds are failing. Look like the new dependency on SwiftSyntax requires a change to the CMake files (which are used to build swiftpm in these environments).

[bnbarham]

Hmm, the toolchain build in the smoke tests and the Windows builds are failing. Look like the new dependency on SwiftSyntax requires a change to the CMake files (which are used to build swiftpm in these environments).

It's just that PackageDescription+Syntax.swift needs to be added to the cmake build (and the old PackageDependency+Syntax.swift removed).

[hi2gage]

Hmm, the toolchain build in the smoke tests and the Windows builds are failing. Look like the new dependency on SwiftSyntax requires a change to the CMake files (which are used to build swiftpm in these environments).

It's just that PackageDescription+Syntax.swift needs to be added to the cmake build (and the old PackageDependency+Syntax.swift removed).

Just pushed the changes to the cmake build

[hi2gage]

@hi2gage that did the trick. Running the tests now. I'm a little worried about adding the PackageDescription dependency since it was really only intended for the package manifest processing, but it actually makes sense how you're using it.

This is actually pretty timely. We've been working at improving support for monorepos where relative packages are common. This will help setting them up.

Yup, we have a monorepo at work so that's why I was motivated to fix this problem. I'm glad that I'm able to help out. If there are other issues / chunks of work in this realm let me know, I've really enjoyed contributing to SPM.

[bnbarham]

@swift-ci please test

[hi2gage]

@swift-ci please test windows

[dschaefer2]

@swift-ci please test windows

[hi2gage]

@swift-ci please test windows

[dschaefer2]

@swift-ci please test

[dschaefer2]

@swift-ci please test windows

[dschaefer2]

I'm not sure why the Windows build is breaking. It uses the CMakeLists file.

[hi2gage]

I'm not sure why the Windows build is breaking. It uses the CMakeLists file.

Yeah me neither I thought that bc39a72 would have fixed this error.

C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swiftpm\Sources\PackageModelSyntax\AddPackageDependency.swift:14:8: error: no such module 'PackageDescription'

Is there a windows VM image or sorts to run this locally?

[dschaefer2]

Looks like a missing dependency in the CMakeLists.txt file.

[dschaefer2]

I'm trying on my Windows machine. There is a Utilities/bootstrap utility that builds using CMake. I couldn't reproduce it on my Mac, which is surprising.

[dschaefer2]

I'm confused by this. There is no library called ProductDescription. Did you mean PackageDescription here?

[hi2gage]

Yeah not sure what I was thinking here, I mixed these up. Just pushed change to PackageDescription

[dschaefer2]

@swift-ci please test

[dschaefer2]

@swift-ci please test windows