Skip to content

Commit 1b7138c

Browse files
tomerdtomerd
committed
make package.resolved deterministic in edge cases
motivation: in some cases like two URLs with casing differences, package.resolved could be non-deterministic changes: * make the maanged dependencies location lookup case insensitive * refactor how package.resolved is saved to test for non material differences * adjust call sites that manipulate the pin store to be more efficient about when to save the resolved file * add warning when expected dependency that should be stored in the resolved file is not found * adjust call sites TODO: add tests
1 parent 23c1486 commit 1b7138c

File tree

4 files changed

+196
-43
lines changed

4 files changed

+196
-43
lines changed

‎Sources/PackageGraph/PinsStore.swift

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,13 @@ public final class PinsStore {
101101
self._pins[pin.packageRef.identity] = pin
102102
}
103103

104+
/// Remove a pin.
105+
///
106+
/// This will replace any previous pin with same package name.
107+
public func remove(_ pin: Pin) {
108+
self._pins[pin.packageRef.identity] = nil
109+
}
110+
104111
/// Unpin all of the currently pinned dependencies.
105112
///
106113
/// This method does not automatically write to state file.

‎Sources/PackageModel/PackageReference.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ extension PackageReference: Equatable {
154154

155155
// TODO: consider rolling into Equatable
156156
public func equalsIncludingLocation(_ other: PackageReference) -> Bool {
157-
return self.identity == other.identity && self.kind.locationString == other.kind.locationString
157+
return self.identity == other.identity && self.kind.locationString.caseInsensitiveCompare(other.kind.locationString) == .orderedSame
158158
}
159159
}
160160

‎Sources/Workspace/Workspace.swift

Lines changed: 116 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -815,7 +815,7 @@ extension Workspace {
815815
let resolver = try self.createResolver(pinsMap: pinsMap, observabilityScope: observabilityScope)
816816
self.activeResolver = resolver
817817

818-
let updateResults = resolveDependencies(
818+
let updateResults = self.resolveDependencies(
819819
resolver: resolver,
820820
constraints: updateConstraints,
821821
observabilityScope: observabilityScope
@@ -836,8 +836,8 @@ extension Workspace {
836836
// Load the updated manifests.
837837
let updatedDependencyManifests = try self.loadDependencyManifests(root: graphRoot, observabilityScope: observabilityScope)
838838

839-
// Update the pins store.
840-
self.pinAll(
839+
// Update the resolved file.
840+
self.saveResolvedFile(
841841
pinsStore: pinsStore,
842842
dependencyManifests: updatedDependencyManifests,
843843
rootManifestsMinimumToolsVersion: rootManifestsMinimumToolsVersion,
@@ -1326,25 +1326,79 @@ extension Workspace {
13261326
// MARK: - Pinning Functions
13271327

13281328
extension Workspace {
1329-
13301329
/// Pins all of the current managed dependencies at their checkout state.
1331-
fileprivate func pinAll(
1330+
fileprivate func saveResolvedFile(
13321331
pinsStore: PinsStore,
13331332
dependencyManifests: DependencyManifests,
13341333
rootManifestsMinimumToolsVersion: ToolsVersion,
13351334
observabilityScope: ObservabilityScope
1336-
) {
1335+
) {
1336+
var dependenciesToPin = [ManagedDependency]()
1337+
let requiredDependencies = dependencyManifests.computePackages().required.filter({ $0.kind.isPinnable })
1338+
for dependency in requiredDependencies {
1339+
if let managedDependency = self.state.dependencies[comparingLocation: dependency] {
1340+
dependenciesToPin.append(managedDependency)
1341+
} else {
1342+
observabilityScope.emit(warning: "required dependency \(dependency.identity) (\(dependency.locationString)) was not found in managed dependencies and will not be recorded in resolved file")
1343+
}
1344+
}
1345+
1346+
// FIXME
1347+
// load the pin store from disk so we can compare for any changes
1348+
// this is needed as we want to avoid re-writing the resolved files
1349+
// unless absolutely required
1350+
let storedPinStore = try! self.pinsStore.load()
1351+
1352+
// compare for any differences between the existing state and the stored one
1353+
// subtle changes between versions of SwiftPM could treat URLs differently
1354+
// in which case we dont want to cause unnecessary churn
1355+
var needsUpdate = false
1356+
if dependenciesToPin.count != storedPinStore.pinsMap.count {
1357+
needsUpdate = true
1358+
} else {
1359+
for dependency in dependenciesToPin {
1360+
if !storedPinStore.pinsMap.contains(where: { $0.value.packageRef.equalsIncludingLocation(dependency.packageRef) }) {
1361+
needsUpdate = true
1362+
break
1363+
}
1364+
}
1365+
}
1366+
1367+
// exist early is there is nothing to do
1368+
guard needsUpdate else {
1369+
return
1370+
}
1371+
1372+
// reset the pinsStore and start pinning the required dependencies.
1373+
pinsStore.unpinAll()
1374+
for dependency in dependenciesToPin {
1375+
pinsStore.pin(dependency)
1376+
}
1377+
1378+
/*
13371379
// Reset the pinsStore and start pinning the required dependencies.
13381380
pinsStore.unpinAll()
13391381

1382+
let requiredDependencies = dependencyManifests.computePackages().required.filter({ $0.kind.isPinnable })
1383+
for dependency in requiredDependencies {
1384+
if let managedDependency = self.state.dependencies[comparingLocation: dependency] {
1385+
pinsStore.pin(managedDependency)
1386+
} else {
1387+
observabilityScope.emit(warning: "required dependency \(dependency.identity) (\(dependency.locationString)) was not found in managed dependencies and will not be recorded in resolved file")
1388+
}
1389+
}
1390+
*/
1391+
1392+
1393+
/*
13401394
let requiredDependencies = dependencyManifests.computePackages().required
13411395
for dependency in self.state.dependencies {
13421396
if requiredDependencies.contains(where: { $0.equalsIncludingLocation(dependency.packageRef) }) {
13431397
pinsStore.pin(dependency)
13441398
}
1345-
}
1399+
}*/
13461400

1347-
observabilityScope.trap{
1401+
observabilityScope.trap {
13481402
try pinsStore.saveState(toolsVersion: rootManifestsMinimumToolsVersion)
13491403
}
13501404

@@ -1398,7 +1452,7 @@ extension Workspace {
13981452
/// The dependency manifests in the transitive closure of root manifest.
13991453
let dependencies: [(manifest: Manifest, dependency: ManagedDependency, productFilter: ProductFilter, fileSystem: FileSystem)]
14001454

1401-
let workspace: Workspace
1455+
private let workspace: Workspace
14021456

14031457
fileprivate init(
14041458
root: PackageGraphRoot,
@@ -1649,7 +1703,7 @@ extension Workspace {
16491703
})
16501704

16511705
// optimization: preload first level dependencies manifest (in parallel)
1652-
let firstLevelDependencies = topLevelManifests.values.map { $0.dependencies.map{ $0.createPackageRef() } }.flatMap { $0 }
1706+
let firstLevelDependencies = topLevelManifests.values.map { $0.dependencies.map{ $0.createPackageRef() } }.flatMap({ $0 })
16531707
let firstLevelManifests = try temp_await { self.loadManagedManifests(for: firstLevelDependencies, observabilityScope: observabilityScope, completion: $0) } // FIXME: this should not block
16541708

16551709
// Continue to load the rest of the manifest for this graph
@@ -1677,7 +1731,7 @@ extension Workspace {
16771731
// dependencies that have the same identity but from a different location
16781732
// which is an error case we diagnose an report about in the GraphLoading part which
16791733
// is prepared to handle the case where not all manifest are available
1680-
$0.packageLocation == dependency.locationString ?
1734+
$0.packageLocation.caseInsensitiveCompare(dependency.locationString) == .orderedSame ?
16811735
KeyedPair($0, key: Key(identity: dependency.identity, productFilter: dependency.productFilter)) : nil
16821736
}
16831737
}
@@ -2524,10 +2578,18 @@ extension Workspace {
25242578
return currentManifests
25252579
}
25262580

2527-
self.validatePinsStore(dependencyManifests: currentManifests, rootManifestsMinimumToolsVersion: rootManifestsMinimumToolsVersion, observabilityScope: observabilityScope)
2581+
// load and update the pins store with any changes from loading the top level dependencies
2582+
guard let pinsStore = self.loadAndUpdatePinsStore(
2583+
dependencyManifests: currentManifests,
2584+
rootManifestsMinimumToolsVersion: rootManifestsMinimumToolsVersion,
2585+
observabilityScope: observabilityScope
2586+
) else {
2587+
// abort if PinsStore reported any errors.
2588+
return currentManifests
2589+
}
25282590

2529-
// Abort if pinsStore is unloadable or if diagnostics has errors.
2530-
guard !observabilityScope.errorsReported, let pinsStore = observabilityScope.trap({ try pinsStore.load() }) else {
2591+
// abort if PinsStore reported any errors.
2592+
guard !observabilityScope.errorsReported else {
25312593
return currentManifests
25322594
}
25332595

@@ -2551,6 +2613,15 @@ extension Workspace {
25512613

25522614
switch result {
25532615
case .notRequired:
2616+
// since nothing changed we can exit early,
2617+
// but need update resolved file and download an missing binary artifact
2618+
self.saveResolvedFile(
2619+
pinsStore: pinsStore,
2620+
dependencyManifests: currentManifests,
2621+
rootManifestsMinimumToolsVersion: rootManifestsMinimumToolsVersion,
2622+
observabilityScope: observabilityScope
2623+
)
2624+
25542625
try self.updateBinaryArtifacts(
25552626
manifests: currentManifests,
25562627
addedOrUpdatedPackages: [],
@@ -2601,7 +2672,8 @@ extension Workspace {
26012672
return updatedDependencyManifests
26022673
}
26032674

2604-
self.pinAll(
2675+
// Update the resolved file.
2676+
self.saveResolvedFile(
26052677
pinsStore: pinsStore,
26062678
dependencyManifests: updatedDependencyManifests,
26072679
rootManifestsMinimumToolsVersion: rootManifestsMinimumToolsVersion,
@@ -2784,43 +2856,32 @@ extension Workspace {
27842856
}
27852857

27862858
/// Validates that each checked out managed dependency has an entry in pinsStore.
2787-
private func validatePinsStore(
2859+
private func loadAndUpdatePinsStore(
27882860
dependencyManifests: DependencyManifests,
27892861
rootManifestsMinimumToolsVersion: ToolsVersion,
27902862
observabilityScope: ObservabilityScope
2791-
) {
2792-
guard let pinsStore = observabilityScope.trap({ try pinsStore.load() }) else {
2793-
return
2863+
) -> PinsStore? {
2864+
guard let pinsStore = observabilityScope.trap({ try self.pinsStore.load() }) else {
2865+
return nil
27942866
}
27952867

2796-
let pins = pinsStore.pinsMap.keys
2797-
let requiredDependencies = dependencyManifests.computePackages().required
2798-
2799-
for dependency in self.state.dependencies {
2800-
switch dependency.state {
2801-
case .sourceControlCheckout, .registryDownload: break
2802-
case .edited, .fileSystem, .custom: continue
2803-
}
2804-
2868+
let requiredDependencies = dependencyManifests.computePackages().required.filter({ $0.kind.isPinnable })
2869+
for dependency in self.state.dependencies.filter({ $0.packageRef.kind.isPinnable }) {
2870+
// a required dependency that is already loaded (managed) should be represented in the pins store.
28052871
// also comparing location as it may have changed at this point
28062872
if requiredDependencies.contains(where: { $0.equalsIncludingLocation(dependency.packageRef) }) {
2807-
// If required identity contains this dependency, it should be in the pins store.
2808-
// also comparing location as it may have changed at this point
2809-
if let pin = pinsStore.pinsMap[dependency.packageRef.identity], pin.packageRef.equalsIncludingLocation(dependency.packageRef) {
2810-
continue
2873+
let pin = pinsStore.pinsMap[dependency.packageRef.identity]
2874+
// if pin not found, or location is different (it may have changed at this point) pin it
2875+
if !(pin?.packageRef.equalsIncludingLocation(dependency.packageRef) ?? false) {
2876+
pinsStore.pin(dependency)
28112877
}
2812-
} else if !pins.contains(dependency.packageRef.identity) {
2813-
// Otherwise, it should *not* be in the pins store.
2814-
continue
2878+
} else if let pin = pinsStore.pinsMap[dependency.packageRef.identity] {
2879+
// otherwise, it should *not* be in the pins store.
2880+
pinsStore.remove(pin)
28152881
}
2816-
2817-
return self.pinAll(
2818-
pinsStore: pinsStore,
2819-
dependencyManifests: dependencyManifests,
2820-
rootManifestsMinimumToolsVersion: rootManifestsMinimumToolsVersion,
2821-
observabilityScope: observabilityScope
2822-
)
28232882
}
2883+
2884+
return pinsStore
28242885
}
28252886

28262887
/// This enum represents state of an external package.
@@ -3296,6 +3357,8 @@ extension Workspace {
32963357
try workingCopy.checkout(revision: checkoutState.revision)
32973358
try? fileSystem.chmod(.userUnWritable, path: checkoutPath, options: [.recursive, .onlyFiles])
32983359

3360+
print("********************* adding \(package.locationString) to managed deps")
3361+
32993362
// Write the state record.
33003363
self.state.dependencies.add(
33013364
try .sourceControlCheckout(
@@ -3557,6 +3620,17 @@ internal extension PackageReference {
35573620
}
35583621
}
35593622

3623+
fileprivate extension PackageReference.Kind {
3624+
var isPinnable: Bool {
3625+
switch self {
3626+
case .remoteSourceControl, .localSourceControl, .registry:
3627+
return true
3628+
default:
3629+
return false
3630+
}
3631+
}
3632+
}
3633+
35603634
// FIXME: remove this when remove the single call site that uses it
35613635
fileprivate extension PackageDependency {
35623636
var isLocal: Bool {

‎Tests/WorkspaceTests/WorkspaceTests.swift

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4094,6 +4094,78 @@ final class WorkspaceTests: XCTestCase {
40944094
}
40954095
}
40964096

4097+
func testManagedDependenciesNotCaseSensitive() throws {
4098+
let sandbox = AbsolutePath("/tmp/ws/")
4099+
let fs = InMemoryFileSystem()
4100+
4101+
let workspace = try MockWorkspace(
4102+
sandbox: sandbox,
4103+
fileSystem: fs,
4104+
roots: [
4105+
MockPackage(
4106+
name: "Foo",
4107+
targets: [
4108+
MockTarget(name: "Foo", dependencies: [
4109+
.product(name: "Bar", package: "bar"),
4110+
.product(name: "Baz", package: "baz")
4111+
])
4112+
],
4113+
products: [],
4114+
dependencies: [
4115+
.sourceControl(url: "https://localhost/bar", requirement: .upToNextMajor(from: "1.0.0")),
4116+
.sourceControl(url: "https://localhost/baz", requirement: .upToNextMajor(from: "1.0.0"))
4117+
]
4118+
),
4119+
],
4120+
packages: [
4121+
MockPackage(
4122+
name: "Bar",
4123+
url: "https://localhost/bar",
4124+
targets: [
4125+
MockTarget(name: "Bar", dependencies: [
4126+
.product(name: "Baz", package: "Baz")
4127+
]),
4128+
],
4129+
products: [
4130+
MockProduct(name: "Bar", targets: ["Bar"])
4131+
],
4132+
dependencies: [
4133+
.sourceControl(url: "https://localhost/Baz", requirement: .upToNextMajor(from: "1.0.0"))
4134+
],
4135+
versions: ["1.0.0"]
4136+
),
4137+
MockPackage(
4138+
name: "Baz",
4139+
url: "https://localhost/baz",
4140+
targets: [
4141+
MockTarget(name: "Baz"),
4142+
],
4143+
products: [
4144+
MockProduct(name: "Baz", targets: ["Baz"])
4145+
],
4146+
versions: ["1.0.0"]
4147+
),
4148+
]
4149+
)
4150+
4151+
try workspace.checkPackageGraph(roots: ["Foo"]) { graph, diagnostics in
4152+
PackageGraphTester(graph) { result in
4153+
result.check(roots: "Foo")
4154+
result.check(packages: "Bar", "Baz", "Foo")
4155+
result.checkTarget("Foo") { result in result.check(dependencies: "Bar", "Baz") }
4156+
result.checkTarget("Bar") { result in result.check(dependencies: "Baz") }
4157+
}
4158+
XCTAssertNoDiagnostics(diagnostics)
4159+
}
4160+
workspace.checkManagedDependencies { result in
4161+
result.check(dependency: "bar", at: .checkout(.version("1.0.0")))
4162+
result.check(dependency: "baz", at: .checkout(.version("1.0.0")))
4163+
XCTAssertEqual(result.managedDependencies[.plain("bar")]?.packageRef.locationString, "https://localhost/bar")
4164+
// root casing should win, so testing for lower case
4165+
XCTAssertEqual(result.managedDependencies[.plain("baz")]?.packageRef.locationString, "https://localhost/baz")
4166+
}
4167+
}
4168+
40974169
func testUnsafeFlags() throws {
40984170
let sandbox = AbsolutePath("/tmp/ws/")
40994171
let fs = InMemoryFileSystem()

0 commit comments

Comments
 (0)