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.

Sign up for GitHub

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

fix(windows bun upgrade): skip cleaning up old binary #9696

Merged
merged 1 commit into from
Mar 31, 2024
Merged

fix(windows bun upgrade): skip cleaning up old binary #9696

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 6 additions & 40 deletions src/cli/upgrade_command.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -948,46 +948,12 @@ pub const UpgradeCommand = struct {

if (Environment.isWindows) {
if (outdated_filename) |to_remove| {
current_executable_buf[target_dir_.len] = '\\';
const delete_old_script = try std.fmt.allocPrint(
ctx.allocator,
// What is this?
// 1. spawns powershell
// 2. waits for all processes with the same path as the current executable to exit (including the current process)
// 3. deletes the old executable
//
// probably possible to hit a race condition, but i think the worst case is simply the file not getting deleted.
//
// in that edge case, the next time you upgrade it will simply override itself, fixing the bug.
//
// -NoNewWindow doesnt work, will keep the parent alive it seems
// -WindowStyle Hidden seems to just do nothing, not sure why.
// Using -WindowStyle Minimized seems to work, but you can spot a powershell icon appear in your taskbar for about ~1 second
//
// Alternative: we could simply do nothing and leave the `.outdated` file.
\\Start-Process powershell.exe -WindowStyle Minimized -ArgumentList "-NoProfile","-ExecutionPolicy","Bypass","-Command",'&{{$ErrorActionPreference=''SilentlyContinue''; Get-Process|Where-Object{{ $_.Path -eq ''{s}'' }}|Wait-Process; Remove-Item -Path ''{s}'' -Force }};'; exit
,
.{
destination_executable,
to_remove,
},
);

var delete_argv = [_]string{
"powershell.exe",
"-NoProfile",
"-ExecutionPolicy",
"Bypass",
"-Command",
delete_old_script,
};

_ = std.ChildProcess.run(.{
.allocator = ctx.allocator,
.argv = &delete_argv,
.cwd = tmpdir_path,
.max_output_bytes = 512,
}) catch {};
// TODO: this file gets left on disk
//
// We should remove it, however we cannot remove an exe that is still running.
// A prior approach was to spawn a subprocess to remove the file, but that
// would open a terminal window, which steals user focus (even if minimized).
_ = to_remove;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can't remove it can we move it (rename it). If we can rename it then we can on future runs detected the renamed file <oldfilename>_deleteme and then remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It turns out you can rename a running executable. We could follow a strategy as described here

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 specifically want to avoid checking for an old exe on every startup of bun.exe. I am already following those instructions except for the last one.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gvilums

It turns out you can rename a running executable.

Awww Yiss! From my Windows days I remember hacking around with renaming when it wouldn't let me delete.

@paperdave

Instead of checking for an old version on startup it can be done on the next upgrade call. If it's just a periodic cleanup.

Copy link
Member Author

Choose a reason for hiding this comment

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

on the next upgrade call

sure, however i don't see too much use here, as the bun.exe.outdated file will simply be re-ovewritten. deleting it only really does something in the case you do bun upgrade twice in a row.

Copy link
Contributor

Choose a reason for hiding this comment

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

True. There may be other lazy opportunities to perform cleanup. There could even be a once very so many executions try to cleanup or after running for so long try to cleanup (just so it stays out of the hot path).

}
}
}
Expand Down
Loading