Skip to content

Commit 6cfc566

Browse files
tomerdtomerd
committed
clarfiy reveresedVersions/versions in pubgrub
motivation: easier to reason about code changes: * rename PacakgeContainer::reversedVersions to PackageContainer::versionsDescending * add PacakgeContainer::versionsAscending to avoid the versionsDescending().reveresed() code * rename PacakgeContainer::versions(filter) to PackageContainer::toolsVersionsAppropriateVersionsDescending to better reflect the true meaning * remove filter from PackageContainer::toolsVersionsAppropriateVersionsDescending since its not used in practice * adjust tests and mocks
1 parent dfccfb1 commit 6cfc566

9 files changed

+69
-52
lines changed

‎Sources/PackageGraph/LocalPackageContainer.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,11 +89,11 @@ public class LocalPackageContainer: PackageContainer {
8989
fatalError("This should never be called")
9090
}
9191

92-
public func versions(filter isIncluded: (Version) -> Bool) throws -> AnySequence<Version> {
92+
public func toolsVersionsAppropriateVersionsDescending() throws -> [Version] {
9393
fatalError("This should never be called")
9494
}
9595

96-
public func reversedVersions() throws -> [Version] {
96+
public func versionsAscending() throws -> [Version] {
9797
fatalError("This should never be called")
9898
}
9999

‎Sources/PackageGraph/PackageContainer.swift

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,19 @@ public protocol PackageContainer {
4949
/// The list will be returned in sorted order, with the latest version *first*.
5050
/// All versions will not be requested at once. Resolver will request the next one only
5151
/// if the previous one did not satisfy all constraints.
52-
func versions(filter isIncluded: (Version) -> Bool) throws -> AnySequence<Version>
52+
func toolsVersionsAppropriateVersionsDescending() throws -> [Version]
53+
54+
/// Get the list of versions in the repository sorted in the ascending order, that is the earliest
55+
/// version appears first.
56+
func versionsAscending() throws -> [Version]
57+
58+
/// Get the list of versions in the repository sorted in the descending order, that is the latest
59+
/// version appears first.
60+
func versionsDescending() throws -> [Version]
5361

5462
/// Get the list of versions in the repository sorted in the reverse order, that is the latest
5563
/// version appears first.
64+
@available(*, deprecated, message: "use versionsDescending instead")
5665
func reversedVersions() throws -> [Version]
5766

5867
// FIXME: We should perhaps define some particularly useful error codes
@@ -90,6 +99,17 @@ public protocol PackageContainer {
9099
func getUpdatedIdentifier(at boundVersion: BoundVersion) throws -> PackageReference
91100
}
92101

102+
extension PackageContainer {
103+
@available(*, deprecated, message: "use versionsDescending instead")
104+
public func reversedVersions() throws -> [Version] {
105+
try self.versionsDescending()
106+
}
107+
108+
public func versionsDescending() throws -> [Version] {
109+
try self.versionsAscending().reversed()
110+
}
111+
}
112+
93113
// MARK: -
94114

95115
/// An individual constraint onto a container.

‎Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1094,7 +1094,7 @@ private final class PubGrubPackageContainer {
10941094
if let pinnedVersion = self.pinnedVersion, requirement.contains(pinnedVersion) {
10951095
return 1
10961096
}
1097-
return try self.underlying.reversedVersions().filter(requirement.contains).count
1097+
return try self.underlying.versionsDescending().filter(requirement.contains).count
10981098
}
10991099

11001100
/// Computes the bounds of the given range against the versions available in the package.
@@ -1105,7 +1105,7 @@ private final class PubGrubPackageContainer {
11051105
var includeLowerBound = true
11061106
var includeUpperBound = true
11071107

1108-
let versions = try self.underlying.reversedVersions()
1108+
let versions = try self.underlying.versionsDescending()
11091109

11101110
if let last = versions.last, range.lowerBound < last {
11111111
includeLowerBound = false
@@ -1131,13 +1131,13 @@ private final class PubGrubPackageContainer {
11311131
}
11321132

11331133
// Return the highest version that is allowed by the input requirement.
1134-
return try self.underlying.reversedVersions().first { versionSet.contains($0) }
1134+
return try self.underlying.versionsDescending().first { versionSet.contains($0) }
11351135
}
11361136

11371137
/// Compute the bounds of incompatible tools version starting from the given version.
11381138
private func computeIncompatibleToolsVersionBounds(fromVersion: Version) throws -> VersionSetSpecifier {
11391139
assert(!self.underlying.isToolsVersionCompatible(at: fromVersion))
1140-
let versions: [Version] = try self.underlying.reversedVersions().reversed()
1140+
let versions: [Version] = try self.underlying.versionsAscending()
11411141

11421142
// This is guaranteed to be present.
11431143
let idx = versions.firstIndex(of: fromVersion)!
@@ -1358,7 +1358,7 @@ private final class PubGrubPackageContainer {
13581358
return result
13591359
}
13601360

1361-
let versions: [Version] = try self.underlying.reversedVersions().reversed()
1361+
let versions: [Version] = try self.underlying.versionsAscending()
13621362

13631363
guard let idx = versions.firstIndex(of: firstVersion) else {
13641364
assertionFailure()

‎Sources/PackageGraph/RepositoryPackageContainer.swift

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
6767
private var dependenciesCacheLock = Lock()
6868

6969
private var knownVersionsCache = ThreadSafeBox<[Version: String]>()
70-
private var reversedVersionsCache = ThreadSafeBox<[Version]>()
7170
private var manifestsCache = ThreadSafeKeyValueStore<Revision, Manifest>()
7271
private var toolsVersionsCache = ThreadSafeKeyValueStore<Version, ToolsVersion>()
7372

@@ -106,17 +105,15 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
106105
})
107106
}
108107
}
109-
110-
public func reversedVersions() throws -> [Version] {
111-
try self.reversedVersionsCache.memoize() {
112-
[Version](try self.knownVersions().keys).sorted().reversed()
113-
}
108+
109+
public func versionsAscending() throws -> [Version] {
110+
[Version](try self.knownVersions().keys).sorted()
114111
}
115112

116113
/// The available version list (in reverse order).
117-
public func versions(filter isIncluded: (Version) -> Bool) throws -> AnySequence<Version> {
118-
let reversedVersions = try self.reversedVersions()
119-
return AnySequence(reversedVersions.filter(isIncluded).lazy.filter({
114+
public func toolsVersionsAppropriateVersionsDescending() throws -> [Version] {
115+
let reversedVersions = try self.versionsDescending()
116+
return reversedVersions.lazy.filter({
120117
// If we have the result cached, return that.
121118
if let result = self.validToolsVersionsCache[$0] {
122119
return result
@@ -126,7 +123,7 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
126123
let isValid = (try? self.toolsVersion(for: $0)).flatMap(self.isValidToolsVersion(_:)) ?? false
127124
self.validToolsVersionsCache[$0] = isValid
128125
return isValid
129-
}))
126+
})
130127
}
131128

132129
public func getTag(for version: Version) -> String? {

‎Sources/SPMTestSupport/MockPackageContainer.swift

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,11 @@ public class MockPackageContainer: PackageContainer {
3838
}
3939

4040
public let _versions: [Version]
41-
public func versions(filter isIncluded: (Version) -> Bool) throws -> AnySequence<Version> {
42-
return AnySequence(_versions.filter(isIncluded))
41+
public func toolsVersionsAppropriateVersionsDescending() throws -> [Version] {
42+
return try self.versionsDescending()
4343
}
4444

45-
public func reversedVersions() throws -> [Version] {
45+
public func versionsAscending() throws -> [Version] {
4646
return _versions
4747
}
4848

@@ -98,8 +98,7 @@ public class MockPackageContainer: PackageContainer {
9898
dependencies: [String: [Dependency]] = [:]
9999
) {
100100
self.name = name
101-
let versions = dependencies.keys.compactMap(Version.init(string:))
102-
self._versions = versions.sorted().reversed()
101+
self._versions = dependencies.keys.compactMap(Version.init(string:)).sorted()
103102
self.dependencies = dependencies
104103
}
105104
}

‎Sources/Workspace/ResolverPrecomputationProvider.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ private struct LocalPackageContainer: PackageContainer {
117117
}
118118
}
119119

120-
func reversedVersions() throws -> [Version] {
120+
func versionsAscending() throws -> [Version] {
121121
if let version = dependency?.state.checkout?.version {
122122
return [version]
123123
} else {
@@ -138,8 +138,8 @@ private struct LocalPackageContainer: PackageContainer {
138138
return currentToolsVersion
139139
}
140140

141-
func versions(filter isIncluded: (Version) -> Bool) throws -> AnySequence<Version> {
142-
return AnySequence(try self.reversedVersions())
141+
func toolsVersionsAppropriateVersionsDescending() throws -> [Version] {
142+
return try self.versionsDescending()
143143
}
144144

145145
func getDependencies(at version: Version, productFilter: ProductFilter) throws -> [PackageContainerConstraint] {

‎Sources/Workspace/Workspace.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2391,7 +2391,7 @@ extension Workspace {
23912391
// FIXME: this should not block
23922392
let container = try temp_await { containerProvider.getContainer(for: package, skipUpdate: true, on: self.queue, completion: $0) } as! RepositoryPackageContainer
23932393
guard let tag = container.getTag(for: version) else {
2394-
throw StringError("Internal error: please file a bug at https://bugs.swift.org with this info -- unable to get tag for \(package) \(version); available versions \(try container.reversedVersions())")
2394+
throw StringError("Internal error: please file a bug at https://bugs.swift.org with this info -- unable to get tag for \(package) \(version); available versions \(try container.versionsDescending())")
23952395
}
23962396
let revision = try container.getRevision(forTag: tag)
23972397
checkoutState = CheckoutState(revision: revision, version: version)

‎Tests/PackageGraphTests/PubgrubTests.swift

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1926,20 +1926,19 @@ public class MockContainer: PackageContainer {
19261926
var toolsVersion: ToolsVersion = ToolsVersion.currentToolsVersion
19271927
var versionsToolsVersions = [Version: ToolsVersion]()
19281928

1929-
public var _versions: [BoundVersion]
1929+
private var _versions: [BoundVersion]
19301930

1931-
public func versions(filter isIncluded: (Version) -> Bool) throws -> AnySequence<Version> {
1931+
// TODO: this does not actually do anything with the tools-version
1932+
public func toolsVersionsAppropriateVersionsDescending() throws -> [Version] {
19321933
var versions: [Version] = []
1933-
for version in self._versions {
1934+
for version in self._versions.reversed() {
19341935
guard case .version(let v) = version else { continue }
1935-
if isIncluded(v) {
1936-
versions.append(v)
1937-
}
1936+
versions.append(v)
19381937
}
1939-
return AnySequence(versions)
1938+
return versions
19401939
}
19411940

1942-
public func reversedVersions() throws -> [Version] {
1941+
public func versionsAscending() throws -> [Version] {
19431942
var versions: [Version] = []
19441943
for version in self._versions {
19451944
guard case .version(let v) = version else { continue }
@@ -1954,7 +1953,7 @@ public class MockContainer: PackageContainer {
19541953
return self.toolsVersion == toolsVersion
19551954
}
19561955

1957-
return (try? self.versions(filter: { _ in true }).contains(version)) ?? false
1956+
return (try? self.toolsVersionsAppropriateVersionsDescending().contains(version)) ?? false
19581957
}
19591958

19601959
public func toolsVersion(for version: Version) throws -> ToolsVersion {
@@ -1999,6 +1998,17 @@ public class MockContainer: PackageContainer {
19991998
return name
20001999
}
20012000

2001+
func appendVersion(_ version: BoundVersion) {
2002+
self._versions.append(version)
2003+
self._versions = self._versions
2004+
.sorted(by: { lhs, rhs -> Bool in
2005+
guard case .version(let lv) = lhs, case .version(let rv) = rhs else {
2006+
return true
2007+
}
2008+
return lv < rv
2009+
})
2010+
}
2011+
20022012
public convenience init(
20032013
name: PackageReference,
20042014
unversionedDependencies: [(package: PackageReference, requirement: PackageRequirement, productFilter: ProductFilter)]
@@ -2038,7 +2048,6 @@ public class MockContainer: PackageContainer {
20382048
let versions = dependencies.keys.compactMap(Version.init(string:))
20392049
self._versions = versions
20402050
.sorted()
2041-
.reversed()
20422051
.map(BoundVersion.version)
20432052
}
20442053
}
@@ -2114,15 +2123,7 @@ class DependencyGraphBuilder {
21142123
container.versionsToolsVersions[v] = toolsVersion ?? container.toolsVersion
21152124
}
21162125

2117-
container._versions.append(version)
2118-
container._versions = container._versions
2119-
.sorted(by: { lhs, rhs -> Bool in
2120-
guard case .version(let lv) = lhs, case .version(let rv) = rhs else {
2121-
return true
2122-
}
2123-
return lv < rv
2124-
})
2125-
.reversed()
2126+
container.appendVersion(version)
21262127

21272128
if container.dependencies[version.description] == nil {
21282129
container.dependencies[version.description] = [:]

‎Tests/PackageGraphTests/RepositoryPackageContainerProviderTests.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
180180

181181
let ref = PackageReference(identity: PackageIdentity(path: repoPath), path: repoPath.pathString)
182182
let container = try provider.getContainer(for: ref, skipUpdate: false)
183-
let v = try container.versions(filter: { _ in true }).map { $0 }
183+
let v = try container.toolsVersionsAppropriateVersionsDescending().map { $0 }
184184
XCTAssertEqual(v, ["2.0.3", "1.0.3", "1.0.2", "1.0.1", "1.0.0"])
185185
}
186186

@@ -235,7 +235,7 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
235235
let provider = createProvider(ToolsVersion(version: "4.0.0"))
236236
let ref = PackageReference(identity: PackageIdentity(url: specifier.url), path: specifier.url)
237237
let container = try provider.getContainer(for: ref, skipUpdate: false)
238-
let v = try container.versions(filter: { _ in true }).map { $0 }
238+
let v = try container.toolsVersionsAppropriateVersionsDescending().map { $0 }
239239
XCTAssertEqual(v, ["1.0.1"])
240240
}
241241

@@ -244,7 +244,7 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
244244
let ref = PackageReference(identity: PackageIdentity(url: specifier.url), path: specifier.url)
245245
let container = try provider.getContainer(for: ref, skipUpdate: false) as! RepositoryPackageContainer
246246
XCTAssertTrue(container.validToolsVersionsCache.isEmpty)
247-
let v = try container.versions(filter: { _ in true }).map { $0 }
247+
let v = try container.toolsVersionsAppropriateVersionsDescending().map { $0 }
248248
XCTAssertEqual(container.validToolsVersionsCache["1.0.0"], false)
249249
XCTAssertEqual(container.validToolsVersionsCache["1.0.1"], true)
250250
XCTAssertEqual(container.validToolsVersionsCache["1.0.2"], true)
@@ -256,7 +256,7 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
256256
let provider = createProvider(ToolsVersion(version: "3.0.0"))
257257
let ref = PackageReference(identity: PackageIdentity(url: specifier.url), path: specifier.url)
258258
let container = try provider.getContainer(for: ref, skipUpdate: false)
259-
let v = try container.versions(filter: { _ in true }).map { $0 }
259+
let v = try container.toolsVersionsAppropriateVersionsDescending().map { $0 }
260260
XCTAssertEqual(v, [])
261261
}
262262

@@ -312,7 +312,7 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
312312
)
313313
let ref = PackageReference(identity: PackageIdentity(path: repoPath), path: repoPath.pathString)
314314
let container = try provider.getContainer(for: ref, skipUpdate: false)
315-
let v = try container.versions(filter: { _ in true }).map { $0 }
315+
let v = try container.toolsVersionsAppropriateVersionsDescending().map { $0 }
316316
XCTAssertEqual(v, ["1.0.4-alpha", "1.0.2-dev.2", "1.0.2-dev", "1.0.1", "1.0.0", "1.0.0-beta.1", "1.0.0-alpha.1"])
317317
}
318318

@@ -352,7 +352,7 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
352352
)
353353
let ref = PackageReference(identity: PackageIdentity(path: repoPath), path: repoPath.pathString)
354354
let container = try provider.getContainer(for: ref, skipUpdate: false)
355-
let v = try container.versions(filter: { _ in true }).map { $0 }
355+
let v = try container.toolsVersionsAppropriateVersionsDescending().map { $0 }
356356
XCTAssertEqual(v, ["2.0.1", "1.0.4", "1.0.2", "1.0.1", "1.0.0"])
357357
}
358358

0 commit comments

Comments
 (0)