Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make updates better and more resilient #245

Merged
merged 3 commits into from
Mar 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/LinuxPlatform/Linux.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
4 changes: 2 additions & 2 deletions Sources/MacOSPlatform/MacOS.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand Down
11 changes: 9 additions & 2 deletions Sources/Swiftly/Install.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we only want to do this when the user passes --use to install? In other words if --use was omitted does this still make sense?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it will be good to try and set a default global toolchain whenever we can to help users avoid cases where there's no default toolchain to fallback. It's an extra annoyance that most people probably don't care to have.

"Give me some kind of a swift toolchain please. I know that I have at least one installed in there."

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)
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/Swiftly/Proxy.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 <toolchain version>` 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 <toolchain version>` or install a new toolchain to use with `swiftly install --use <toolchain version>`.")
}

// Prevent circularities with a memento environment variable
Expand Down
2 changes: 1 addition & 1 deletion Sources/Swiftly/Run.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 <toolchain version>` 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 <toolchain version>` or install a new toolchain to use with `swiftly install --use <toolchain version>`.")
}

do {
Expand Down
18 changes: 14 additions & 4 deletions Sources/Swiftly/Uninstall.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we going to allow swiftly then to be able to uninstall toolchains it didn't have a hand in installing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the toolchain is in the inUse property of the config.json then it probably did have a hand in installing the toolchain at some point in time. This includes it in the list of installed toolchains for the purposes of the uninstall command. Below swiftly will expunge it from the config, and try once again to remove it from disk.

}
toolchains = installedToolchains
}

guard !toolchains.isEmpty else {
Expand Down Expand Up @@ -108,18 +113,23 @@ 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: "")
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, verbose: verbose)
SwiftlyCore.print("done")
}
}
10 changes: 5 additions & 5 deletions Sources/Swiftly/Update.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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?
Expand All @@ -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 {
Expand Down
13 changes: 10 additions & 3 deletions Sources/Swiftly/Use.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm: You shouldn't be able to select a toolchain that wasn't installed by swiftly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a rare case, caused by things like improper handling of the update operation that this PR should solve. Another potential case is race conditions. This is pure hardening of the code, and also giving the user a way back to a nominal state by using a toolchain that is in the list that swiftly installed.

}

return (defaultInUse, .globalDefault)
}
5 changes: 4 additions & 1 deletion Sources/SwiftlyCore/Platform.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions Tests/SwiftlyTests/PlatformTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
18 changes: 18 additions & 0 deletions Tests/SwiftlyTests/UninstallTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
}
}
}
34 changes: 31 additions & 3 deletions Tests/SwiftlyTests/UpdateTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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()
Expand Down