Skip to content

Commit

Permalink
[PackageGraph] Fix package name validation for product target depende…
Browse files Browse the repository at this point in the history
…ncies

The package name validation was wrong for product target dependencies
(`.product(name: "<product>", package: "<package>")`) as we were looking
up the package using its identity instead of the name. This completely
breaks package loading in 5.2 if a package wants to use a product from
a dependency that doesn't match the package name of that dependency.

<rdar://problem/59821906>

(cherry picked from commit e3a880b)
  • Loading branch information
aciidgh committed Feb 27, 2020
1 parent cabb453 commit f2318da
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 7 deletions.
20 changes: 13 additions & 7 deletions Sources/PackageGraph/PackageGraphLoader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -265,20 +265,26 @@ private func createResolvedPackages(
})

// Create a map of package builders keyed by the package identity.
let packageMap: [String: ResolvedPackageBuilder] = packageBuilders.spm_createDictionary({
// FIXME: This shouldn't be needed once <rdar://problem/33693433> is fixed.
let packageMapByIdentity: [String: ResolvedPackageBuilder] = packageBuilders.spm_createDictionary{
let identity = PackageReference.computeIdentity(packageURL: $0.package.manifest.url)
return (identity, $0)
})
}
let packageMapByName: [String: ResolvedPackageBuilder] = packageBuilders.spm_createDictionary{ ($0.package.name, $0) }

// In the first pass, we wire some basic things.
for packageBuilder in packageBuilders {
let package = packageBuilder.package

// Establish the manifest-declared package dependencies.
packageBuilder.dependencies = package.manifest.allRequiredDependencies.compactMap({ dependency in
packageBuilder.dependencies = package.manifest.allRequiredDependencies.compactMap { dependency in
// Use the package name to lookup the dependency. The package name will be present in packages with tools version >= 5.2.
if let dependencyName = dependency.explicitName, let resolvedPackage = packageMapByName[dependencyName] {
return resolvedPackage
}

// Otherwise, look it up by its identity.
let url = config.mirroredURL(forURL: dependency.url)
let resolvedPackage = packageMap[PackageReference.computeIdentity(packageURL: url)]
let resolvedPackage = packageMapByIdentity[PackageReference.computeIdentity(packageURL: url)]

// We check that the explicit package dependency name matches the package name.
if let resolvedPackage = resolvedPackage,
Expand All @@ -294,7 +300,7 @@ private func createResolvedPackages(
}

return resolvedPackage
})
}

// Create target builders for each target in the package.
let targetBuilders = package.targets.map({ ResolvedTargetBuilder(target: $0, diagnostics: diagnostics) })
Expand Down Expand Up @@ -390,7 +396,7 @@ private func createResolvedPackages(
if let packageName = productRef.package {
// Find the declared package and check that it contains
// the product we found above.
guard let dependencyPackage = packageMap[packageName.lowercased()], dependencyPackage.products.contains(product) else {
guard let dependencyPackage = packageMapByName[packageName], dependencyPackage.products.contains(product) else {
let error = PackageGraphError.productDependencyIncorrectPackage(
name: productRef.name, package: packageName)
diagnostics.emit(error, location: diagnosticLocation())
Expand Down
39 changes: 39 additions & 0 deletions Tests/PackageGraphTests/PackageGraphTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,45 @@ class PackageGraphTests: XCTestCase {
}
}

func testPackageNameValidationInProductTargetDependency() throws {
let fs = InMemoryFileSystem(emptyFiles:
"/Foo/Sources/Foo/foo.swift",
"/Bar/Sources/Bar/bar.swift"
)

let diagnostics = DiagnosticsEngine()
_ = loadPackageGraph(fs: fs, diagnostics: diagnostics,
manifests: [
Manifest.createManifest(
name: "Foo",
path: "/Foo",
url: "/Foo",
v: .v5_2,
packageKind: .root,
dependencies: [
PackageDependencyDescription(name: "UnBar", url: "/Bar", requirement: .branch("master")),
],
targets: [
TargetDescription(name: "Foo", dependencies: [.product(name: "BarProduct", package: "UnBar")]),
]),
Manifest.createV4Manifest(
name: "UnBar",
path: "/Bar",
url: "/Bar",
packageKind: .remote,
products: [
ProductDescription(name: "BarProduct", targets: ["Bar"])
],
targets: [
TargetDescription(name: "Bar"),
]),
]
)

// Expect no diagnostics.
DiagnosticsEngineTester(diagnostics) { _ in }
}

func testUnusedDependency() throws {
let fs = InMemoryFileSystem(emptyFiles:
"/Foo/Sources/Foo/foo.swift",
Expand Down
1 change: 1 addition & 0 deletions Tests/PackageGraphTests/XCTestManifests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ extension PackageGraphTests {
("testInvalidExplicitPackageDependencyName", testInvalidExplicitPackageDependencyName),
("testMultipleDuplicateModules", testMultipleDuplicateModules),
("testNestedDuplicateModules", testNestedDuplicateModules),
("testPackageNameValidationInProductTargetDependency", testPackageNameValidationInProductTargetDependency),
("testProductDependencies", testProductDependencies),
("testProductDependencyNotFound", testProductDependencyNotFound),
("testProductDependencyNotFoundImprovedDiagnostic", testProductDependencyNotFoundImprovedDiagnostic),
Expand Down

0 comments on commit f2318da

Please sign in to comment.