Repository navigation
Fix installations going out of sync - #607
Conversation
|
|
||
| try await ctx.output(installInfo) | ||
| return (nil, pathChanged) | ||
| await ctx.message("Toolchain \(version) could not be located in \(toolchainBinDir).") |
There was a problem hiding this comment.
suggestion (non-blocking): This can give a more actionable error message indicating that swiftly uninstall <version> followed by swiftly install <version> could get everything back in order for this toolchain version.
| #expect(before == after) | ||
| } | ||
|
|
||
| /// Tests that a toolchain that is installed according to the config, but missing from disk, is installed again. |
There was a problem hiding this comment.
praise: It's always good to see another test for edge cases, especially ones that have a direct user impact.
|
|
||
| try await ctx.output(installInfo) | ||
| return (nil, pathChanged) | ||
| await ctx.message("Toolchain \(version) could not be located in \(toolchainBinDir).") |
There was a problem hiding this comment.
Does this compile? This function is supposed to return a tuple but I don't see this returning anything?
There was a problem hiding this comment.
Hmm there is only 1 func message in the whole repo, it's this one, and it doesn't return anything. You must be thinking about some other function I guess?
Should I change this func message's behavior?
There was a problem hiding this comment.
No, execute returns the (String?, Bool). I was thinking that this was at the bottom of the function since I misread the indentation level.
| try await Use.execute(ctx, version, globalDefault: false, verbose: verbose, &config) | ||
| } else { | ||
| false | ||
| if config.installedToolchains.contains(version) { |
There was a problem hiding this comment.
Once we have the return value, I think that a guard here is easier to read.
e.g:
guard config.installedToolchains.contains(version) else {
await ctx.message("Toolchain \(version) could not be located in \(toolchainBinDir)")
<return (nil, false)>
}
// everything else ...There was a problem hiding this comment.
I mean the suggested code wouldn't really match the logic there 🤔.
There was a problem hiding this comment.
doh, I misread the indentation level.
ce324ea to
aa91b0b
Compare
|
Looks like some auto-formatting changes sneaked into the last commit (presumably due to my VSCode auto-formatting the file, as I've set it to, using swift-format). I've amended those formatting changes. |
| guard try await fs.exists(atPath: toolchainBinDir) else { | ||
| throw SwiftlyError( | ||
| message: | ||
| "Toolchain \(version) could not be located in \(toolchainBinDir). You can try `swiftly install \(version.name)` to install it again." |
There was a problem hiding this comment.
issue: this is a bit misleading now because you'd have to uninstall the toolchain to clear it out of config.json first, and then install it again to download and sync up the config.json with the toolchain directory in the fs.
Fixes #439.
The issue manifests like so: https://github.com/swift-dns/swift-idna/actions/runs/36990986233/job/110786900189?pr=53 (similar to the linked issue above).
Claude's report
Swiftly considers a toolchain installed as soon as it is listed in
installedToolchainsofconfig.json:swiftly/Sources/Swiftly/Install.swift
Line 253 in c8cf2e3
If the toolchain's files are gone while
config.jsonstill lists it (removed by hand, a differentSWIFTLY_TOOLCHAINS_DIR, or a CI cache that restored~/.swiftlybut not the toolchains directory),swiftly installreports it as already installed.swiftly usethen fails while listing the toolchain'susr/binto set up the proxies:swiftly/Sources/Swiftly/Install.swift
Lines 161 to 162 in c8cf2e3
This patch makes
swiftly installinstall such a toolchain again instead of reporting it as already installed:It also makes the proxy setup of
swiftly useandswiftly linkreport the missing toolchain, the wayproxyEnvironmentalready does, instead of surfacing the Foundation error.Both cases are covered by new tests.