Skip to content

Commit e9c883d

Browse files
tomerdtomerd
authored
refactor PackageDependencyDescription towards identity changes (#3239)
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 c2f7eef commit e9c883d

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
@@ -199,7 +199,7 @@ extension PackageGraphError: CustomStringConvertible {
199199

200200
// If the package dependency name is the same as the package name, or if the product name and package name
201201
// don't correspond, we need to rewrite the target dependency to explicit specify the package name.
202-
if packageDependency.name == packageName || productName != packageName {
202+
if packageDependency.nameForTargetDependencyResolutionOnly == packageName || productName != packageName {
203203
solutionSteps.append("""
204204
reference the package in the target dependency with '.product(name: "\(productName)", package: \
205205
"\(packageName)")'
@@ -209,7 +209,7 @@ extension PackageGraphError: CustomStringConvertible {
209209
// If the name of the product and the package are the same, or if the package dependency implicit name
210210
// deduced from the URL is not correct, we need to rewrite the package dependency declaration to specify the
211211
// package name.
212-
if productName == packageName || packageDependency.name != packageName {
212+
if productName == packageName || packageDependency.nameForTargetDependencyResolutionOnly != packageName {
213213
let dependencySwiftRepresentation = packageDependency.swiftRepresentation(overridingName: packageName)
214214
solutionSteps.append("""
215215
provide the name of the package dependency with '\(dependencySwiftRepresentation)'
@@ -229,14 +229,14 @@ fileprivate extension PackageDependencyDescription {
229229
func swiftRepresentation(overridingName: String? = nil) -> String {
230230
var parameters: [String] = []
231231

232-
if let name = overridingName ?? explicitName {
232+
if let name = overridingName ?? self.explicitNameForTargetDependencyResolutionOnly {
233233
parameters.append("name: \"\(name)\"")
234234
}
235235

236236
if requirement == .localPackage {
237-
parameters.append("path: \"\(url)\"")
237+
parameters.append("path: \"\(self.location)\"")
238238
} else {
239-
parameters.append("url: \"\(url)\"")
239+
parameters.append("url: \"\(self.location)\"")
240240

241241
switch requirement {
242242
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
@@ -384,7 +384,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
384384
diagnostics: DiagnosticsEngine?
385385
) throws {
386386
let dependenciesByIdentity = Dictionary(grouping: manifest.dependencies, by: { dependency in
387-
PackageIdentity(url: dependency.url)
387+
PackageIdentity(url: dependency.location)
388388
})
389389

390390
let duplicateDependencyIdentities = dependenciesByIdentity
@@ -406,7 +406,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
406406
let duplicateDependencyNames = manifest.dependencies
407407
.lazy
408408
.filter({ !duplicateDependencies.contains($0) })
409-
.map({ $0.name })
409+
.map({ $0.nameForTargetDependencyResolutionOnly })
410410
.spm_findDuplicates()
411411

412412
for name in duplicateDependencyNames {
@@ -455,7 +455,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
455455
try diagnostics.emit(.unknownTargetPackageDependency(
456456
packageName: packageName ?? "unknown package name",
457457
targetName: target.name,
458-
validPackages: manifest.dependencies.map { $0.name }
458+
validPackages: manifest.dependencies.map { $0.nameForTargetDependencyResolutionOnly }
459459
))
460460
}
461461
case .byName(let name, _):
@@ -467,7 +467,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
467467
try diagnostics.emit(.unknownTargetDependency(
468468
dependency: name,
469469
targetName: target.name,
470-
validDependencies: manifest.dependencies.map { $0.name }
470+
validDependencies: manifest.dependencies.map { $0.nameForTargetDependencyResolutionOnly }
471471
))
472472
}
473473
}

‎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
@@ -185,12 +185,12 @@ public final class Manifest: ObjectIdentifierProtocol {
185185
for target in targetsRequired(for: products) {
186186
for targetDependency in target.dependencies {
187187
if let dependency = packageDependency(referencedBy: targetDependency) {
188-
requiredDependencyURLs.insert(dependency.url)
188+
requiredDependencyURLs.insert(dependency.location)
189189
}
190190
}
191191
}
192192

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

@@ -212,7 +212,7 @@ public final class Manifest: ObjectIdentifierProtocol {
212212
})
213213

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

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

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

230230
for target in targets {
231231
for targetDependency in target.dependencies {
@@ -244,7 +244,7 @@ public final class Manifest: ObjectIdentifierProtocol {
244244
}
245245

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

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

278278
/// 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)