Skip to content

Commit a6f29d5

Browse files
tomerdtomerd
committed
refactor PackageDependencyDescription towards identity changes
motivation: improve code infrastrucuture towards fixing correctness issues with identity changes: * rename PackageDependencyDescription::url to PackageDependencyDescription::location to reflect its true meaning * rename PackageDependencyDescription::name to PackageDependencyDescription::nameForTargetDependencyResolutionOnly to make sure its not used for wrong reasons * move "file://" prefix handing from PackageDependencyDescription::init to PackageDependencyDescription::init(json) which where all the other location validation is done * addjsut call-sites * adjust and add tests
1 parent 91fac80 commit a6f29d5

26 files changed

+216
-221
lines changed

‎Sources/Commands/Describe.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,8 @@ fileprivate struct DescribedPackage: Encodable {
105105
let requirement: PackageDependencyDescription.Requirement?
106106

107107
init(from dependency: PackageDependencyDescription) {
108-
self.name = dependency.explicitName
109-
self.url = dependency.url
108+
self.name = dependency.explicitNameForTargetDependencyResolutionOnly
109+
self.url = dependency.location
110110
self.requirement = dependency.requirement
111111
}
112112
}

‎Sources/PackageGraph/PackageGraph+Loading.swift

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ extension PackageGraph {
4343
let manifestMap = Dictionary(uniqueKeysWithValues: manifestMapSequence)
4444
let successors: (GraphLoadingNode) -> [GraphLoadingNode] = { node in
4545
node.requiredDependencies().compactMap({ dependency in
46-
let url = mirrors.effectiveURL(forURL: dependency.url)
46+
let url = mirrors.effectiveURL(forURL: dependency.location)
4747
return manifestMap[PackageIdentity(url: url)].map { manifest in
4848
GraphLoadingNode(manifest: manifest, productFilter: dependency.productFilter)
4949
}
@@ -53,11 +53,11 @@ extension PackageGraph {
5353
// Construct the root manifest and root dependencies set.
5454
let rootManifestSet = Set(root.manifests)
5555
let rootDependencies = Set(root.dependencies.compactMap({
56-
manifestMap[PackageIdentity(url: $0.url)]
56+
manifestMap[PackageIdentity(url: $0.location)]
5757
}))
5858
let rootManifestNodes = root.manifests.map { GraphLoadingNode(manifest: $0, productFilter: .everything) }
5959
let rootDependencyNodes = root.dependencies.lazy.compactMap { (dependency: PackageDependencyDescription) -> GraphLoadingNode? in
60-
guard let manifest = manifestMap[PackageIdentity(url: dependency.url)] else { return nil }
60+
guard let manifest = manifestMap[PackageIdentity(url: dependency.location)] else { return nil }
6161
return GraphLoadingNode(manifest: manifest, productFilter: dependency.productFilter)
6262
}
6363
let inputManifests = rootManifestNodes + rootDependencyNodes
@@ -223,11 +223,11 @@ private func createResolvedPackages(
223223
var dependencies = [ResolvedPackageBuilder]()
224224
// Establish the manifest-declared package dependencies.
225225
package.manifest.dependenciesRequired(for: packageBuilder.productFilter).forEach { dependency in
226-
let dependencyURL = mirrors.effectiveURL(forURL: dependency.url)
226+
let dependencyURL = mirrors.effectiveURL(forURL: dependency.location)
227227
let dependencyIdentity = PackageIdentity(url: dependencyURL)
228228

229229
// Use the package name to lookup the dependency. The package name will be present in packages with tools version >= 5.2.
230-
if let explicitDependencyName = dependency.explicitName, let resolvedPackage = packageMapByName[explicitDependencyName] {
230+
if let explicitDependencyName = dependency.explicitNameForTargetDependencyResolutionOnly, let resolvedPackage = packageMapByName[explicitDependencyName] {
231231
guard !dependencies.contains(resolvedPackage) else {
232232
// check if this resolvedPackage already listed in the dependencies
233233
// this means that the dependencies share the same name
@@ -258,7 +258,7 @@ private func createResolvedPackages(
258258
return diagnostics.emit(error, location: diagnosticLocation)
259259
}
260260
// check that the explicit package dependency name matches the package name.
261-
if let explicitDependencyName = dependency.explicitName, resolvedPackage.package.name != explicitDependencyName {
261+
if let explicitDependencyName = dependency.explicitNameForTargetDependencyResolutionOnly, resolvedPackage.package.name != explicitDependencyName {
262262
// check if this resolvedPackage url is the same as the dependency one
263263
// if not, this means that the dependencies share the same identity
264264
// FIXME: this works but the way we find out about this is based on a side effect, need to improve it when working on identity
@@ -417,15 +417,15 @@ private func createResolvedPackages(
417417
let referencedPackageURL = mirrors.effectiveURL(forURL: product.packageBuilder.package.manifest.packageLocation)
418418
let referencedPackageIdentity = PackageIdentity(url: referencedPackageURL)
419419
guard let packageDependency = (packageBuilder.package.manifest.dependencies.first { package in
420-
let packageURL = mirrors.effectiveURL(forURL: package.url)
420+
let packageURL = mirrors.effectiveURL(forURL: package.location)
421421
let packageIdentity = PackageIdentity(url: packageURL)
422422
return packageIdentity == referencedPackageIdentity
423423
}) else {
424424
throw InternalError("dependency reference for \(referencedPackageURL) not found")
425425
}
426426

427427
let packageName = product.packageBuilder.package.name
428-
if productRef.name != packageDependency.name || packageDependency.name != packageName {
428+
if productRef.name != packageDependency.nameForTargetDependencyResolutionOnly || packageDependency.nameForTargetDependencyResolutionOnly != packageName {
429429
let error = PackageGraphError.productDependencyMissingPackage(
430430
productName: productRef.name,
431431
targetName: targetBuilder.target.name,

‎Sources/PackageGraph/PackageGraph.swift

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ extension PackageGraphError: CustomStringConvertible {
210210

211211
// If the package dependency name is the same as the package name, or if the product name and package name
212212
// don't correspond, we need to rewrite the target dependency to explicit specify the package name.
213-
if packageDependency.name == packageName || productName != packageName {
213+
if packageDependency.nameForTargetDependencyResolutionOnly == packageName || productName != packageName {
214214
solutionSteps.append("""
215215
reference the package in the target dependency with '.product(name: "\(productName)", package: \
216216
"\(packageName)")'
@@ -220,7 +220,7 @@ extension PackageGraphError: CustomStringConvertible {
220220
// If the name of the product and the package are the same, or if the package dependency implicit name
221221
// deduced from the URL is not correct, we need to rewrite the package dependency declaration to specify the
222222
// package name.
223-
if productName == packageName || packageDependency.name != packageName {
223+
if productName == packageName || packageDependency.nameForTargetDependencyResolutionOnly != packageName {
224224
let dependencySwiftRepresentation = packageDependency.swiftRepresentation(overridingName: packageName)
225225
solutionSteps.append("""
226226
provide the name of the package dependency with '\(dependencySwiftRepresentation)'
@@ -240,14 +240,14 @@ fileprivate extension PackageDependencyDescription {
240240
func swiftRepresentation(overridingName: String? = nil) -> String {
241241
var parameters: [String] = []
242242

243-
if let name = overridingName ?? explicitName {
243+
if let name = overridingName ?? self.explicitNameForTargetDependencyResolutionOnly {
244244
parameters.append("name: \"\(name)\"")
245245
}
246246

247247
if requirement == .localPackage {
248-
parameters.append("path: \"\(url)\"")
248+
parameters.append("path: \"\(self.location)\"")
249249
} else {
250-
parameters.append("url: \"\(url)\"")
250+
parameters.append("url: \"\(self.location)\"")
251251

252252
switch requirement {
253253
case .branch(let branch):

‎Sources/PackageGraph/PackageModel+Extensions.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import SourceControl
1414
extension PackageDependencyDescription {
1515
/// Create the package reference object for the dependency.
1616
public func createPackageRef(mirrors: DependencyMirrors) -> PackageReference {
17-
let effectiveURL = mirrors.effectiveURL(forURL: url)
17+
let effectiveURL = mirrors.effectiveURL(forURL: self.location)
1818

1919
// FIXME: The identity of a package dependency is currently based on
2020
// on a name computed from the package's effective URL. This

‎Sources/PackageLoading/ManifestLoader.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -451,7 +451,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
451451
diagnostics: DiagnosticsEngine?
452452
) throws {
453453
let dependenciesByIdentity = Dictionary(grouping: manifest.dependencies, by: { dependency in
454-
PackageIdentity(url: dependency.url)
454+
PackageIdentity(url: dependency.location)
455455
})
456456

457457
let duplicateDependencyIdentities = dependenciesByIdentity
@@ -473,7 +473,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
473473
let duplicateDependencyNames = manifest.dependencies
474474
.lazy
475475
.filter({ !duplicateDependencies.contains($0) })
476-
.map({ $0.name })
476+
.map({ $0.nameForTargetDependencyResolutionOnly })
477477
.spm_findDuplicates()
478478

479479
for name in duplicateDependencyNames {
@@ -522,7 +522,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
522522
try diagnostics.emit(.unknownTargetPackageDependency(
523523
packageName: packageName ?? "unknown package name",
524524
targetName: target.name,
525-
validPackages: manifest.dependencies.map { $0.name }
525+
validPackages: manifest.dependencies.map { $0.nameForTargetDependencyResolutionOnly }
526526
))
527527
}
528528
case .byName(let name, _):
@@ -534,7 +534,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
534534
try diagnostics.emit(.unknownTargetDependency(
535535
dependency: name,
536536
targetName: target.name,
537-
validDependencies: manifest.dependencies.map { $0.name }
537+
validDependencies: manifest.dependencies.map { $0.nameForTargetDependencyResolutionOnly }
538538
))
539539
}
540540
}

‎Sources/PackageLoading/PackageDescription4Loader.swift

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -309,40 +309,43 @@ extension PackageDependencyDescription.Requirement {
309309

310310
extension PackageDependencyDescription {
311311
fileprivate init(v4 json: JSON, toolsVersion: ToolsVersion, packageLocation: String, fileSystem: FileSystem) throws {
312-
let isBaseURLRemote = URL.scheme(packageLocation) != nil
312+
let filePrefix = "file://"
313313

314-
func fixLocation(_ location: String, requirement: Requirement) throws -> String {
314+
func fixLocation(_ dependencyLocation: String) throws -> String {
315315
// If base URL is remote (http/ssh), we can't do any "fixing".
316-
if isBaseURLRemote {
317-
return location
316+
if URL.scheme(packageLocation) != nil {
317+
return dependencyLocation
318318
}
319319

320-
// If the dependency URL starts with '~/', try to expand it.
321-
if location.hasPrefix("~/") {
322-
return fileSystem.homeDirectory.appending(RelativePath(String(location.dropFirst(2)))).pathString
323-
}
324-
325-
// If the dependency URL is not remote, try to "fix" it.
326-
if URL.scheme(location) == nil {
320+
if dependencyLocation.hasPrefix("~/") {
321+
// If the dependency URL starts with '~/', try to expand it.
322+
return fileSystem.homeDirectory.appending(RelativePath(String(dependencyLocation.dropFirst(2)))).pathString
323+
} else if dependencyLocation.hasPrefix(filePrefix) {
324+
// FIXME: SwiftPM can't handle file locations with file:// scheme so we need to
325+
// strip that. We need to design a Location data structure for SwiftPM.
326+
return AbsolutePath(String(dependencyLocation.dropFirst(filePrefix.count))).pathString
327+
} else if URL.scheme(dependencyLocation) == nil {
328+
// If the dependency URL is not remote, try to "fix" it.
327329
// If the URL has no scheme, we treat it as a path (either absolute or relative to the base URL).
328-
return AbsolutePath(location, relativeTo: AbsolutePath(packageLocation)).pathString
329-
}
330-
331-
if case .localPackage = requirement {
332-
do {
333-
return try AbsolutePath(validating: location).pathString
334-
} catch PathValidationError.invalidAbsolutePath(let path) {
335-
throw ManifestParseError.invalidManifestFormat("'\(path)' is not a valid path for path-based dependencies; use relative or absolute path instead.", diagnosticFile: nil)
336-
}
330+
return AbsolutePath(dependencyLocation, relativeTo: AbsolutePath(packageLocation)).pathString
337331
}
338332

339-
return location
333+
return dependencyLocation
340334
}
341335

342-
let requirement = try Requirement(v4: json.get("requirement"))
343-
let location = try fixLocation(json.get("url"), requirement: requirement)
336+
let location = try fixLocation(json.get("url"))
344337
let name: String? = json.get("name")
345-
self.init(name: name, url: location, requirement: requirement)
338+
let requirement = try Requirement(v4: json.get("requirement"))
339+
340+
if case .localPackage = requirement {
341+
do {
342+
_ = try AbsolutePath(validating: location)
343+
} catch PathValidationError.invalidAbsolutePath(let path) {
344+
throw ManifestParseError.invalidManifestFormat("'\(path)' is not a valid path for path-based dependencies; use relative or absolute path instead.", diagnosticFile: nil)
345+
}
346+
}
347+
348+
self.init(name: name, location: location, requirement: requirement)
346349
}
347350
}
348351

‎Sources/PackageModel/Manifest.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -184,12 +184,12 @@ public final class Manifest: ObjectIdentifierProtocol {
184184
for target in targetsRequired(for: products) {
185185
for targetDependency in target.dependencies {
186186
if let dependency = packageDependency(referencedBy: targetDependency) {
187-
requiredDependencyURLs.insert(dependency.url)
187+
requiredDependencyURLs.insert(dependency.location)
188188
}
189189
}
190190
}
191191

192-
return dependencies.filter { requiredDependencyURLs.contains($0.url) }
192+
return dependencies.filter { requiredDependencyURLs.contains($0.location) }
193193
#endif
194194
}
195195

@@ -211,7 +211,7 @@ public final class Manifest: ObjectIdentifierProtocol {
211211
})
212212

213213
let requiredTargetNames = Set(productTargetNames).union(dependentTargetNames)
214-
let requiredTargets = requiredTargetNames.compactMap({ targetsByName[$0] })
214+
let requiredTargets = requiredTargetNames.compactMap{ targetsByName[$0] }
215215
return requiredTargets
216216
}
217217

@@ -224,7 +224,7 @@ public final class Manifest: ObjectIdentifierProtocol {
224224
) -> [PackageDependencyDescription] {
225225

226226
var registry: (known: [String: ProductFilter], unknown: Set<String>) = ([:], [])
227-
let availablePackages = Set(dependencies.lazy.map({ $0.name }))
227+
let availablePackages = Set(dependencies.lazy.map{ $0.nameForTargetDependencyResolutionOnly })
228228

229229
for target in targets {
230230
for targetDependency in target.dependencies {
@@ -243,7 +243,7 @@ public final class Manifest: ObjectIdentifierProtocol {
243243
}
244244

245245
return dependencies.compactMap { dependency in
246-
if let filter = associations[dependency.name] {
246+
if let filter = associations[dependency.nameForTargetDependencyResolutionOnly] {
247247
return dependency.filtered(by: filter)
248248
} else if keepUnused {
249249
// Register that while the dependency was kept, no products are needed.
@@ -271,7 +271,7 @@ public final class Manifest: ObjectIdentifierProtocol {
271271
return nil
272272
}
273273

274-
return dependencies.first(where: { $0.name == packageName })
274+
return dependencies.first(where: { $0.nameForTargetDependencyResolutionOnly == packageName })
275275
}
276276

277277
/// Registers a required product with a particular dependency if possible, or registers it as unknown.

‎Sources/PackageModel/Manifest/PackageDependencyDescription.swift

Lines changed: 14 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -30,19 +30,14 @@ public struct PackageDependencyDescription: Equatable, Codable, Hashable {
3030
}
3131
}
3232

33-
/// The name of the package dependency explicitly defined in the manifest, if any.
34-
public let explicitName: String?
33+
/// An explicit name set by the user, to be used *only* for target dependencies resolution
34+
public let explicitNameForTargetDependencyResolutionOnly: String?
3535

36-
/// The name of the packagedependency,
37-
/// either explicitly defined in the manifest,
38-
/// or derived from the URL.
39-
///
40-
/// - SeeAlso: `explicitName`
41-
/// - SeeAlso: `url`
42-
public let name: String
36+
/// A computed name to be used *only* for target dependencies resolution
37+
public let nameForTargetDependencyResolutionOnly: String
4338

44-
/// The url of the package dependency.
45-
public let url: String
39+
/// The location of the package dependency.
40+
public let location: String
4641

4742
/// The dependency requirement.
4843
public let requirement: Requirement
@@ -53,30 +48,23 @@ public struct PackageDependencyDescription: Equatable, Codable, Hashable {
5348
/// Create a package dependency.
5449
public init(
5550
name: String? = nil,
56-
url: String,
51+
location: String,
5752
requirement: Requirement,
5853
productFilter: ProductFilter = .everything
5954
) {
60-
// FIXME: SwiftPM can't handle file URLs with file:// scheme so we need to
61-
// strip that. We need to design a URL data structure for SwiftPM.
62-
let filePrefix = "file://"
63-
let normalizedURL: String
64-
if url.hasPrefix(filePrefix) {
65-
normalizedURL = AbsolutePath(String(url.dropFirst(filePrefix.count))).pathString
66-
} else {
67-
normalizedURL = url
68-
}
69-
70-
self.explicitName = name
71-
self.name = name ?? LegacyPackageIdentity.computeDefaultName(fromURL: normalizedURL)
72-
self.url = normalizedURL
55+
self.explicitNameForTargetDependencyResolutionOnly = name
56+
self.nameForTargetDependencyResolutionOnly = name ?? LegacyPackageIdentity.computeDefaultName(fromURL: location)
57+
self.location = location
7358
self.requirement = requirement
7459
self.productFilter = productFilter
7560
}
7661

7762
/// Returns a new package dependency with the specified products.
7863
public func filtered(by productFilter: ProductFilter) -> PackageDependencyDescription {
79-
PackageDependencyDescription(name: explicitName, url: url, requirement: requirement, productFilter: productFilter)
64+
PackageDependencyDescription(name: self.explicitNameForTargetDependencyResolutionOnly,
65+
location: self.location,
66+
requirement: self.requirement,
67+
productFilter: productFilter)
8068
}
8169
}
8270

0 commit comments

Comments
 (0)