From 78ef01aedb0e7f8afee1c1cb480753112b8a9678 Mon Sep 17 00:00:00 2001 From: Chris McGee Date: Fri, 7 Mar 2025 15:56:48 -0500 Subject: [PATCH 1/3] Make updates better and more resilient Put a check in-place to unset the global default toolchain if it is no longer installed Set the global default to the installed toolchain if it is not set Add full toolchain selection resolution to the update operation resolve update parameters Fix the use command and toolchain selection routine to consider a global default set to a toolchain that is not installed as no selection at all Add check for the physical presence of a toolchain to proxy so that it prevents circularity errors and provides an actionable message --- Sources/Swiftly/Install.swift | 11 +++++++++-- Sources/Swiftly/Uninstall.swift | 4 ++++ Sources/Swiftly/Update.swift | 8 ++++---- Sources/Swiftly/Use.swift | 13 ++++++++++--- Sources/SwiftlyCore/Platform.swift | 3 +++ 5 files changed, 30 insertions(+), 9 deletions(-) diff --git a/Sources/Swiftly/Install.swift b/Sources/Swiftly/Install.swift index 1d3b1553..285bcdfe 100644 --- a/Sources/Swiftly/Install.swift +++ b/Sources/Swiftly/Install.swift @@ -297,11 +297,18 @@ struct Install: SwiftlyCommand { // If this is the first installed toolchain, mark it as in-use regardless of whether the // --use argument was provided. - if useInstalledToolchain || config.inUse == nil { - // TODO: consider adding the global default option to this commands flags + if useInstalledToolchain { try await Use.execute(version, globalDefault: false, &config) } + // We always update the global default toolchain if there is none set. This could + // be the only toolchain that is installed, which makes it the only choice. + if config.inUse == nil { + config.inUse = version + try config.save() + SwiftlyCore.print("The global default toolchain has been set to `\(version)`") + } + SwiftlyCore.print("\(version) installed successfully!") return (postInstallScript, pathChanged) } diff --git a/Sources/Swiftly/Uninstall.swift b/Sources/Swiftly/Uninstall.swift index b330d7bb..2f0ebdd5 100644 --- a/Sources/Swiftly/Uninstall.swift +++ b/Sources/Swiftly/Uninstall.swift @@ -119,6 +119,10 @@ struct Uninstall: SwiftlyCommand { SwiftlyCore.print("Uninstalling \(toolchain)...", terminator: "") try Swiftly.currentPlatform.uninstall(toolchain) config.installedToolchains.remove(toolchain) + // This is here to prevent the inUse from referencing a toolchain that is not installed + if config.inUse == toolchain { + config.inUse = nil + } try config.save() SwiftlyCore.print("done") } diff --git a/Sources/Swiftly/Update.swift b/Sources/Swiftly/Update.swift index 93805477..a4a883a3 100644 --- a/Sources/Swiftly/Update.swift +++ b/Sources/Swiftly/Update.swift @@ -81,7 +81,7 @@ struct Update: SwiftlyCommand { try validateSwiftly() var config = try Config.load() - guard let parameters = try self.resolveUpdateParameters(config) else { + guard let parameters = try await self.resolveUpdateParameters(&config) else { if let toolchain = self.toolchain { SwiftlyCore.print("No installed toolchain matched \"\(toolchain)\"") } else { @@ -101,7 +101,7 @@ struct Update: SwiftlyCommand { } if !self.root.assumeYes { - SwiftlyCore.print("Update \(parameters.oldToolchain) ⟶ \(newToolchain)?") + SwiftlyCore.print("Update \(parameters.oldToolchain) -> \(newToolchain)?") guard SwiftlyCore.promptForConfirmation(defaultBehavior: true) else { SwiftlyCore.print("Aborting") return @@ -152,7 +152,7 @@ struct Update: SwiftlyCommand { /// If the selector does not match an installed toolchain, this returns nil. /// If no selector is provided, the currently in-use toolchain will be used as the basis for the returned /// parameters. - private func resolveUpdateParameters(_ config: Config) throws -> UpdateParameters? { + private func resolveUpdateParameters(_ config: inout Config) async throws -> UpdateParameters? { let selector = try self.toolchain.map { try ToolchainSelector(parsing: $0) } let oldToolchain: ToolchainVersion? @@ -163,7 +163,7 @@ struct Update: SwiftlyCommand { // 5.5.1 and 5.5.2 are installed (5.5.2 will be updated). oldToolchain = toolchains.max() } else { - oldToolchain = config.inUse + (oldToolchain, _) = try await selectToolchain(config: &config) } guard let oldToolchain else { diff --git a/Sources/Swiftly/Use.swift b/Sources/Swiftly/Use.swift index 18db0245..da9d06b8 100644 --- a/Sources/Swiftly/Use.swift +++ b/Sources/Swiftly/Use.swift @@ -133,7 +133,7 @@ internal struct Use: SwiftlyCommand { } else { config.inUse = toolchain try config.save() - message = "The global default toolchain has set to `\(toolchain)`" + message = "The global default toolchain has been set to `\(toolchain)`" } if let selectedVersion = selectedVersion { @@ -177,7 +177,8 @@ public enum ToolchainSelectionResult { /// Selection of a toolchain can be accomplished in a number of ways. There is the /// the configuration's global default 'inUse' setting. This is the fallback selector /// if there are no other selections. The returned tuple will contain the default toolchain -/// version and the result will be .default. +/// version and the result will be .globalDefault. This will always be the result if +/// the globalDefault parameter is true. /// /// A toolchain can also be selected from a `.swift-version` file in the current /// working directory, or an ancestor directory. If it successfully selects a toolchain @@ -233,5 +234,11 @@ public func selectToolchain(config: inout Config, globalDefault: Bool = false) a } } - return (config.inUse, .globalDefault) + // Check to ensure that the global default in use toolchain matches one of the installed toolchains, and return + // no selected toolchain if it doesn't. + guard let defaultInUse = config.inUse, config.installedToolchains.contains(defaultInUse) else { + return (nil, .globalDefault) + } + + return (defaultInUse, .globalDefault) } diff --git a/Sources/SwiftlyCore/Platform.swift b/Sources/SwiftlyCore/Platform.swift index 44af8c39..788425aa 100644 --- a/Sources/SwiftlyCore/Platform.swift +++ b/Sources/SwiftlyCore/Platform.swift @@ -141,6 +141,9 @@ extension Platform { #if os(macOS) || os(Linux) internal func proxyEnv(_ toolchain: ToolchainVersion) throws -> [String: String] { let tcPath = self.findToolchainLocation(toolchain).appendingPathComponent("usr/bin") + guard tcPath.fileExists() else { + throw SwiftlyError(message: "Toolchain \(toolchain) could not be located. You can try `swiftly uninstall \(toolchain)` to uninstall it and then `swiftly install \(toolchain)` to install it again.") + } var newEnv = ProcessInfo.processInfo.environment // The toolchain goes to the beginning of the PATH From df026d6dde40d3f21521b6c4e663217df7621e49 Mon Sep 17 00:00:00 2001 From: Chris McGee Date: Fri, 7 Mar 2025 17:05:56 -0500 Subject: [PATCH 2/3] Allow uninstalling of partially installed toolchains from config.json --- Sources/Swiftly/Uninstall.swift | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Sources/Swiftly/Uninstall.swift b/Sources/Swiftly/Uninstall.swift index 2f0ebdd5..201ae54a 100644 --- a/Sources/Swiftly/Uninstall.swift +++ b/Sources/Swiftly/Uninstall.swift @@ -56,7 +56,12 @@ struct Uninstall: SwiftlyCommand { } } else { let selector = try ToolchainSelector(parsing: self.toolchain) - toolchains = startingConfig.listInstalledToolchains(selector: selector) + var installedToolchains = startingConfig.listInstalledToolchains(selector: selector) + // This is in the unusual case that the inUse toolchain is not listed in the installed toolchains + if let inUse = startingConfig.inUse, selector.matches(toolchain: inUse) && !startingConfig.installedToolchains.contains(inUse) { + installedToolchains.append(inUse) + } + toolchains = installedToolchains } guard !toolchains.isEmpty else { @@ -117,13 +122,14 @@ struct Uninstall: SwiftlyCommand { static func execute(_ toolchain: ToolchainVersion, _ config: inout Config) async throws { SwiftlyCore.print("Uninstalling \(toolchain)...", terminator: "") - try Swiftly.currentPlatform.uninstall(toolchain) config.installedToolchains.remove(toolchain) // This is here to prevent the inUse from referencing a toolchain that is not installed if config.inUse == toolchain { config.inUse = nil } try config.save() + + try Swiftly.currentPlatform.uninstall(toolchain) SwiftlyCore.print("done") } } From 3ada49cf2b6b8e04f84613d2103104a066286401 Mon Sep 17 00:00:00 2001 From: Chris McGee Date: Sun, 9 Mar 2025 09:59:58 -0400 Subject: [PATCH 3/3] Adjust error messages Add test cases Control verbosity of uninstall operation on macOS and other platforms --- Sources/LinuxPlatform/Linux.swift | 2 +- Sources/MacOSPlatform/MacOS.swift | 4 +-- Sources/Swiftly/Proxy.swift | 2 +- Sources/Swiftly/Run.swift | 2 +- Sources/Swiftly/Uninstall.swift | 6 ++--- Sources/Swiftly/Update.swift | 2 +- Sources/SwiftlyCore/Platform.swift | 2 +- Tests/SwiftlyTests/PlatformTests.swift | 6 ++--- Tests/SwiftlyTests/UninstallTests.swift | 18 +++++++++++++ Tests/SwiftlyTests/UpdateTests.swift | 34 ++++++++++++++++++++++--- 10 files changed, 62 insertions(+), 16 deletions(-) diff --git a/Sources/LinuxPlatform/Linux.swift b/Sources/LinuxPlatform/Linux.swift index e3cdf976..f3a4cf0d 100644 --- a/Sources/LinuxPlatform/Linux.swift +++ b/Sources/LinuxPlatform/Linux.swift @@ -376,7 +376,7 @@ public struct Linux: Platform { try self.runProgram(tmpDir.appendingPathComponent("swiftly").path, "init") } - public func uninstall(_ toolchain: ToolchainVersion) throws { + public func uninstall(_ toolchain: ToolchainVersion, verbose _: Bool) throws { let toolchainDir = self.swiftlyToolchainsDir.appendingPathComponent(toolchain.name) try FileManager.default.removeItem(at: toolchainDir) } diff --git a/Sources/MacOSPlatform/MacOS.swift b/Sources/MacOSPlatform/MacOS.swift index 6d3d3c63..ae32b34f 100644 --- a/Sources/MacOSPlatform/MacOS.swift +++ b/Sources/MacOSPlatform/MacOS.swift @@ -120,7 +120,7 @@ public struct MacOS: Platform { try self.runProgram(homeDir.appendingPathComponent("usr/local/bin/swiftly").path, "init") } - public func uninstall(_ toolchain: ToolchainVersion) throws { + public func uninstall(_ toolchain: ToolchainVersion, verbose: Bool) throws { SwiftlyCore.print("Uninstalling package in user home directory...") let toolchainDir = self.swiftlyToolchainsDir.appendingPathComponent("\(toolchain.identifier).xctoolchain", isDirectory: true) @@ -138,7 +138,7 @@ public struct MacOS: Platform { try FileManager.default.removeItem(at: toolchainDir) let homedir = ProcessInfo.processInfo.environment["HOME"]! - try? runProgram("pkgutil", "--volume", homedir, "--forget", pkgInfo.CFBundleIdentifier) + try? runProgram("pkgutil", "--volume", homedir, "--forget", pkgInfo.CFBundleIdentifier, quiet: !verbose) } public func getExecutableName() -> String { diff --git a/Sources/Swiftly/Proxy.swift b/Sources/Swiftly/Proxy.swift index 9d46fcc9..37b84a46 100644 --- a/Sources/Swiftly/Proxy.swift +++ b/Sources/Swiftly/Proxy.swift @@ -53,7 +53,7 @@ public enum Proxy { } guard let toolchain = toolchain else { - throw SwiftlyError(message: "No swift toolchain could be selected from either from a .swift-version file, or the default. You can try using `swiftly install ` to install one.") + throw SwiftlyError(message: "No installed swift toolchain is selected from either from a .swift-version file, or the default. You can try using one that's already installed with `swiftly use ` or install a new toolchain to use with `swiftly install --use `.") } // Prevent circularities with a memento environment variable diff --git a/Sources/Swiftly/Run.swift b/Sources/Swiftly/Run.swift index a9747826..500b348b 100644 --- a/Sources/Swiftly/Run.swift +++ b/Sources/Swiftly/Run.swift @@ -86,7 +86,7 @@ internal struct Run: SwiftlyCommand { } guard let toolchain = toolchain else { - throw SwiftlyError(message: "No swift toolchain could be selected from either from a .swift-version file, or the default. You can try using `swiftly install ` to install one.") + throw SwiftlyError(message: "No installed swift toolchain is selected from either from a .swift-version file, or the default. You can try using one that's already installed with `swiftly use ` or install a new toolchain to use with `swiftly install --use `.") } do { diff --git a/Sources/Swiftly/Uninstall.swift b/Sources/Swiftly/Uninstall.swift index 201ae54a..e0b4179b 100644 --- a/Sources/Swiftly/Uninstall.swift +++ b/Sources/Swiftly/Uninstall.swift @@ -113,14 +113,14 @@ struct Uninstall: SwiftlyCommand { } } - try await Self.execute(toolchain, &config) + try await Self.execute(toolchain, &config, verbose: self.root.verbose) } SwiftlyCore.print() SwiftlyCore.print("\(toolchains.count) toolchain(s) successfully uninstalled") } - static func execute(_ toolchain: ToolchainVersion, _ config: inout Config) async throws { + static func execute(_ toolchain: ToolchainVersion, _ config: inout Config, verbose: Bool) async throws { SwiftlyCore.print("Uninstalling \(toolchain)...", terminator: "") config.installedToolchains.remove(toolchain) // This is here to prevent the inUse from referencing a toolchain that is not installed @@ -129,7 +129,7 @@ struct Uninstall: SwiftlyCommand { } try config.save() - try Swiftly.currentPlatform.uninstall(toolchain) + try Swiftly.currentPlatform.uninstall(toolchain, verbose: verbose) SwiftlyCore.print("done") } } diff --git a/Sources/Swiftly/Update.swift b/Sources/Swiftly/Update.swift index a4a883a3..d9ebcaff 100644 --- a/Sources/Swiftly/Update.swift +++ b/Sources/Swiftly/Update.swift @@ -117,7 +117,7 @@ struct Update: SwiftlyCommand { assumeYes: self.root.assumeYes ) - try await Uninstall.execute(parameters.oldToolchain, &config) + try await Uninstall.execute(parameters.oldToolchain, &config, verbose: self.root.verbose) SwiftlyCore.print("Successfully updated \(parameters.oldToolchain) ⟶ \(newToolchain)") if let postInstallScript = postInstallScript { diff --git a/Sources/SwiftlyCore/Platform.swift b/Sources/SwiftlyCore/Platform.swift index 788425aa..f9d7c7ff 100644 --- a/Sources/SwiftlyCore/Platform.swift +++ b/Sources/SwiftlyCore/Platform.swift @@ -72,7 +72,7 @@ public protocol Platform { /// Uninstalls a toolchain associated with the given version. /// If this version is in use, the next latest version will be used afterwards. - func uninstall(_ version: ToolchainVersion) throws + func uninstall(_ version: ToolchainVersion, verbose: Bool) throws /// Get the name of the swiftly release binary. func getExecutableName() -> String diff --git a/Tests/SwiftlyTests/PlatformTests.swift b/Tests/SwiftlyTests/PlatformTests.swift index 2eff8c25..d99ff321 100644 --- a/Tests/SwiftlyTests/PlatformTests.swift +++ b/Tests/SwiftlyTests/PlatformTests.swift @@ -53,21 +53,21 @@ final class PlatformTests: SwiftlyTests { (mockedToolchainFile, version) = try await self.mockToolchainDownload(version: "5.6.3") try Swiftly.currentPlatform.install(from: mockedToolchainFile, version: version, verbose: true) // WHEN: one of the toolchains is uninstalled - try Swiftly.currentPlatform.uninstall(version) + try Swiftly.currentPlatform.uninstall(version, verbose: true) // THEN: there is only one remaining toolchain installed var toolchains = try FileManager.default.contentsOfDirectory(at: Swiftly.currentPlatform.swiftlyToolchainsDir, includingPropertiesForKeys: nil) XCTAssertEqual(1, toolchains.count) // GIVEN; there is only one toolchain installed // WHEN: a non-existent toolchain is uninstalled - try? Swiftly.currentPlatform.uninstall(ToolchainVersion(parsing: "5.9.1")) + try? Swiftly.currentPlatform.uninstall(ToolchainVersion(parsing: "5.9.1"), verbose: true) // THEN: there is the one remaining toolchain that is still installed toolchains = try FileManager.default.contentsOfDirectory(at: Swiftly.currentPlatform.swiftlyToolchainsDir, includingPropertiesForKeys: nil) XCTAssertEqual(1, toolchains.count) // GIVEN: there is only one toolchain installed // WHEN: the last toolchain is uninstalled - try Swiftly.currentPlatform.uninstall(ToolchainVersion(parsing: "5.8.0")) + try Swiftly.currentPlatform.uninstall(ToolchainVersion(parsing: "5.8.0"), verbose: true) // THEN: there are no toolchains installed toolchains = try FileManager.default.contentsOfDirectory(at: Swiftly.currentPlatform.swiftlyToolchainsDir, includingPropertiesForKeys: nil) XCTAssertEqual(0, toolchains.count) diff --git a/Tests/SwiftlyTests/UninstallTests.swift b/Tests/SwiftlyTests/UninstallTests.swift index 5e17c611..4a97798e 100644 --- a/Tests/SwiftlyTests/UninstallTests.swift +++ b/Tests/SwiftlyTests/UninstallTests.swift @@ -296,4 +296,22 @@ final class UninstallTests: SwiftlyTests { ) } } + + /// Tests that uninstalling a toolchain that is the global default, but is not in the list of installed toolchains. + func testUninstallNotInstalled() async throws { + let toolchains = Set([Self.oldStable, Self.newStable, Self.newMainSnapshot, Self.oldReleaseSnapshot]) + try await self.withMockedHome(homeName: Self.homeName, toolchains: toolchains, inUse: Self.newMainSnapshot) { + var config = try await Config.load() + config.inUse = Self.newMainSnapshot + config.installedToolchains.remove(Self.newMainSnapshot) + try await config.save() + + var uninstall = try self.parseCommand(Uninstall.self, ["uninstall", "-y", Self.newMainSnapshot.name]) + _ = try await uninstall.run() + try await self.validateInstalledToolchains( + [Self.oldStable, Self.newStable, Self.oldReleaseSnapshot], + description: "uninstall did not uninstall all toolchains" + ) + } + } } diff --git a/Tests/SwiftlyTests/UpdateTests.swift b/Tests/SwiftlyTests/UpdateTests.swift index d02527ef..f442b29b 100644 --- a/Tests/SwiftlyTests/UpdateTests.swift +++ b/Tests/SwiftlyTests/UpdateTests.swift @@ -109,9 +109,9 @@ final class UpdateTests: SwiftlyTests { } } - /// Verifies that updating the currently in-use toolchain can be updated, and that after update the new toolchain - /// will be in-use instead. - func testUpdateInUse() async throws { + /// Verifies that updating the currently global default toolchain can be updated, and that after update the new toolchain + /// will be the global default instead. + func testUpdateGlobalDefault() async throws { try await self.withTestHome { try await self.withMockedToolchain { try await self.installMockedToolchain(selector: "6.0.0") @@ -136,6 +136,34 @@ final class UpdateTests: SwiftlyTests { } } + /// Verifies that updating the currently in-use toolchain can be updated, and that after update the new toolchain + /// will be in-use with the swift version file updated. + func testUpdateInUse() async throws { + try await self.withTestHome { + try await self.withMockedToolchain { + try await self.installMockedToolchain(selector: "6.0.0") + + let versionFile = URL(fileURLWithPath: FileManager.default.currentDirectoryPath).appendingPathComponent(".swift-version") + try "6.0.0".write(to: versionFile, atomically: true, encoding: .utf8) + + var update = try self.parseCommand(Update.self, ["update", "-y", "--no-verify", "--post-install-file=\(Swiftly.currentPlatform.getTempFilePath().path)"]) + try await update.run() + + let versionFileContents = try String(contentsOf: versionFile, encoding: .utf8) + let inUse = try ToolchainVersion(parsing: versionFileContents) + XCTAssertGreaterThan(inUse, .init(major: 6, minor: 0, patch: 0)) + + // Since the global default was set to 6.0.0, and that toolchain is no longer installed + // the update should have unset it to prevent the config from going into a bad state. + let config = try Config.load() + XCTAssertTrue(config.inUse == nil) + + // The new toolchain should be installed + XCTAssertTrue(config.installedToolchains.contains(inUse)) + } + } + } + /// Verifies that snapshots, both from the main branch and from development branches, can be updated. func testUpdateSnapshot() async throws { let snapshotsAvailable = try await self.snapshotsAvailable()