From e143dc508dbe72d25a20eb95ea5c0158b16b7b15 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Thu, 18 Jul 2024 11:22:03 -0700 Subject: [PATCH 1/3] [PackageGraph] Add an accessor to check whether Term and VersionSetSpecifier support prerelease versions --- .../Resolution/PubGrub/Term.swift | 4 +++ .../PackageGraph/VersionSetSpecifier.swift | 25 +++++++++++++++++++ .../VersionSetSpecifierTests.swift | 25 +++++++++++++++++++ 3 files changed, 54 insertions(+) diff --git a/Sources/PackageGraph/Resolution/PubGrub/Term.swift b/Sources/PackageGraph/Resolution/PubGrub/Term.swift index ebd6c6f0c8f..7d020cc3b56 100644 --- a/Sources/PackageGraph/Resolution/PubGrub/Term.swift +++ b/Sources/PackageGraph/Resolution/PubGrub/Term.swift @@ -40,6 +40,10 @@ public struct Term: Equatable, Hashable { ) } + package var supportsPrereleases: Bool { + self.requirement.supportsPrereleases + } + /// Check if this term satisfies another term, e.g. if `self` is true, /// `other` must also be true. public func satisfies(_ other: Term) -> Bool { diff --git a/Sources/PackageGraph/VersionSetSpecifier.swift b/Sources/PackageGraph/VersionSetSpecifier.swift index cdf27ddd805..b9526294ceb 100644 --- a/Sources/PackageGraph/VersionSetSpecifier.swift +++ b/Sources/PackageGraph/VersionSetSpecifier.swift @@ -466,6 +466,21 @@ extension VersionSetSpecifier { } } +extension VersionSetSpecifier { + package var supportsPrereleases: Bool { + switch self { + case .empty, .any: + false + case .exact(let version): + version.supportsPrerelease + case .range(let range): + range.supportsPrereleases + case .ranges(let ranges): + ranges.contains(where: \.supportsPrereleases) + } + } +} + extension VersionSetSpecifier: CustomStringConvertible { public var description: String { switch self { @@ -505,4 +520,14 @@ fileprivate extension Range where Bound == Version { func isHigherThan(_ other: Range) -> Bool { return other.isLowerThan(self) } + + var supportsPrereleases: Bool { + self.lowerBound.supportsPrerelease || self.upperBound.supportsPrerelease + } +} + +fileprivate extension Version { + var supportsPrerelease: Bool { + !self.prereleaseIdentifiers.isEmpty + } } diff --git a/Tests/PackageGraphTests/VersionSetSpecifierTests.swift b/Tests/PackageGraphTests/VersionSetSpecifierTests.swift index 48de4bfa0d0..a79e635b02e 100644 --- a/Tests/PackageGraphTests/VersionSetSpecifierTests.swift +++ b/Tests/PackageGraphTests/VersionSetSpecifierTests.swift @@ -151,4 +151,29 @@ final class VersionSetSpecifierTests: XCTestCase { XCTAssertTrue(VersionSetSpecifier.range("2.0.1"..<"2.0.2") == VersionSetSpecifier.ranges(["2.0.1"..<"2.0.2"])) XCTAssertTrue(VersionSetSpecifier.ranges(["2.0.1"..<"2.0.2"]) == VersionSetSpecifier.range("2.0.1"..<"2.0.2")) } + + func testPrereleases() { + XCTAssertFalse(VersionSetSpecifier.any.supportsPrereleases) + XCTAssertFalse(VersionSetSpecifier.empty.supportsPrereleases) + XCTAssertFalse(VersionSetSpecifier.exact("0.0.1").supportsPrereleases) + + XCTAssertTrue(VersionSetSpecifier.exact("0.0.1-latest").supportsPrereleases) + XCTAssertTrue(VersionSetSpecifier.range("0.0.1-latest" ..< "2.0.0").supportsPrereleases) + XCTAssertTrue(VersionSetSpecifier.range("0.0.1" ..< "2.0.0-latest").supportsPrereleases) + + XCTAssertTrue(VersionSetSpecifier.ranges([ + "0.0.1" ..< "0.0.2", + "0.0.1" ..< "2.0.0-latest", + ]).supportsPrereleases) + + XCTAssertTrue(VersionSetSpecifier.ranges([ + "0.0.1-latest" ..< "0.0.2", + "0.0.1" ..< "2.0.0", + ]).supportsPrereleases) + + XCTAssertFalse(VersionSetSpecifier.ranges([ + "0.0.1" ..< "0.0.2", + "0.0.1" ..< "2.0.0", + ]).supportsPrereleases) + } } From 4dcc7453d9e1465f1daca43729daf5483d6d1688 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Thu, 18 Jul 2024 13:44:24 -0700 Subject: [PATCH 2/3] [PubGrub] Prefer undecided packages that don't support pre-release versions Prefer packages that don't allow pre-release versions to allow propagation logic it find dependencies that limit the range before making any decisions. This means that we'd always prefer release versions. --- .../PubGrub/PubGrubDependencyResolver.swift | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift b/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift index 37bfea4d8a9..f9e086ee0e1 100644 --- a/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift +++ b/Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift @@ -764,7 +764,17 @@ public struct PubGrubDependencyResolver { let start = DispatchTime.now() let counts = try result.get() // forced unwraps safe since we are testing for count and errors above - let pkgTerm = undecided.min { counts[$0]! < counts[$1]! }! + let pkgTerm = undecided.min { + // Prefer packages that don't allow pre-release versions + // to allow propagation logic to find dependencies that + // limit the range before making any decisions. This means + // that we'd always prefer release versions. + if $0.supportsPrereleases != $1.supportsPrereleases { + return !$0.supportsPrereleases + } + + return counts[$0]! < counts[$1]! + }! self.delegate?.willResolve(term: pkgTerm) // at this point the container is cached let container = try self.provider.getCachedContainer(for: pkgTerm.node.package) From ea543aab46be8bac7b77d93423af588cfc0d14bb Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Fri, 19 Jul 2024 00:07:32 -0700 Subject: [PATCH 3/3] [PubGrub] Relax `isValidDecision` check to handle prerelease versions correctly The intersection between release and pre-release ranges is allowed to produce a pre-release range which means that the solver is allowed to make a pre-release version decision even when some of the versions didn't allow pre-releases. --- .../Resolution/PubGrub/Term.swift | 31 +++- .../PackageGraph/VersionSetSpecifier.swift | 38 +++++ Tests/PackageGraphTests/PubgrubTests.swift | 152 ++++++++++++++++++ 3 files changed, 220 insertions(+), 1 deletion(-) diff --git a/Sources/PackageGraph/Resolution/PubGrub/Term.swift b/Sources/PackageGraph/Resolution/PubGrub/Term.swift index 7d020cc3b56..9a8c458004c 100644 --- a/Sources/PackageGraph/Resolution/PubGrub/Term.swift +++ b/Sources/PackageGraph/Resolution/PubGrub/Term.swift @@ -104,9 +104,38 @@ public struct Term: Equatable, Hashable { /// - There has to be no decision for it. /// - The package version has to match all assignments. public func isValidDecision(for solution: PartialSolution) -> Bool { + // The intersection between release and pre-release ranges is + // allowed to produce a pre-release range. This means that the + // solver is allowed to make a pre-release version decision + // even when some of the versions didn't allow pre-releases. + // + // This means that we should ignore pre-release differences + // while checking derivations and assert only if the term is + // pre-release but the last assignment wasn't. + if self.supportsPrereleases { + if let assignment = solution.assignments.last(where: { $0.term.node == self.node }) { + assert(assignment.term.supportsPrereleases) + } + } + for assignment in solution.assignments where assignment.term.node == self.node { assert(!assignment.isDecision, "Expected assignment to be a derivation.") - guard self.satisfies(assignment.term) else { return false } + + // This is not great but dragging `ignorePrereleases` through all the APIs seems + // worse. This is valid because we can have a derivation chain which is something + // like - "0.0.1"..<"1.0.0" -> "0.0.4-latest"..<"0.0.6" and make a decision + // `0.0.4-alpha5` based on that if there is no `0.0.4` release. In vacuum this is + // (currently) incorrect because `0.0.4-alpha5` doesn't satisfy the initial + // range that doesn't support pre-release versions. Since the solver is + // allowed to derive a pre-release range we consider the original range to + // be pre-release range implicitly. + let term = if self.supportsPrereleases && !assignment.term.supportsPrereleases { + Term(self.node, self.requirement.withoutPrereleases) + } else { + self + } + + guard term.satisfies(assignment.term) else { return false } } return true } diff --git a/Sources/PackageGraph/VersionSetSpecifier.swift b/Sources/PackageGraph/VersionSetSpecifier.swift index b9526294ceb..12b92e0f268 100644 --- a/Sources/PackageGraph/VersionSetSpecifier.swift +++ b/Sources/PackageGraph/VersionSetSpecifier.swift @@ -479,6 +479,23 @@ extension VersionSetSpecifier { ranges.contains(where: \.supportsPrereleases) } } + + package var withoutPrereleases: VersionSetSpecifier { + if !supportsPrereleases { + return self + } + + return switch self { + case .empty, .any: + self + case .range(let range): + .range(range.withoutPrerelease) + case .ranges(let ranges): + .ranges(ranges.map { $0.withoutPrerelease }) + case .exact(let version): + .exact(version.withoutPrerelease) + } + } } extension VersionSetSpecifier: CustomStringConvertible { @@ -524,10 +541,31 @@ fileprivate extension Range where Bound == Version { var supportsPrereleases: Bool { self.lowerBound.supportsPrerelease || self.upperBound.supportsPrerelease } + + var withoutPrerelease: Range { + if !supportsPrereleases { + return self + } + + return Range(uncheckedBounds: ( + lower: self.lowerBound.withoutPrerelease, + upper: self.upperBound.withoutPrerelease + )) + } } fileprivate extension Version { var supportsPrerelease: Bool { !self.prereleaseIdentifiers.isEmpty } + + var withoutPrerelease: Version { + Version( + self.major, + self.minor, + self.patch, + prereleaseIdentifiers: [], + buildMetadataIdentifiers: self.buildMetadataIdentifiers + ) + } } diff --git a/Tests/PackageGraphTests/PubgrubTests.swift b/Tests/PackageGraphTests/PubgrubTests.swift index 66759152546..bebe4e7f75e 100644 --- a/Tests/PackageGraphTests/PubgrubTests.swift +++ b/Tests/PackageGraphTests/PubgrubTests.swift @@ -3282,6 +3282,158 @@ final class PubGrubBacktrackTests: XCTestCase { .contains(where: { $0.message == "[DependencyResolver] resolved 'd' @ '2.3.0'" }) ) } + + func testPrereleaseVersionSelection() throws { + try builder.serve("a", at: "1.0.0", with: [ + "a": [ + "b": (.versionSet(.range("0.0.8"..<"2.0.0")), .specific(["b"])) + ] + ]) + + try builder.serve("b", at: "1.0.0-prerelease-20240710") + + try builder.serve("c", at: "1.0.0", with: [ + "c": [ + "d": (.versionSet(.range("1.0.5"..<"2.0.0")), .specific(["d"])) + ] + ]) + try builder.serve("c", at: "1.0.1") + + try builder.serve("d", at: "1.0.0-prerelease-20240710") + try builder.serve("d", at: "1.0.6-prerelease-1") + try builder.serve("d", at: "1.0.6") + + let resolver = builder.create() + // The order matters here because solver used to assign `b` before `a`. + let dependencies1 = try builder.create(dependencies: [ + "a": (.versionSet(.exact("1.0.0")), .specific(["a"])), + "b": (.versionSet(.range("1.0.0-latest"..<"2.0.0")), .specific(["b"])) + ]) + + AssertResult(resolver.solve(constraints: dependencies1), [ + ("a", .version("1.0.0")), + ("b", .version("1.0.0-prerelease-20240710")) + ]) + + let dependencies2 = try builder.create(dependencies: [ + "c": (.versionSet(.exact("1.0.0")), .specific(["c"])), + "d": (.versionSet(.range("1.0.0-latest"..<"2.0.0")), .specific(["d"])) + ]) + + AssertResult(resolver.solve(constraints: dependencies2), [ + ("c", .version("1.0.0")), + ("d", .version("1.0.6")) + ]) + } + + func testPrereleaseExactRequirement() throws { + try builder.serve("c", at: "1.0.0", with: [ + "c": [ + "d": (.versionSet(.range("1.0.4"..<"2.0.0")), .specific(["d"])) + ] + ]) + try builder.serve("c", at: "1.0.1") + + try builder.serve("d", at: "1.0.0-prerelease-20240710") + try builder.serve("d", at: "1.0.4") + try builder.serve("d", at: "1.0.6-prerelease-1") + + let resolver = builder.create() + + let exactDependencies = try builder.create(dependencies: [ + "c": (.versionSet(.exact("1.0.0")), .specific(["c"])), + "d": (.versionSet(.exact("1.0.6-prerelease-1")), .specific(["d"])) + ]) + + // FIXME: This should produce a valid solution but cannot at the + // moment because "1.0.4"..<"2.0.0" doesn't support pre-release versions + // and there is no way to infer it. + let resultWithExact = resolver.solve(constraints: exactDependencies) + XCTAssertMatch( + resultWithExact.errorMsg, + .contains("Dependencies could not be resolved because root depends on 'd' 1.0.6-prerelease-1") + ) + + let rangeDependency = try builder.create(dependencies: [ + "c": (.versionSet(.exact("1.0.0")), .specific(["c"])), + "d": (.versionSet(.range("1.0.5"..<"1.0.6-prerelease-2")), .specific(["d"])) + ]) + + let resultWithRange = resolver.solve(constraints: rangeDependency) + AssertResult(resultWithRange, [ + ("c", .version("1.0.0")), + ("d", .version("1.0.6-prerelease-1")) + ]) + } + + func testReleaseOverPrerelease() throws { + try builder.serve("a", at: "1.0.0", with: [ + "a": [ + "b": (.versionSet(.range("0.0.8"..<"2.0.0")), .specific(["b"])) + ] + ]) + + try builder.serve("b", at: "1.0.0-prerelease-20240616") + try builder.serve("b", at: "1.0.0") + + let resolver = builder.create() + // The order matters here because solver used to assign `b` before `a`. + let dependencies1 = try builder.create(dependencies: [ + "a": (.versionSet(.exact("1.0.0")), .specific(["a"])), + "b": (.versionSet(.range("1.0.0-latest"..<"2.0.0")), .specific(["b"])) + ]) + + AssertResult(resolver.solve(constraints: dependencies1), [ + ("a", .version("1.0.0")), + ("b", .version("1.0.0")) + ]) + + let dependencies2 = try builder.create(dependencies: [ + "b": (.versionSet(.range("1.0.0-latest"..<"2.0.0")), .specific(["b"])), + "a": (.versionSet(.exact("1.0.0")), .specific(["a"])) + ]) + + AssertResult(resolver.solve(constraints: dependencies2), [ + ("a", .version("1.0.0")), + ("b", .version("1.0.0")) + ]) + } + + func testPrereleaseInferenceThroughDependencies() throws { + try builder.serve("a", at: "1.0.0", with: [ + "a": [ + "b": (.versionSet(.range("1.0.0"..<"2.0.0-latest")), .specific(["b"])) + ] + ]) + + try builder.serve("b", at: "0.0.8-prerelease-20230310") + try builder.serve("b", at: "0.0.8") + try builder.serve("b", at: "1.0.0-prerelease-20230616") + try builder.serve("b", at: "1.0.0") + try builder.serve("b", at: "1.9.9-prerelease-20240702") + + let resolver = builder.create() + // The order matters here because solver used to assign `b` before `a`. + let dependencies1 = try builder.create(dependencies: [ + "a": (.versionSet(.exact("1.0.0")), .specific(["a"])), + "b": (.versionSet(.range("0.0.8"..<"2.0.0")), .specific(["b"])) + ]) + + AssertResult(resolver.solve(constraints: dependencies1), [ + ("a", .version("1.0.0")), + ("b", .version("1.9.9-prerelease-20240702")) + ]) + + let dependencies2 = try builder.create(dependencies: [ + "b": (.versionSet(.range("0.0.8"..<"2.0.0")), .specific(["b"])), + "a": (.versionSet(.exact("1.0.0")), .specific(["a"])) + ]) + + AssertResult(resolver.solve(constraints: dependencies2), [ + ("a", .version("1.0.0")), + ("b", .version("1.9.9-prerelease-20240702")) + ]) + } } extension PinsStore.PinState {