From 3fedbbd69dbcea1eb6a3ee479345712c7687940c Mon Sep 17 00:00:00 2001 From: Ankit Aggarwal Date: Wed, 17 Oct 2018 17:18:04 -0700 Subject: [PATCH] [PackageLoading] Filter versions containing older tools version We were only doing this for root packages, we also need to do this check during dependency resolution. Alamofire as a dependency shows manifest parse error instead of tools version message --- Sources/Commands/SwiftPackageTool.swift | 4 +- .../RepositoryPackageContainerProvider.swift | 10 ++++- Sources/PackageLoading/PackageBuilder.swift | 5 --- Sources/PackageModel/ToolsVersion.swift | 3 ++ Sources/TestSupport/TestWorkspace.swift | 13 +++++- Sources/Workspace/InitPackage.swift | 2 +- Sources/Workspace/ToolsVersionWriter.swift | 2 +- Sources/Workspace/Workspace.swift | 2 +- .../VersionSpecificTests.swift | 4 ++ ...ositoryPackageContainerProviderTests.swift | 18 ++++---- .../ToolsVersionWriterTests.swift | 4 +- Tests/WorkspaceTests/WorkspaceTests.swift | 42 +++++++++++++++++++ Tests/WorkspaceTests/XCTestManifests.swift | 1 + 13 files changed, 88 insertions(+), 22 deletions(-) diff --git a/Sources/Commands/SwiftPackageTool.swift b/Sources/Commands/SwiftPackageTool.swift index 1d07b843dde..8013009e75c 100644 --- a/Sources/Commands/SwiftPackageTool.swift +++ b/Sources/Commands/SwiftPackageTool.swift @@ -210,14 +210,14 @@ public class SwiftPackageTool: SwiftTool { // FIXME: Probably lift this error defination to ToolsVersion. throw ToolsVersionLoader.Error.malformed(specifier: value, file: pkg) } - try writeToolsVersion(at: pkg, version: toolsVersion, fs: &localFileSystem) + try writeToolsVersion(at: pkg, version: toolsVersion, fs: localFileSystem) case .setCurrent: // Write the tools version with current version but with patch set to zero. // We do this to avoid adding unnecessary constraints to patch versions, if // the package really needs it, they can do it using --set option. try writeToolsVersion( - at: pkg, version: ToolsVersion.currentToolsVersion.zeroedPatch, fs: &localFileSystem) + at: pkg, version: ToolsVersion.currentToolsVersion.zeroedPatch, fs: localFileSystem) } case .generateXcodeproj: diff --git a/Sources/PackageGraph/RepositoryPackageContainerProvider.swift b/Sources/PackageGraph/RepositoryPackageContainerProvider.swift index 6df8f091bd3..9c68dda7968 100644 --- a/Sources/PackageGraph/RepositoryPackageContainerProvider.swift +++ b/Sources/PackageGraph/RepositoryPackageContainerProvider.swift @@ -268,7 +268,15 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve // Otherwise, compute and cache the result. let isValid = (try? self.toolsVersion(for: $0)).flatMap({ - self.currentToolsVersion >= $0 + guard $0 >= ToolsVersion.minimumRequired else { + return false + } + + guard self.currentToolsVersion >= $0 else { + return false + } + + return true }) ?? false self.validToolsVersionsCache[$0] = isValid diff --git a/Sources/PackageLoading/PackageBuilder.swift b/Sources/PackageLoading/PackageBuilder.swift index 8869bec9322..7b51c877c95 100644 --- a/Sources/PackageLoading/PackageBuilder.swift +++ b/Sources/PackageLoading/PackageBuilder.swift @@ -45,9 +45,6 @@ public enum ModuleError: Swift.Error { /// We found multiple LinuxMain.swift files. case multipleLinuxMainFound(package: String, linuxMainFiles: [AbsolutePath]) - /// The package should support version 3 compiler but doesn't. - case mustSupportSwift3Compiler(package: String) - /// The tools version in use is not compatible with target's sources. case incompatibleToolsVersions(package: String, required: [SwiftLanguageVersion], current: ToolsVersion) @@ -82,8 +79,6 @@ extension ModuleError: CustomStringConvertible { case .multipleLinuxMainFound(let package, let linuxMainFiles): let files = linuxMainFiles.map({ $0.asString }).sorted().joined(separator: ", ") return "package '\(package)' has multiple linux main files: \(files)" - case .mustSupportSwift3Compiler(let package): - return "package '\(package)' must support Swift 3 because its minimum tools version is 3" case .incompatibleToolsVersions(let package, let required, let current): if required.isEmpty { return "package '\(package)' supported Swift language versions is empty" diff --git a/Sources/PackageModel/ToolsVersion.swift b/Sources/PackageModel/ToolsVersion.swift index e9ed1fc8c1b..dc2e74aa051 100644 --- a/Sources/PackageModel/ToolsVersion.swift +++ b/Sources/PackageModel/ToolsVersion.swift @@ -25,6 +25,9 @@ public struct ToolsVersion: CustomStringConvertible, Comparable { "\(Versioning.currentVersion.minor)." + "\(Versioning.currentVersion.patch)")! + /// The minimum tools version that is required by the package manager. + public static let minimumRequired: ToolsVersion = .v4 + /// Regex pattern to parse tools version. The format is SemVer 2.0 with an /// addition that specifying the patch version is optional. static let toolsVersionRegex = try! NSRegularExpression(pattern: "^" + diff --git a/Sources/TestSupport/TestWorkspace.swift b/Sources/TestSupport/TestWorkspace.swift index 7ae2bc010a0..07f1992c579 100644 --- a/Sources/TestSupport/TestWorkspace.swift +++ b/Sources/TestSupport/TestWorkspace.swift @@ -78,6 +78,7 @@ public final class TestWorkspace { let sourcesDir = packagePath.appending(component: "Sources") let url = (isRoot ? packagePath : packagesDir.appending(RelativePath(package.path ?? package.name))).asString let specifier = RepositorySpecifier(url: url) + let manifestPath = packagePath.appending(component: Manifest.filename) // Create targets on disk. let repo = repoProvider.specifierMap[specifier] ?? InMemoryGitRepository(path: packagePath, fs: fs as! InMemoryFileSystem) @@ -86,6 +87,10 @@ public final class TestWorkspace { try repo.createDirectory(targetDir, recursive: true) try repo.writeFileContents(targetDir.appending(component: "file.swift"), bytes: "") } + if let toolsVersion = package.toolsVersion { + try repo.writeFileContents(manifestPath, bytes: "") + try writeToolsVersion(at: packagePath, version: toolsVersion, fs: repo) + } repo.commit() let versions: [String?] = isRoot ? [nil] : package.versions @@ -93,7 +98,7 @@ public final class TestWorkspace { let v = version.flatMap(Version.init(string:)) manifests[.init(url: url, version: v)] = Manifest( name: package.name, - path: packagePath.appending(component: Manifest.filename), + path: manifestPath, url: url, version: v, manifestVersion: .v4, @@ -466,6 +471,8 @@ public struct TestPackage { public let products: [TestProduct] public let dependencies: [TestDependency] public let versions: [String?] + // FIXME: This should be per-version. + public let toolsVersion: ToolsVersion? public init( name: String, @@ -473,7 +480,8 @@ public struct TestPackage { targets: [TestTarget], products: [TestProduct], dependencies: [TestDependency] = [], - versions: [String?] = [] + versions: [String?] = [], + toolsVersion: ToolsVersion? = nil ) { self.name = name self.path = path @@ -481,6 +489,7 @@ public struct TestPackage { self.products = products self.dependencies = dependencies self.versions = versions + self.toolsVersion = toolsVersion } public static func genericPackage1(named name: String) -> TestPackage { diff --git a/Sources/Workspace/InitPackage.swift b/Sources/Workspace/InitPackage.swift index 809d5803bab..925ea99254d 100644 --- a/Sources/Workspace/InitPackage.swift +++ b/Sources/Workspace/InitPackage.swift @@ -137,7 +137,7 @@ public final class InitPackage { // Write the current tools version. try writeToolsVersion( - at: manifest.parentDirectory, version: version, fs: &localFileSystem) + at: manifest.parentDirectory, version: version, fs: localFileSystem) } private func writeREADMEFile() throws { diff --git a/Sources/Workspace/ToolsVersionWriter.swift b/Sources/Workspace/ToolsVersionWriter.swift index 23cb8aa4377..6d661e7fd09 100644 --- a/Sources/Workspace/ToolsVersionWriter.swift +++ b/Sources/Workspace/ToolsVersionWriter.swift @@ -18,7 +18,7 @@ import Utility /// - Parameters: /// - path: The path of the package. /// - version: The version to write. -public func writeToolsVersion(at path: AbsolutePath, version: ToolsVersion, fs: inout FileSystem) throws { +public func writeToolsVersion(at path: AbsolutePath, version: ToolsVersion, fs: FileSystem) throws { let file = path.appending(component: Manifest.filename) assert(fs.isFile(file), "Tools version file not present") diff --git a/Sources/Workspace/Workspace.swift b/Sources/Workspace/Workspace.swift index 1db734c5dc6..c7065b0818f 100644 --- a/Sources/Workspace/Workspace.swift +++ b/Sources/Workspace/Workspace.swift @@ -1033,7 +1033,7 @@ extension Workspace { at: packagePath, fileSystem: fileSystem) // Make sure the package has the right minimum tools version. - guard toolsVersion >= .v4 else { + guard toolsVersion >= ToolsVersion.minimumRequired else { throw WorkspaceDiagnostics.IncompatibleToolsVersion( rootPackagePath: packagePath, requiredToolsVersion: .v4, diff --git a/Tests/FunctionalTests/VersionSpecificTests.swift b/Tests/FunctionalTests/VersionSpecificTests.swift index 4c56b6f783d..779fb589eda 100644 --- a/Tests/FunctionalTests/VersionSpecificTests.swift +++ b/Tests/FunctionalTests/VersionSpecificTests.swift @@ -50,6 +50,10 @@ class VersionSpecificTests: XCTestCase { // Create the version to test against. try fs.writeFileContents(depPath.appending(component: "Package.swift")) { + // FIXME: We end up filtering this manifest if it has an invalid + // tools version as they're assumed to be v3 manifests. Should we + // do something better? + $0 <<< "// swift-tools-version:4.2\n" $0 <<< "NOT_A_VALID_PACKAGE" } try fs.writeFileContents(depPath.appending(component: "foo.swift")) { diff --git a/Tests/PackageGraphTests/RepositoryPackageContainerProviderTests.swift b/Tests/PackageGraphTests/RepositoryPackageContainerProviderTests.swift index 6cb6a95c4c9..4d5d9245efb 100644 --- a/Tests/PackageGraphTests/RepositoryPackageContainerProviderTests.swift +++ b/Tests/PackageGraphTests/RepositoryPackageContainerProviderTests.swift @@ -280,14 +280,18 @@ class RepositoryPackageContainerProviderTests: XCTestCase { repo.commit() try repo.tag(name: "1.0.0") - try repo.writeFileContents(filePath, bytes: "// swift-tools-version:3.1.0;hello\n") + try repo.writeFileContents(filePath, bytes: "// swift-tools-version:4.0") repo.commit() try repo.tag(name: "1.0.1") - try repo.writeFileContents(filePath, bytes: "// swift-tools-version:4.0.0\n") + try repo.writeFileContents(filePath, bytes: "// swift-tools-version:4.2.0;hello\n") repo.commit() try repo.tag(name: "1.0.2") + try repo.writeFileContents(filePath, bytes: "// swift-tools-version:4.2.0\n") + repo.commit() + try repo.tag(name: "1.0.3") + let inMemRepoProvider = InMemoryGitRepositoryProvider() inMemRepoProvider.add(specifier: specifier, repository: repo) @@ -307,21 +311,21 @@ class RepositoryPackageContainerProviderTests: XCTestCase { } do { - let provider = createProvider(ToolsVersion(version: "3.1.0")) + let provider = createProvider(ToolsVersion(version: "4.0.0")) let ref = PackageReference(identity: "foo", path: specifier.url) let container = try await { provider.getContainer(for: ref, completion: $0) } let v = container.versions(filter: { _ in true }).map{$0} - XCTAssertEqual(v, ["1.0.1", "1.0.0"]) + XCTAssertEqual(v, ["1.0.1"]) } do { - let provider = createProvider(ToolsVersion(version: "4.0.0")) + let provider = createProvider(ToolsVersion(version: "4.2.0")) let ref = PackageReference(identity: "foo", path: specifier.url) let container = try await { provider.getContainer(for: ref, completion: $0) } XCTAssertEqual((container as! RepositoryPackageContainer).validToolsVersionsCache, [:]) let v = container.versions(filter: { _ in true }).map{$0} - XCTAssertEqual((container as! RepositoryPackageContainer).validToolsVersionsCache, ["1.0.1": true, "1.0.0": true, "1.0.2": true]) - XCTAssertEqual(v, ["1.0.2", "1.0.1", "1.0.0"]) + XCTAssertEqual((container as! RepositoryPackageContainer).validToolsVersionsCache, ["1.0.1": true, "1.0.0": false, "1.0.3": true, "1.0.2": true]) + XCTAssertEqual(v, ["1.0.3", "1.0.2", "1.0.1"]) } do { diff --git a/Tests/WorkspaceTests/ToolsVersionWriterTests.swift b/Tests/WorkspaceTests/ToolsVersionWriterTests.swift index 24647165ca9..8d5d17f1008 100644 --- a/Tests/WorkspaceTests/ToolsVersionWriterTests.swift +++ b/Tests/WorkspaceTests/ToolsVersionWriterTests.swift @@ -106,7 +106,7 @@ class ToolsVersionWriterTests: XCTestCase { _ result: (ByteString) -> Void ) { do { - var fs: FileSystem = InMemoryFileSystem() + let fs: FileSystem = InMemoryFileSystem() let file = AbsolutePath("/pkg/Package.swift") @@ -114,7 +114,7 @@ class ToolsVersionWriterTests: XCTestCase { try fs.writeFileContents(file, bytes: stream.bytes) try writeToolsVersion( - at: file.parentDirectory, version: version, fs: &fs) + at: file.parentDirectory, version: version, fs: fs) result(try fs.readFileContents(file)) } catch { diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift index 9ef5fb43f3b..4ccc322431a 100644 --- a/Tests/WorkspaceTests/WorkspaceTests.swift +++ b/Tests/WorkspaceTests/WorkspaceTests.swift @@ -1237,6 +1237,48 @@ final class WorkspaceTests: XCTestCase { } } + func testMinimumRequiredToolsVersionInDependencyResolution() throws { + let sandbox = AbsolutePath("/tmp/ws/") + let fs = InMemoryFileSystem() + + let workspace = try TestWorkspace( + sandbox: sandbox, + fs: fs, + roots: [ + TestPackage( + name: "Root", + targets: [ + TestTarget(name: "Root", dependencies: ["Foo"]), + ], + products: [], + dependencies: [ + TestDependency(name: "Foo", requirement: .upToNextMajor(from: "1.0.0")), + ] + ), + ], + packages: [ + TestPackage( + name: "Foo", + targets: [ + TestTarget(name: "Foo"), + ], + products: [ + TestProduct(name: "Foo", targets: ["Foo"]), + ], + versions: ["1.0.0"], + toolsVersion: .v3 + ), + ] + ) + + workspace.checkPackageGraph(roots: ["Root"]) { (graph, diagnostics) in + DiagnosticsEngineTester(diagnostics) { result in + result.check(diagnostic: .contains("/tmp/ws/pkgs/Foo @ 1.0.0..<2.0.0"), behavior: .error) + result.check(diagnostic: .contains("product dependency 'Foo' not found"), behavior: .error, location: "'Root' /tmp/ws/roots/Root") + } + } + } + func testToolsVersionRootPackages() throws { let sandbox = AbsolutePath("/tmp/ws/") let fs = InMemoryFileSystem() diff --git a/Tests/WorkspaceTests/XCTestManifests.swift b/Tests/WorkspaceTests/XCTestManifests.swift index 637c6614fe2..a52205355a8 100644 --- a/Tests/WorkspaceTests/XCTestManifests.swift +++ b/Tests/WorkspaceTests/XCTestManifests.swift @@ -64,6 +64,7 @@ extension WorkspaceTests { ("testLocalDependencyWithPackageUpdate", testLocalDependencyWithPackageUpdate), ("testLocalLocalSwitch", testLocalLocalSwitch), ("testLocalVersionSwitch", testLocalVersionSwitch), + ("testMinimumRequiredToolsVersionInDependencyResolution", testMinimumRequiredToolsVersionInDependencyResolution), ("testMissingEditCanRestoreOriginalCheckout", testMissingEditCanRestoreOriginalCheckout), ("testMultipleRootPackages", testMultipleRootPackages), ("testPackageMirror", testPackageMirror),