Implement local dependencies proposal #1583
Conversation
|
This is mostly complete but needs actual tests and I need to make sure I didn't miss any edge case. @hartbit I'll appreciate if you can review the code |
aciidgh
force-pushed
the
local-packages
branch
10 times, most recently
from
9ddac0b to
40c7587
Compare
|
@swift-ci please smoke test |
aciidgh
changed the title
<rdar://problem/39418745> [SR-7433]: Implement SE-201 Package Manager Local Dependencies
|
@swift-ci please smoke test |
| @@ -24,6 +24,12 @@ extension Package.Dependency: Equatable { | |||
| url: String, | |||
| _ requirement: Package.Dependency.Requirement | |||
| ) -> Package.Dependency { | |||
| // FIXME: This is suboptimal but its the only way to do this right now. | |||
There was a problem hiding this comment.
It seems weird to encode local dependency as requirement. Could we have made dependency into an enum:
public enum Dependency {
case remote(RemoteDependency)
case local(LocalDependency)
}
public class RemoteDependency {
/// The dependency requirement.
public enum Requirement {
case exactItem(Version)
case rangeItem(Range<Version>)
case revisionItem(String)
case branchItem(String)
}
/// The url of the dependency.
public let url: String
/// The dependency requirement.
public let requirement: Requirement
/// Create a dependency.
init(url: String, requirement: Requirement) {
self.url = url
self.requirement = requirement
}
}
public class LocalDependency {
public let path: String
}There was a problem hiding this comment.
Possibly...I plan to revisit these data structure as part of upcoming runtime refactor.
There was a problem hiding this comment.
Also note that we can't change the public facing data structures.
There was a problem hiding this comment.
Can’t we change the public facing data structures as part of a 4.2 PD?
| @@ -17,6 +17,7 @@ public enum DependencyResolverError: Error, Equatable, CustomStringConvertible { | |||
| case unsatisfiable | |||
|
|
|||
| /// The resolver encountered a versioned container which has a revision dependency. | |||
| // FIXME: Rename this to incompatible constraints. | |||
There was a problem hiding this comment.
Didn't you want to rename it now?
| @@ -133,6 +134,9 @@ extension PackageDescription4.Package.Dependency.Requirement { | |||
|
|
|||
| case .exactItem(let version): | |||
| return .versionSet(.exact(Version(pdVersion: version))) | |||
|
|
|||
| case .localPackageItem: | |||
There was a problem hiding this comment.
I'm still confused as to why we're representing local packages through Requirement. Is this the only solution?
There was a problem hiding this comment.
Requirement is correct though as it is a requirement.
| guard case .checkout(let currentState) = dependency.state else { | ||
| let error = WorkspaceDiagnostics.DependencyAlreadyInEditMode(dependencyName: packageName) | ||
| return diagnostics.emit(error) | ||
| // FIXME: This should be "can resolve" and not "can edit". |
There was a problem hiding this comment.
Is there a reason we're not renaming it now?
There was a problem hiding this comment.
Nope, just a low-priority change.
rdar://problem/39418745 [SR-7433]: Implement SE-201 Package Manager Local Dependencies