Skip to content

Commit 45573ef

Browse files
authored
Avoid loading duplicated packages if a root overrides a local dependency (#5550)
This was quite subtle. Normally, we resolve before calling `loadDependencyManifests` and roots will override any type of dependency in such a way that they don't become part of managed dependencies which means if we try to load via `loadManagedManifest`, we will get no result, essentially filtering them from dependency manifests. Now when requiring a resolved file, we are producing managed dependencies from the resolved file, plus we were adding entries for any local dependencies unconditionaly which would mean we would get results from `loadManagedManifest`, resulting in an additional entry in dependency manifests, compared to the "normal" mode. This PR changes the behavior such that we don't add a local dependency to managed dependencies if there's a root with the same identity which matches the results of the "normal" mode. Fixes rdar://92622234, rdar://83337879 (cherry picked from commit 378dc41)
1 parent 12828f7 commit 45573ef

File tree

2 files changed

+58
-5
lines changed

2 files changed

+58
-5
lines changed

Sources/Workspace/Workspace.swift

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1879,15 +1879,17 @@ extension Workspace {
18791879
let allManifestsWithPossibleDuplicates = try topologicalSort(input) { pair in
18801880
// optimization: preload manifest we know about in parallel
18811881
let dependenciesRequired = pair.item.dependenciesRequired(for: pair.key.productFilter)
1882-
// pre-populate managed dependencies if we are asked to do so
1883-
// FIXME: this seems like hack, needs further investigation why this is needed
1882+
let dependenciesToLoad = dependenciesRequired.map{ $0.createPackageRef() }.filter { !loadedManifests.keys.contains($0.identity) }
1883+
// pre-populate managed dependencies if we are asked to do so (this happens when resolving to a resolved file)
18841884
if automaticallyAddManagedDependencies {
1885-
try dependenciesRequired.filter { $0.isLocal }.forEach { dependency in
1886-
try self.state.dependencies.add(.fileSystem(packageRef: dependency.createPackageRef()))
1885+
try dependenciesToLoad.forEach { ref in
1886+
// Since we are creating managed dependencies based on the resolved file in this mode, but local packages aren't part of that file, they will be missing from it. So we're eagerly adding them here, but explicitly don't add any that are overridden by a root with the same identity since that would lead to loading the given package twice, once as a root and once as a dependency which violates various assumptions.
1887+
if case .fileSystem = ref.kind, !root.manifests.keys.contains(ref.identity) {
1888+
try self.state.dependencies.add(.fileSystem(packageRef: ref))
1889+
}
18871890
}
18881891
observabilityScope.trap { try self.state.save() }
18891892
}
1890-
let dependenciesToLoad = dependenciesRequired.map{ $0.createPackageRef() }.filter { !loadedManifests.keys.contains($0.identity) }
18911893
let dependenciesManifests = try temp_await { self.loadManagedManifests(for: dependenciesToLoad, observabilityScope: observabilityScope, completion: $0) }
18921894
dependenciesManifests.forEach { loadedManifests[$0.key] = $0.value }
18931895
return dependenciesRequired.compactMap { dependency in

Tests/WorkspaceTests/WorkspaceTests.swift

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4146,6 +4146,57 @@ final class WorkspaceTests: XCTestCase {
41464146
}
41474147
}
41484148

4149+
func testForceResolveToResolvedVersionsDuplicateLocalDependency() throws {
4150+
let sandbox = AbsolutePath("/tmp/ws/")
4151+
let fs = InMemoryFileSystem()
4152+
4153+
let workspace = try MockWorkspace(
4154+
sandbox: sandbox,
4155+
fileSystem: fs,
4156+
roots: [
4157+
MockPackage(
4158+
name: "Root",
4159+
targets: [
4160+
MockTarget(name: "Root", dependencies: ["Foo", "Bar"]),
4161+
],
4162+
products: [],
4163+
dependencies: [
4164+
.sourceControl(path: "./Foo", requirement: .upToNextMajor(from: "1.0.0")),
4165+
.fileSystem(path: "./Bar"),
4166+
]
4167+
),
4168+
MockPackage(
4169+
name: "Bar",
4170+
targets: [
4171+
MockTarget(name: "Bar"),
4172+
],
4173+
products: [
4174+
MockProduct(name: "Bar", targets: ["Bar"]),
4175+
]
4176+
),
4177+
],
4178+
packages: [
4179+
MockPackage(
4180+
name: "Foo",
4181+
targets: [
4182+
MockTarget(name: "Foo"),
4183+
],
4184+
products: [
4185+
MockProduct(name: "Foo", targets: ["Foo"]),
4186+
],
4187+
versions: ["1.0.0", "1.2.0", "1.3.2"]
4188+
),
4189+
]
4190+
)
4191+
4192+
try workspace.checkPackageGraph(roots: ["Root", "Bar"]) { _, diagnostics in
4193+
XCTAssertNoDiagnostics(diagnostics)
4194+
}
4195+
try workspace.checkPackageGraph(roots: ["Root", "Bar"], forceResolvedVersions: true) { _, diagnostics in
4196+
XCTAssertNoDiagnostics(diagnostics)
4197+
}
4198+
}
4199+
41494200
func testForceResolveWithNoResolvedFile() throws {
41504201
let sandbox = AbsolutePath("/tmp/ws/")
41514202
let fs = InMemoryFileSystem()

0 commit comments

Comments
 (0)