fix: preserve freshly-installed v2 binary during v1 legacy cleanup#309
Merged
Conversation
The v1 (Python/PyInstaller) binary-zip layout installed `phase` alongside an
`_internal/` directory in /usr/local/bin. A v2 *binary* install lands in that
same directory, but cleanup_legacy() removed `${dir}/phase` whenever
`${dir}/_internal` was present — deleting the v2 binary it had just installed.
The installer still printed "installed successfully" and exited 0, leaving the
machine with no `phase` at all.
This only bit distros that fall through to the binary install path (Arch, and
any OS/distro without a matching deb/rpm/apk package); package installs put v2
in /usr/bin and were unaffected.
cleanup_legacy() now takes the absolute path of the just-installed binary and
never deletes it — guarding both the _internal removal loop and the
stale-symlink branch. Also drops the old, misleading package-manager advisory
blocks (they fired even after a successful v2 install).
Verified against real v1.21.3 and v2.3.0 release artifacts across binary,
deb, rpm and apk installs, real v1->v2 package upgrades, the stale-symlink
path, and idempotent re-runs.
Follow-up to the keep-guard fix, addressing review of #309: - Compare inodes (-ef) instead of paths when deciding whether ${dir}/phase is the just-installed binary. On the (exotic) topology where a loop dir is a symlink onto another (e.g. /usr/local/bin -> /usr/bin), a v1 leftover and the fresh v2 binary are the same physical file under two paths; the old string compare missed that and deleted v2 on the package path. Verified on dash and busybox. - Add a safety net: if $keep no longer exists after cleanup, die loudly instead of printing "installed successfully" over a broken install. This fails closed for any future regression in this class. - Drop the non-load-bearing string guard on the stale-symlink branch (a successful package install has already replaced the symlink with a regular file, so [ -L ] is false there); the -ef check now covers it.
Deploying phase-cli-install-script with
|
| Latest commit: |
f60eff3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://77d51908.phase-cli-install-script.pages.dev |
| Branch Preview URL: | https://fix-install-legacy-cleanup.phase-cli-install-script.pages.dev |
rohan-chaturvedi
approved these changes
Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The v1 (Python/PyInstaller) binary-zip install laid down
phaseand an_internal/directory side by side in/usr/local/bin. A v2 binary install lands in that same directory, butcleanup_legacy()deleted${dir}/phasewhenever${dir}/_internalexisted — removing the v2 binary it had just installed.The installer still printed
installed successfullyand exited0, so the machine was silently left with nophasecommand at all.This only affected distros that fall through to the binary install path — Arch, and any OS/distro without a matching deb/rpm/apk package. Package installs put v2 in
/usr/binand were unaffected.Reproduced (Arch, real v1.21.3 → v2.3.0 artifacts), old script
Fix
cleanup_legacy()now takes the absolute path of the just-installed binary (keep) and never deletes it — guarding both the_internalremoval loop and the stale-symlink branch. Also removes the old package-manager advisory blocks, which were misleading (they fired even right after a successful v2 install).Verification
Tested in containers against real v1.21.3 and v2.3.0 release artifacts:
_internalremoved/usr/local/bin/usr/bin/usr/bin/phasesymlink removed, v2 keptsh -nclean.