From 088b7d5337f16fb0626f44a98c3bef52bb00351a Mon Sep 17 00:00:00 2001 From: Boris Buegling Date: Fri, 11 Aug 2023 13:29:17 -0700 Subject: [PATCH] Improve version-specific manifest selection Currently, we only pick a non-exact match for version-specific manifests if its tools-version is *higher* than the regular one. That seems like an edge case only and we should *also* pick it if the regular manifest is incompatible. This both matches intuition as well as our current documentation on the topic. It should also be backwards-compatible with the current behavior since the new behavior only comes into play if we would have previously failed to load a manifest at all. Co-authored-by: Anders Bertelrud --- .../PackageLoading/ToolsVersionParser.swift | 64 ++++++++++--------- Sources/PackageModel/ToolsVersion.swift | 45 +++++++++---- .../ToolsVersionParserTests.swift | 15 +++++ 3 files changed, 84 insertions(+), 40 deletions(-) diff --git a/Sources/PackageLoading/ToolsVersionParser.swift b/Sources/PackageLoading/ToolsVersionParser.swift index 6c3cade990f..87f72d7bb73 100644 --- a/Sources/PackageLoading/ToolsVersionParser.swift +++ b/Sources/PackageLoading/ToolsVersionParser.swift @@ -583,8 +583,6 @@ extension ManifestLoader { } } - // Otherwise, check if there is a version-specific manifest that has - // a higher tools version than the main Package.swift file. let contents: [String] do { contents = try fileSystem.getDirectoryContents(packagePath) } catch { throw ToolsVersionParser.Error.inaccessiblePackage(path: packagePath, reason: String(describing: error)) @@ -607,39 +605,47 @@ extension ManifestLoader { let regularManifest = packagePath.appending(component: Manifest.filename) - // Find the newest version-specific manifest that is compatible with the the current tools version. - if let versionSpecificCandidate = versionSpecificManifests.keys.sorted(by: >).first(where: { $0 <= currentToolsVersion }) { + // Try to get the tools version of the regular manifest. As the comment marker is missing, we default to + // tools version 3.1.0 (as documented). + let regularManifestToolsVersion: ToolsVersion + do { + regularManifestToolsVersion = try ToolsVersionParser.parse(manifestPath: regularManifest, fileSystem: fileSystem) + } + catch let error as UnsupportedToolsVersion where error.packageToolsVersion == .v3 { + regularManifestToolsVersion = .v3 + } - let versionSpecificManifest = packagePath.appending(component: versionSpecificManifests[versionSpecificCandidate]!) + // Find the newest version-specific manifest that is compatible with the current tools version. + guard let versionSpecificCandidate = versionSpecificManifests.keys.sorted(by: >).first(where: { $0 <= currentToolsVersion }) else { + // Otherwise, return the regular manifest. + return regularManifest + } - // SwiftPM 4 introduced tools-version designations; earlier packages default to tools version 3.1.0. - // See https://swift.org/blog/swift-package-manager-manifest-api-redesign. - let versionSpecificManifestToolsVersion: ToolsVersion - if versionSpecificCandidate < .v4 { - versionSpecificManifestToolsVersion = .v3 - } - else { - versionSpecificManifestToolsVersion = try ToolsVersionParser.parse(manifestPath: versionSpecificManifest, fileSystem: fileSystem) - } + let versionSpecificManifest = packagePath.appending(component: versionSpecificManifests[versionSpecificCandidate]!) - // Try to get the tools version of the regular manifest. At the comment marker is missing, we default to - // tools version 3.1.0 (as documented). - let regularManifestToolsVersion: ToolsVersion - do { - regularManifestToolsVersion = try ToolsVersionParser.parse(manifestPath: regularManifest, fileSystem: fileSystem) - } - catch let error as UnsupportedToolsVersion where error.packageToolsVersion == .v3 { - regularManifestToolsVersion = .v3 - } + // SwiftPM 4 introduced tools-version designations; earlier packages default to tools version 3.1.0. + // See https://swift.org/blog/swift-package-manager-manifest-api-redesign. + let versionSpecificManifestToolsVersion: ToolsVersion + if versionSpecificCandidate < .v4 { + versionSpecificManifestToolsVersion = .v3 + } + else { + versionSpecificManifestToolsVersion = try ToolsVersionParser.parse(manifestPath: versionSpecificManifest, fileSystem: fileSystem) + } - // Compare the tools version of this manifest with the regular - // manifest and use the version-specific manifest if it has - // a greater tools version. - if versionSpecificManifestToolsVersion > regularManifestToolsVersion { + // Compare the tools version of this manifest with the regular + // manifest and use the version-specific manifest if it has + // a greater tools version. + if versionSpecificManifestToolsVersion > regularManifestToolsVersion { + return versionSpecificManifest + } else { + // If there's no primary candidate, validate the regular manifest. + if regularManifestToolsVersion.validateToolsVersion(currentToolsVersion) { + return regularManifest + } else { + // If that's incompatible, use the closest version-specific manifest we got. return versionSpecificManifest } } - - return regularManifest } } diff --git a/Sources/PackageModel/ToolsVersion.swift b/Sources/PackageModel/ToolsVersion.swift index 258243e22ab..4ddb1273cfc 100644 --- a/Sources/PackageModel/ToolsVersion.swift +++ b/Sources/PackageModel/ToolsVersion.swift @@ -105,30 +105,53 @@ public struct ToolsVersion: Equatable, Hashable, Codable, Sendable { _version = version } + private enum ValidationResult { + case valid + case unsupportedToolsVersion + case requireNewerTools + } + + private func _validateToolsVersion(_ currentToolsVersion: ToolsVersion) -> ValidationResult { + // We don't want to throw any error when using the special vNext version. + if SwiftVersion.current.isDevelopment && self == .vNext { + return .valid + } + + // Make sure the package has the right minimum tools version. + guard self >= .minimumRequired else { + return .unsupportedToolsVersion + } + + // Make sure the package isn't newer than the current tools version. + guard currentToolsVersion >= self else { + return .requireNewerTools + } + + return .valid + } + /// Returns true if the tools version is valid and can be used by this /// version of the package manager. + public func validateToolsVersion(_ currentToolsVersion: ToolsVersion) -> Bool { + return self._validateToolsVersion(currentToolsVersion) == .valid + } + public func validateToolsVersion( _ currentToolsVersion: ToolsVersion, packageIdentity: PackageIdentity, packageVersion: String? = .none ) throws { - // We don't want to throw any error when using the special vNext version. - if SwiftVersion.current.isDevelopment && self == .vNext { - return - } - - // Make sure the package has the right minimum tools version. - guard self >= .minimumRequired else { + switch self._validateToolsVersion(currentToolsVersion) { + case .valid: + break + case .unsupportedToolsVersion: throw UnsupportedToolsVersion( packageIdentity: packageIdentity, packageVersion: packageVersion, currentToolsVersion: currentToolsVersion, packageToolsVersion: self ) - } - - // Make sure the package isn't newer than the current tools version. - guard currentToolsVersion >= self else { + case .requireNewerTools: throw RequireNewerTools( packageIdentity: packageIdentity, packageVersion: packageVersion, diff --git a/Tests/PackageLoadingTests/ToolsVersionParserTests.swift b/Tests/PackageLoadingTests/ToolsVersionParserTests.swift index 564659c266b..1cbae7794f4 100644 --- a/Tests/PackageLoadingTests/ToolsVersionParserTests.swift +++ b/Tests/PackageLoadingTests/ToolsVersionParserTests.swift @@ -764,4 +764,19 @@ class ToolsVersionParserTests: XCTestCase { } } + func testVersionSpecificManifestMostCompatibleIfLower() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/pkg/foo" + ) + let root = AbsolutePath("/pkg") + + try fs.writeFileContents(root.appending("Package.swift"), string: "// swift-tools-version:6.0.0\n") + try fs.writeFileContents(root.appending("Package@swift-5.0.swift"), string: "// swift-tools-version:5.0.0\n") + + let currentToolsVersion = ToolsVersion(version: "5.5.0") + let manifestPath = try ManifestLoader.findManifest(packagePath: root, fileSystem: fs, currentToolsVersion: currentToolsVersion) + let version = try ToolsVersionParser.parse(manifestPath: manifestPath, fileSystem: fs) + try version.validateToolsVersion(currentToolsVersion, packageIdentity: .plain("lunch")) + XCTAssertEqual(version.description, "5.0.0") + } }