From 4fea70bf34eb2d7f52d766dd7ac2d4268881abe5 Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Thu, 10 Jul 2025 17:25:55 -0400 Subject: [PATCH 1/6] Allow ToolsVersionParser to throw missing version tools error Seems like we were catching this error and automatically adding the UnsupportedToolsVersion error with v3. Will investigate if this breaks any tests, but we probably want to persist the actual error --- .../PackageLoading/ToolsVersionParser.swift | 4 --- .../ToolsVersionParserTests.swift | 25 +++++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/Sources/PackageLoading/ToolsVersionParser.swift b/Sources/PackageLoading/ToolsVersionParser.swift index f38e2e4eb07..0cbe5afeb73 100644 --- a/Sources/PackageLoading/ToolsVersionParser.swift +++ b/Sources/PackageLoading/ToolsVersionParser.swift @@ -55,11 +55,7 @@ public struct ToolsVersionParser { throw ManifestParseError.emptyManifest(path: manifestPath) } - do { return try self.parse(utf8String: manifestContentsDecodedWithUTF8) - } catch Error.malformedToolsVersionSpecification(.commentMarker(.isMissing)) { - throw UnsupportedToolsVersion(packageIdentity: .init(path: manifestPath), currentToolsVersion: .current, packageToolsVersion: .v3) - } } public static func parse(utf8String: String) throws -> ToolsVersion { diff --git a/Tests/PackageLoadingTests/ToolsVersionParserTests.swift b/Tests/PackageLoadingTests/ToolsVersionParserTests.swift index c7c6b05778d..74f56daccf9 100644 --- a/Tests/PackageLoadingTests/ToolsVersionParserTests.swift +++ b/Tests/PackageLoadingTests/ToolsVersionParserTests.swift @@ -369,6 +369,31 @@ final class ToolsVersionParserTests: XCTestCase { } } + /// Verifies that the correct error is thrown for each Swift tools version specification missing entirely.. + func testMissingSwiftToolsVersion() throws { + let manifestSnippetsWithoutVersionSpecifier = [ + "\n import PackageDescription", + ] + + // TODO bp update comments here, as this was copied from above test + // seems that this was introduced in https://github.com/swiftlang/swift-package-manager/pull/4302 + for manifestSnippet in manifestSnippetsWithoutVersionSpecifier { + XCTAssertThrowsError( + try self.parse(manifestSnippet), + "a 'ToolsVersionLoader.Error' should've been thrown, because the version specifier is missing from the Swift tools version specification" + ) { error in + guard let error = error as? ToolsVersionParser.Error, case .malformedToolsVersionSpecification(.commentMarker(.isMissing)) = error else { + XCTFail("'ToolsVersionLoader.Error.malformedToolsVersionSpecification(.versionSpecifier(.isMissing))' should've been thrown, but a different error is thrown") + return + } + XCTAssertEqual( + error.description, + "the manifest is missing a Swift tools version specification; consider prepending to the manifest '\(ToolsVersion.current.specification())' to specify the current Swift toolchain version as the lowest Swift version supported by the project; if such a specification already exists, consider moving it to the top of the manifest, or prepending it with '//' to help Swift Package Manager find it" + ) + } + } + } + /// Verifies that the correct error is thrown for each misspelt comment marker in Swift tools version specification. func testMisspeltSpecificationCommentMarkers() throws { let manifestSnippetsWithMisspeltSpecificationCommentMarker = [ From 5dae24fb692d25acbd63eeb77fd05790b8063edb Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Wed, 16 Jul 2025 10:45:32 -0400 Subject: [PATCH 2/6] Whitespace fix --- Sources/PackageLoading/ToolsVersionParser.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/PackageLoading/ToolsVersionParser.swift b/Sources/PackageLoading/ToolsVersionParser.swift index 0cbe5afeb73..c08ee694d2a 100644 --- a/Sources/PackageLoading/ToolsVersionParser.swift +++ b/Sources/PackageLoading/ToolsVersionParser.swift @@ -55,7 +55,7 @@ public struct ToolsVersionParser { throw ManifestParseError.emptyManifest(path: manifestPath) } - return try self.parse(utf8String: manifestContentsDecodedWithUTF8) + return try self.parse(utf8String: manifestContentsDecodedWithUTF8) } public static func parse(utf8String: String) throws -> ToolsVersion { From 310e1f16f6b7bbc654ee04939b2de326a8a4e79f Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Wed, 16 Jul 2025 16:49:20 -0400 Subject: [PATCH 3/6] Add tests to assure missing tools version error is being thrown --- .../ToolsVersionParserTests.swift | 50 ++++++++++++------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/Tests/PackageLoadingTests/ToolsVersionParserTests.swift b/Tests/PackageLoadingTests/ToolsVersionParserTests.swift index 74f56daccf9..7a7b3746ffa 100644 --- a/Tests/PackageLoadingTests/ToolsVersionParserTests.swift +++ b/Tests/PackageLoadingTests/ToolsVersionParserTests.swift @@ -371,26 +371,21 @@ final class ToolsVersionParserTests: XCTestCase { /// Verifies that the correct error is thrown for each Swift tools version specification missing entirely.. func testMissingSwiftToolsVersion() throws { - let manifestSnippetsWithoutVersionSpecifier = [ - "\n import PackageDescription", - ] + let manifestSnippetsWithoutVersionSpecifier = "\n import PackageDescription" // TODO bp update comments here, as this was copied from above test - // seems that this was introduced in https://github.com/swiftlang/swift-package-manager/pull/4302 - for manifestSnippet in manifestSnippetsWithoutVersionSpecifier { - XCTAssertThrowsError( - try self.parse(manifestSnippet), - "a 'ToolsVersionLoader.Error' should've been thrown, because the version specifier is missing from the Swift tools version specification" - ) { error in - guard let error = error as? ToolsVersionParser.Error, case .malformedToolsVersionSpecification(.commentMarker(.isMissing)) = error else { - XCTFail("'ToolsVersionLoader.Error.malformedToolsVersionSpecification(.versionSpecifier(.isMissing))' should've been thrown, but a different error is thrown") - return - } - XCTAssertEqual( - error.description, - "the manifest is missing a Swift tools version specification; consider prepending to the manifest '\(ToolsVersion.current.specification())' to specify the current Swift toolchain version as the lowest Swift version supported by the project; if such a specification already exists, consider moving it to the top of the manifest, or prepending it with '//' to help Swift Package Manager find it" - ) + XCTAssertThrowsError( + try self.parse(manifestSnippetsWithoutVersionSpecifier), + "a 'ToolsVersionParser.Error' should've been thrown, because the version specifier is missing from the Swift tools version specification" + ) { error in + guard let error = error as? ToolsVersionParser.Error, case .malformedToolsVersionSpecification(.commentMarker(.isMissing)) = error else { + XCTFail("'ToolsVersionLoader.Error.malformedToolsVersionSpecification(.versionSpecifier(.isMissing))' should've been thrown, but a different error is thrown") + return } + XCTAssertEqual( + error.description, + "the manifest is missing a Swift tools version specification; consider prepending to the manifest '\(ToolsVersion.current.specification())' to specify the current Swift toolchain version as the lowest Swift version supported by the project; if such a specification already exists, consider moving it to the top of the manifest, or prepending it with '//' to help Swift Package Manager find it" + ) } } @@ -862,4 +857,25 @@ final class ToolsVersionParserTests: XCTestCase { try version.validateToolsVersion(currentToolsVersion, packageIdentity: .plain("lunch")) XCTAssertEqual(version.description, "5.0.0") } + + func testMissingToolsVersionManifest() throws { + let fs = InMemoryFileSystem() + let root = AbsolutePath("/pkg") + + try fs.writeFileContents(root.appending("Package.swift"), string: "\n import PackageDescription") + XCTAssertThrowsError( + try ManifestLoader.findManifest(packagePath: root, fileSystem: fs, currentToolsVersion: .current) + ) { error in + guard let error = error as? ToolsVersionParser.Error, case .malformedToolsVersionSpecification(.commentMarker(.isMissing)) = error else { + XCTFail("'ToolsVersionParser.Error.malformedToolsVersionSpecification(.commentMarker(.isMissing))' should've been thrown, but a different error is thrown.") + return + } + + XCTAssertEqual( + error.description, + "the manifest is missing a Swift tools version specification; consider prepending to the manifest '\(ToolsVersion.current.specification())' to specify the current Swift toolchain version as the lowest Swift version supported by the project; if such a specification already exists, consider moving it to the top of the manifest, or prepending it with '//' to help Swift Package Manager find it" + ) + } + } + } From 8d3c96fcf1bb062eccc6174208c879c0ae3ef4eb Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Wed, 16 Jul 2025 16:52:04 -0400 Subject: [PATCH 4/6] Fix comment for test --- Tests/PackageLoadingTests/ToolsVersionParserTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/PackageLoadingTests/ToolsVersionParserTests.swift b/Tests/PackageLoadingTests/ToolsVersionParserTests.swift index 7a7b3746ffa..9239947c6d2 100644 --- a/Tests/PackageLoadingTests/ToolsVersionParserTests.swift +++ b/Tests/PackageLoadingTests/ToolsVersionParserTests.swift @@ -369,7 +369,7 @@ final class ToolsVersionParserTests: XCTestCase { } } - /// Verifies that the correct error is thrown for each Swift tools version specification missing entirely.. + /// Verifies that the correct error is thrown for each Swift tools version specification missing entirely. func testMissingSwiftToolsVersion() throws { let manifestSnippetsWithoutVersionSpecifier = "\n import PackageDescription" From 0b5f53963b85e1828930ff519c24e23b7882c23c Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Thu, 17 Jul 2025 11:21:36 -0400 Subject: [PATCH 5/6] Update check for version-specific manifests --- Sources/PackageLoading/ToolsVersionParser.swift | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Sources/PackageLoading/ToolsVersionParser.swift b/Sources/PackageLoading/ToolsVersionParser.swift index c08ee694d2a..92383fe8aba 100644 --- a/Sources/PackageLoading/ToolsVersionParser.swift +++ b/Sources/PackageLoading/ToolsVersionParser.swift @@ -634,6 +634,7 @@ extension ManifestLoader { }, uniquingKeysWith: { $1 }) let regularManifest = packagePath.appending(component: Manifest.filename) +// 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). @@ -641,8 +642,13 @@ extension ManifestLoader { do { regularManifestToolsVersion = try ToolsVersionParser.parse(manifestPath: regularManifest, fileSystem: fileSystem) } - catch let error as UnsupportedToolsVersion where error.packageToolsVersion == .v3 { - regularManifestToolsVersion = .v3 + catch let error as ToolsVersionParser.Error { + if case .malformedToolsVersionSpecification(.commentMarker(.isMissing)) = error, + !versionSpecificManifests.isEmpty { + regularManifestToolsVersion = .v3 + } else { + throw error + } } // Find the newest version-specific manifest that is compatible with the current tools version. From 657051965b0d905e1ecca63932e6a74a9d2bcaac Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Fri, 18 Jul 2025 11:40:23 -0400 Subject: [PATCH 6/6] Add comments to modified tools version check + test case --- Sources/PackageLoading/ToolsVersionParser.swift | 4 +++- .../ToolsVersionParserTests.swift | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/Sources/PackageLoading/ToolsVersionParser.swift b/Sources/PackageLoading/ToolsVersionParser.swift index 92383fe8aba..e3df3d9e0df 100644 --- a/Sources/PackageLoading/ToolsVersionParser.swift +++ b/Sources/PackageLoading/ToolsVersionParser.swift @@ -634,7 +634,6 @@ extension ManifestLoader { }, uniquingKeysWith: { $1 }) let regularManifest = packagePath.appending(component: Manifest.filename) -// 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). @@ -643,6 +642,9 @@ extension ManifestLoader { regularManifestToolsVersion = try ToolsVersionParser.parse(manifestPath: regularManifest, fileSystem: fileSystem) } catch let error as ToolsVersionParser.Error { + // If we have version-specific manifests, there are still more checks we must do if + // the error being thrown is that of a missing comment marker. + // Set the tools version to 3.1.0 since earlier packages default to this. if case .malformedToolsVersionSpecification(.commentMarker(.isMissing)) = error, !versionSpecificManifests.isEmpty { regularManifestToolsVersion = .v3 diff --git a/Tests/PackageLoadingTests/ToolsVersionParserTests.swift b/Tests/PackageLoadingTests/ToolsVersionParserTests.swift index 9239947c6d2..96c96d1c330 100644 --- a/Tests/PackageLoadingTests/ToolsVersionParserTests.swift +++ b/Tests/PackageLoadingTests/ToolsVersionParserTests.swift @@ -862,6 +862,7 @@ final class ToolsVersionParserTests: XCTestCase { let fs = InMemoryFileSystem() let root = AbsolutePath("/pkg") + // First, test the missing comment marker with the latest tools version. try fs.writeFileContents(root.appending("Package.swift"), string: "\n import PackageDescription") XCTAssertThrowsError( try ManifestLoader.findManifest(packagePath: root, fileSystem: fs, currentToolsVersion: .current) @@ -876,6 +877,22 @@ final class ToolsVersionParserTests: XCTestCase { "the manifest is missing a Swift tools version specification; consider prepending to the manifest '\(ToolsVersion.current.specification())' to specify the current Swift toolchain version as the lowest Swift version supported by the project; if such a specification already exists, consider moving it to the top of the manifest, or prepending it with '//' to help Swift Package Manager find it" ) } + + // Next, test the missing comment marker with tools version .v4. This should default to using 3.1.0. + try fs.writeFileContents(root.appending("Package.swift"), string: "\n import PackageDescription") + XCTAssertThrowsError( + try ManifestLoader.findManifest(packagePath: root, fileSystem: fs, currentToolsVersion: .v4) + ) { error in + guard let error = error as? UnsupportedToolsVersion, error.packageToolsVersion == .v3 else { + XCTFail("'UnsupportedToolsVersion' should've been thrown, but a different error is thrown.") + return + } + + XCTAssertEqual( + error.description, + "the manifest is missing a Swift tools version specification; consider prepending to the manifest '\(ToolsVersion.current.specification())' to specify the current Swift toolchain version as the lowest Swift version supported by the project; if such a specification already exists, consider moving it to the top of the manifest, or prepending it with '//' to help Swift Package Manager find it" + ) + } } }