Skip to content

Proposed identity improvement improvements #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Proposed identity improvement improvements #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2bb7125
Make all PackageReference initializers private
mattt Dec 30, 2020
0678b50
Remove identity parameter from PackageReference constructors
mattt Dec 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Sources/Commands/SwiftPackageCollectionsTool.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,7 @@ public struct SwiftPackageCollectionsTool: ParsableCommand {
}

mutating func run() throws {
let identity = PackageIdentity(url: self.packageUrl)
let reference = PackageReference.remote(identity: identity, location: self.packageUrl)
let reference = PackageReference.remote(location: self.packageUrl)

do { // assume URL is for a package
let result = try with { collections in
Expand Down
16 changes: 11 additions & 5 deletions Sources/PackageCollections/Utility.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import PackageModel
import SourceControl
import TSCBasic

struct MultipleErrors: Error {
let errors: [Error]
Expand Down Expand Up @@ -52,10 +53,15 @@ internal extension Result {
extension PackageReference {
/// Initializes a `PackageReference` from `RepositorySpecifier`
init(repository: RepositorySpecifier, kind: PackageReference.Kind = .remote) {
self.init(
identity: PackageIdentity(url: repository.url),
kind: kind,
location: repository.url
)
switch kind {
case .root:
let path = AbsolutePath(repository.url)
self = .root(path: path)
case .local:
let path = AbsolutePath(repository.url)
self = .local(path: path)
case .remote:
self = .remote(location: repository.url)
}
}
}
2 changes: 1 addition & 1 deletion Sources/PackageGraph/PackageGraphRoot.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public struct PackageGraphRoot {
public init(input: PackageGraphRootInput, manifests: [Manifest], explicitProduct: String? = nil) {
self.packageRefs = zip(input.packages, manifests).map { (path, manifest) in
let identity = PackageIdentity(url: manifest.url)
return PackageReference.root(identity: identity, path: path)
return PackageReference.root(path: path).with(alternateIdentity: identity)
}
self.manifests = manifests

Expand Down
14 changes: 8 additions & 6 deletions Sources/PackageGraph/PackageModel+Extensions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import PackageModel
import SourceControl
import TSCBasic

extension PackageDependencyDescription {
/// Create the package reference object for the dependency.
Expand All @@ -24,12 +25,13 @@ extension PackageDependencyDescription {
// as its identity, as it will be needed for supporting package
// registries.
let identity = PackageIdentity(url: effectiveURL)

return PackageReference(
identity: identity,
kind: requirement == .localPackage ? .local : .remote,
location: effectiveURL
)

if requirement == .localPackage {
let path = AbsolutePath(effectiveURL)
return PackageReference.local(path: path).with(alternateIdentity: identity)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we make progress on the identity changes my goal is to deprecate the idea of "alternate identity". as far as I can tell, the original goal of the "alternate identity" feature (it had a different name prior to swiftlang#3152) was to support an identity as driven by the name field in the manifest which is something that is too free-form to serve as an identity and we should deprecate. as such, my plan was to keep "alternate identity" for backwards compatibility for a while with a deprecation warning on the name field for manifest with new tools-version and restructure the code so its easy to remove when we are ready.

Copy link

@tomerd tomerd Jan 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to this end, we may want to consider a PackageReference.local(identity: PackageIdentity? = nil, path: AbsolutePath) or something similar

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecating this would be great — using the name causes so much confusion. I think it would make sense to have PackageReference.local(identity: PackageIdentity? = nil, path: AbsolutePath).

} else {
return PackageReference.remote(location: effectiveURL).with(alternateIdentity: identity)
}
}
}

Expand Down
10 changes: 1 addition & 9 deletions Sources/PackageGraph/PinsStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,22 +144,14 @@ extension PinsStore.Pin: JSONMappable, JSONSerializable {
throw InternalError("unknown location")
}

// backwards compatibility 12/2020
let identity: PackageIdentity
if let value: PackageIdentity = json.get("identity") {
identity = value
} else {
identity = PackageIdentity(url: location)
}

// backwards compatibility 12/2020
var alternateIdentity: PackageIdentity? = nil
if let value: PackageIdentity = json.get("alternate_identity") {
alternateIdentity = value
} else if let value: String = json.get("name") {
alternateIdentity = PackageIdentity(name: value)
}
let package = PackageReference.remote(identity: identity, location: location)
let package = PackageReference.remote(location: location)
self.packageRef = alternateIdentity.map{ package.with(alternateIdentity: $0) } ?? package
self.state = try json.get("state")
}
Expand Down
7 changes: 3 additions & 4 deletions Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,9 @@ public struct PubgrubDependencyResolver {

/// Execute the resolution algorithm to find a valid assignment of versions.
public func solve(constraints: [Constraint]) -> Result<[DependencyResolver.Binding], Error> {
let root = DependencyResolutionNode.root(package: .root(
identity: PackageIdentity(url: "<synthesized-root>"),
path: AbsolutePath("/synthesized-root-path")
))
let identity = PackageIdentity(url: "<synthesized-root>")
let path = AbsolutePath("/synthesized-root-path")
let root = DependencyResolutionNode.root(package: PackageReference.root(path: path).with(alternateIdentity: identity))

do {
// strips state
Expand Down
27 changes: 15 additions & 12 deletions Sources/PackageModel/PackageReference.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,7 @@ public struct PackageReference: Codable {
}
}

/// Create a package reference given its identity and repository.
public init(identity: PackageIdentity, kind: Kind, location: String) {
self.init(identity: identity, kind: kind, location: location, alternateIdentity: nil)
}

private init(identity: PackageIdentity, kind: Kind, location: String, alternateIdentity: PackageIdentity?) {
private init(identity: PackageIdentity, kind: Kind, location: String, alternateIdentity: PackageIdentity? = nil) {
self.identity = identity
self.location = location
self.kind = kind
Expand All @@ -69,23 +64,31 @@ public struct PackageReference: Codable {
}
}

public init(manifest: Manifest) {
let identity = PackageIdentity(url: manifest.url)
self.init(identity: identity, kind: manifest.packageKind, location: manifest.url)
}

// FIXME: the purpose of this is to allow identity override based on the identity in the manifest which is hacky
// this should be removed when we remove name from manifest
/// Create a new package reference object with the given identity.
public func with(alternateIdentity: PackageIdentity) -> PackageReference {
return PackageReference(identity: self.identity, kind: self.kind, location: self.location, alternateIdentity: alternateIdentity)
}

public static func root(identity: PackageIdentity, path: AbsolutePath) -> PackageReference {
PackageReference(identity: identity, kind: .root, location: path.pathString)
public static func root(path: AbsolutePath) -> PackageReference {
let identity = PackageIdentity(path: path)
return PackageReference(identity: identity, kind: .root, location: path.pathString)
}

public static func local(identity: PackageIdentity, path: AbsolutePath) -> PackageReference {
PackageReference(identity: identity, kind: .local, location: path.pathString)
public static func local(path: AbsolutePath) -> PackageReference {
let identity = PackageIdentity(path: path)
return PackageReference(identity: identity, kind: .local, location: path.pathString)
}

public static func remote(identity: PackageIdentity, location: String) -> PackageReference {
PackageReference(identity: identity, kind: .remote, location: location)
public static func remote(location: String) -> PackageReference {
let identity = PackageIdentity(url: location)
return PackageReference(identity: identity, kind: .remote, location: location)
}
}

Expand Down
4 changes: 2 additions & 2 deletions Sources/SPMTestSupport/MockDependencyGraph.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public extension MockDependencyGraph {
self.result = Dictionary(uniqueKeysWithValues: result.map { value in
let (container, version) = value
guard case .string(let str) = version else { fatalError() }
let package = PackageReference.remote(identity: PackageIdentity(url: container.lowercased()), location: "/\(container)")
let package = PackageReference.remote(location: "/\(container)")
return (package, Version(string: str)!)
})
self.name = name
Expand Down Expand Up @@ -91,7 +91,7 @@ private extension MockPackageContainer.Constraint {
guard case .string(let identifier)? = dict["identifier"] else { fatalError() }
guard let requirement = dict["requirement"] else { fatalError() }
let products: ProductFilter = try! JSON(dict).get("products")
let package = PackageReference.remote(identity: PackageIdentity(url: identifier), location: "")
let package = PackageReference.remote(location: "").with(alternateIdentity: PackageIdentity(url: identifier))
self.init(package: package, versionRequirement: VersionSetSpecifier(requirement), products: products)
}
}
Expand Down
4 changes: 2 additions & 2 deletions Sources/SPMTestSupport/MockPackageContainer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,11 @@ public class MockPackageContainer: PackageContainer {
var dependencies: [String: [Dependency]] = [:]
for (version, deps) in dependenciesByVersion {
dependencies[version.description] = deps.map {
let ref = PackageReference.remote(identity: PackageIdentity(url: $0.container), location: "/\($0.container)")
let ref = PackageReference.remote(location: "/\($0.container)")
return (ref, .versionSet($0.versionRequirement))
}
}
let ref = PackageReference.remote(identity: PackageIdentity(name: name), location: "/\(name)")
let ref = PackageReference.remote(location: "/\(name)")
self.init(package: ref, dependencies: dependencies)
}

Expand Down
7 changes: 1 addition & 6 deletions Sources/Workspace/ResolverPrecomputationProvider.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,7 @@ private struct LocalPackageContainer: PackageContainer {

// Gets the package reference from the managed dependency or computes it for root packages.
var identifier: PackageReference {
if let identifier = dependency?.packageRef {
return identifier
} else {
let identity = PackageIdentity(url: manifest.url)
return .root(identity: identity, path: manifest.path)
}
return dependency?.packageRef ?? .root(path: manifest.path)
}

func versionsAscending() throws -> [Version] {
Expand Down
54 changes: 27 additions & 27 deletions Sources/Workspace/Workspace.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1137,17 +1137,15 @@ extension Workspace {

var inputIdentities: Set<PackageReference> = []
let inputNodes: [GraphLoadingNode] = root.manifests.map({ manifest in
let identity = PackageIdentity(url: manifest.url)
let package = PackageReference(identity: identity, kind: manifest.packageKind, location: manifest.url)
let package = PackageReference(manifest: manifest)
inputIdentities.insert(package)
let node = GraphLoadingNode(manifest: manifest, productFilter: .everything)
return node
}) + root.dependencies.compactMap({ dependency in
let url = workspace.config.mirrors.effectiveURL(forURL: dependency.url)
let identity = PackageIdentity(url: url)
let package = PackageReference.remote(identity: identity, location: url)
let package = PackageReference.remote(location: url)
inputIdentities.insert(package)
guard let manifest = manifestsMap[identity] else { return nil }
guard let manifest = manifestsMap[package.identity] else { return nil }
let node = GraphLoadingNode(manifest: manifest, productFilter: dependency.productFilter)
return node
})
Expand All @@ -1156,19 +1154,31 @@ extension Workspace {
_ = transitiveClosure(inputNodes) { node in
return node.manifest.dependenciesRequired(for: node.productFilter).compactMap({ dependency in
let url = workspace.config.mirrors.effectiveURL(forURL: dependency.url)
let identity = PackageIdentity(url: url)
let package = PackageReference.remote(identity: identity, location: url)
let package = PackageReference.remote(location: url)
requiredIdentities.insert(package)
guard let manifest = manifestsMap[identity] else { return nil }
guard let manifest = manifestsMap[package.identity] else { return nil }
return GraphLoadingNode(manifest: manifest, productFilter: dependency.productFilter)
})
}
// FIXME: This should be an ordered set.
requiredIdentities = inputIdentities.union(requiredIdentities)

let availableIdentities: Set<PackageReference> = Set(manifestsMap.map({
let url = workspace.config.mirrors.effectiveURL(forURL: $0.1.url)
return PackageReference(identity: $0.key, kind: $0.1.packageKind, location: url)
let availableIdentities: Set<PackageReference> = Set(manifestsMap.map({ (identity, manifest) in
let url = workspace.config.mirrors.effectiveURL(forURL: manifest.url)

let package: PackageReference
switch manifest.packageKind {
case .root:
let path = AbsolutePath(url)
package = .root(path: path)
case .local:
let path = AbsolutePath(url)
package = .local(path: path)
case .remote:
package = .remote(location: url)
}

return package.with(alternateIdentity: identity)
}))
// We should never have loaded a manifest we don't need.
assert(availableIdentities.isSubset(of: requiredIdentities), "\(availableIdentities) | \(requiredIdentities)")
Expand All @@ -1187,16 +1197,11 @@ extension Workspace {
// resolver doesn't try to resolve it.
switch managedDependency.state {
case .edited:
// FIXME: We shouldn't need to construct a new package reference object here.
// We should get the correct one from managed dependency object.
let ref = PackageReference.local(
identity: managedDependency.packageRef.identity,
path: AbsolutePath(managedDependency.packageRef.location)
)
let constraint = PackageContainerConstraint(
package: ref,
package: managedDependency.packageRef,
requirement: .unversioned,
products: productFilter)
products: productFilter
)
allConstraints.append(constraint)
case .checkout, .local:
break
Expand All @@ -1219,16 +1224,11 @@ extension Workspace {
case .checkout, .local: continue
case .edited: break
}
// FIXME: We shouldn't need to construct a new package reference object here.
// We should get the correct one from managed dependency object.
let ref = PackageReference.local(
identity: managedDependency.packageRef.identity,
path: workspace.path(for: managedDependency)
)
let constraint = PackageContainerConstraint(
package: ref,
package: managedDependency.packageRef,
requirement: .unversioned,
products: productFilter)
products: productFilter
)
constraints.append(constraint)
}
return constraints
Expand Down
2 changes: 1 addition & 1 deletion Tests/PackageCollectionsTests/GitHubPackageMetadataProviderTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ class GitHubPackageMetadataProviderTests: XCTestCase {
func testInvalidRef() throws {
fixture(name: "Collections") { _ in
let provider = GitHubPackageMetadataProvider()
let reference = PackageReference.local(identity: .init(name: "test"), path:AbsolutePath("/"))
let reference = PackageReference.local(path: .root).with(alternateIdentity: PackageIdentity(name: "test"))
XCTAssertThrowsError(try tsc_await { callback in provider.get(reference, callback: callback) }, "should throw error") { error in
XCTAssertEqual(error as? GitHubPackageMetadataProvider.Errors, .invalidReferenceType(reference))
}
Expand Down
20 changes: 10 additions & 10 deletions Tests/PackageGraphTests/PubgrubTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,11 @@ private let v1_5Range: VersionSetSpecifier = .range(v1_5..<v2)
private let v1to3Range: VersionSetSpecifier = .range(v1..<v3)
private let v2Range: VersionSetSpecifier = .range(v2..<v3)

let aRef = PackageReference.local(identity: PackageIdentity(name: "a"), path: AbsolutePath("/"))
let bRef = PackageReference.local(identity: PackageIdentity(name: "b"), path: AbsolutePath("/"))
let cRef = PackageReference.local(identity: PackageIdentity(name: "c"), path: AbsolutePath("/"))
let aRef = PackageReference.local(path: .root).with(alternateIdentity: PackageIdentity(name: "a"))
let bRef = PackageReference.local(path: .root).with(alternateIdentity: PackageIdentity(name: "b"))
let cRef = PackageReference.local(path: .root).with(alternateIdentity: PackageIdentity(name: "c"))

let rootRef = PackageReference.root(identity: PackageIdentity(name: "root"), path: AbsolutePath("/"))
let rootRef = PackageReference.root(path: .root).with(alternateIdentity: PackageIdentity(name: "root"))
let rootNode = DependencyResolutionNode.root(package: rootRef)
let rootCause = try! Incompatibility(Term(rootNode, .exact(v1)), root: rootNode)
let _cause = try! Incompatibility("[email protected]", root: rootNode)
Expand Down Expand Up @@ -301,7 +301,7 @@ final class PubgrubTests: XCTestCase {

func testUpdatePackageIdentifierAfterResolution() {
let fooURL = "https://example.com/foo"
let fooRef = PackageReference.remote(identity: PackageIdentity(url: fooURL), location: fooURL)
let fooRef = PackageReference.remote(location: fooURL)
let foo = MockContainer(package: fooRef, dependenciesByVersion: [v1: [:]])
foo.updatedPackage = "bar"

Expand Down Expand Up @@ -395,7 +395,7 @@ final class PubgrubTests: XCTestCase {
root: rootNode), at: .topLevel)
state2.decide(rootNode, at: v1)
XCTAssertEqual(state2.solution.assignments.count, 1)
try solver2.propagate(state: state2, node: .root(package: .root(identity: PackageIdentity(name: "root"), path: AbsolutePath("/"))))
try solver2.propagate(state: state2, node: .root(package: PackageReference.root(path: .root).with(alternateIdentity: PackageIdentity(name: "root"))))
XCTAssertEqual(state2.solution.assignments.count, 2)
}

Expand Down Expand Up @@ -2084,7 +2084,7 @@ class DependencyGraphBuilder {
if let reference = self.references[packageName] {
return reference
}
let newReference = PackageReference.remote(identity: PackageIdentity(name: packageName), location: "/\(packageName)")
let newReference = PackageReference.remote(location: "/\(packageName)")
self.references[packageName] = newReference
return newReference
}
Expand Down Expand Up @@ -2190,7 +2190,7 @@ extension Term: ExpressibleByStringLiteral {
}

let name = components[0]
let packageReference = PackageReference.remote(identity: PackageIdentity(name: name), location: "")
let packageReference = PackageReference.remote(location: "").with(alternateIdentity: PackageIdentity(name: name))

guard case let .versionSet(vs) = requirement! else {
fatalError()
Expand All @@ -2203,12 +2203,12 @@ extension Term: ExpressibleByStringLiteral {

extension PackageReference: ExpressibleByStringLiteral {
public init(stringLiteral value: String) {
let ref = PackageReference.remote(identity: PackageIdentity(name: value), location: "")
let ref = PackageReference.remote(location: "").with(alternateIdentity: PackageIdentity(name: value))
self = ref
}

init(_ name: String) {
self = Self.remote(identity: PackageIdentity(name: name), location: "")
self = Self.remote(location: "").with(alternateIdentity: PackageIdentity(name: name))
}
}
extension Result where Success == [DependencyResolver.Binding] {
Expand Down
Loading