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

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

Merged
merged 1 commit into from
Mar 31, 2024

Conversation

paperclover
Copy link
Member

replacing the binary using bun upgrade is problematic, as you cannot delete a binary that is currently running.

my previous solution used a subprocess that outlived the upgrader, but the only way to do this on windows will create a console window for the process. "okay sure, it will flicker in the taskbar but all is fine", i thought.

it seems this window steals user focus for some reason:

image

and

image

yikes. i'm going to remove this behavior for now and if this problem can be solved then we can do so later. the new behavior is equal to what some other software does, for example deno upgrade.

Copy link
Contributor

Copy link
Contributor

❌🪟 @paperdave, there are 12 test regressions on Windows x86_64

  • test\cli\install\bun-add.test.ts
  • test\cli\install\bun-run-bunfig.test.ts
  • test\cli\install\bunx.test.ts
  • test\cli\install\registry\bun-install-registry.test.ts
  • test\js\bun\console\console-iterator.test.ts
  • test\js\node\dns\node-dns.test.js
  • test\js\node\http2\node-http2.test.js
  • test\js\node\stream\node-stream.test.js
  • test\js\web\fetch\body-stream.test.ts
  • test\js\web\websocket\websocket.test.js
  • test\js\third_party\postgres\postgres.test.ts
  • test\js\web\workers\worker.test.ts

Full Test Output

// 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).

@paperclover
Copy link
Member Author

im going to merge this in interest in having it ready for 1.1. while leaving some data on disk isn't perfectly ideal, it is better than the status quo.

and again i don't feel like this is particularly bad considering other software has taken this route.

@paperclover paperclover merged commit 456a323 into main Mar 31, 2024
26 of 31 checks passed
@paperclover paperclover deleted the dave/win/upgrade-no-cleanup branch March 31, 2024 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants