-
Notifications
You must be signed in to change notification settings - Fork 49
Add ability to self uninstall swiftly #344
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
base: main
Are you sure you want to change the base?
Add ability to self uninstall swiftly #344
Conversation
Hi @cmcgee1024 do you mind taking a look at this PR when you have time? Thanks! |
import SystemPackage | ||
import Testing | ||
|
||
@Suite struct SelfUninstallTests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: It's good to see the tests here for this functionality that are small and fast to run.
// Create a profile file with the source line | ||
let userHome = SwiftlyTests.ctx.mockedHomeDir! | ||
|
||
let profileHome: FilePath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Can this test invoke a swiftly init
to do the usual install behaviour instead of duplicating the logic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well yeah ofc, my thought was to keep the tests as independent as possible so future changes to swiftly init
won't interfere with something here, but i guess they are strongly coupled anyways. So what I did here was manually creating shell profiles only and see if the source lines are removed correctly. This might've been better if it's a unit test for that particular line of code removing the sourcelines:
for path in profilePaths {
if try await fs.exists(atPath: path) {
await ctx.print("Removing swiftly source line from \(path)...")
let isFishProfile = path.extension == "fish"
let sourceLine = isFishProfile ? fishSourceLine : shSourceLine
if case let profileContents = try String(contentsOf: path, encoding: .utf8), profileContents.contains(sourceLine) {
let newContents = profileContents.replacingOccurrences(of: sourceLine, with: "")
try Data(newContents.utf8).write(to: path, options: [.atomic])
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks good. I've added a few suggestions, and a question.
Hi @cmcgee1024 I just made the changes you requested! lmk if there's anything else :) |
README.md
Outdated
@@ -59,7 +59,12 @@ This command checks to see if there are new versions of swiftly itself and upgra | |||
|
|||
## Uninstalling swiftly | |||
|
|||
Currently, only manual uninstallation is supported. If you need to uninstall swiftly, please follow the instructions below: | |||
swiftly can be savely removed with the following command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: s/savely/safely/
here
} | ||
|
||
await ctx.print("Removing swiftly binary at \(swiftlyBin)...") | ||
try await fs.remove(atPath: swiftlyBin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (blocking): Thinking on this a little bit I think that we will need to take some extra care here. fs.remove
is a big hammer. It can remove important directories and files without warning or recourse. The user can set their SWIFTLY_BIN_DIR
to a shared directory with other binaries in it.
suggestion: Instead of removing the directory, how about removing the individual proxies by checking if they are symlinks to the swiftly binary, then remove the swiftly binary itself. Finally, if this directory is empty, then remove it.
The same kind of thing should probably be used for the SWIFTLY_HOME_DIR
too. Remove the config.json, the environment files, and then remove the directory if it is empty.
For toolchains directory, remove all of the known toolchains first, and then check if it is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's take advantage of the verbose flag here. If it's set then we report every file path that we removed.
try await SwiftlyTests.runCommand(Init.self, ["init", "--assume-yes", "--skip-install"]) | ||
|
||
let fishSourceLine = """ | ||
# Added by swiftly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: The user might reorganize their profile manually, or invoke some kind of tooling that does that. Some of these lines might exist, and others might not. Maybe a user removes the comment, leaving only the source line. We could perhaps match each line and remove them individually to make this a bit more robust and safe. Each one is unique enough that I think they shouldn't have too many false matches.
Sources/Swiftly/SelfUninstall.swift
Outdated
let contents = try String(contentsOf: path, encoding: .utf8) | ||
if contents.contains(sourceLine) { | ||
let updated = contents.replacingOccurrences(of: sourceLine, with: "") | ||
try Data(updated.utf8).write(to: path, options: [.atomic]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: If the verbose flag is set then output that the profile file was updated.
swiftly self-uninstall
self-uninstall
command #84Implements the command
swiftly self-uninstall
, the ability to self uninstall swiftly incase it's no longer needed.It deletes swiftly by removing everything under
swiftBinDir
andswiftHomeDir
, and also removes the lines in shell profiles added by swiftlyTODO:
Questions:
fs.remove(atPath: ...)
?Issue
Due to the way we mock home directories, the
removesEntryFromShellProfile
fails because any shell profiles.zprofile
,.profile
will be removed alongsideswiftlyBinDir
/swiftlyHomeDir
, so the expect present check fails. Need to come up a better way to managemockedTestHome
directories