fix(bifrost): do not create ~/.bash_profile during install - #712
Open
Jon-Becker wants to merge 1 commit into
Open
fix(bifrost): do not create ~/.bash_profile during install#712Jon-Becker wants to merge 1 commit into
Jon-Becker wants to merge 1 commit into
Conversation
The installer appended the PATH export to both ~/.bashrc and ~/.bash_profile using >>, which created ~/.bash_profile when it did not exist. On Debian/Ubuntu/WSL, bash login shells then read the new ~/.bash_profile instead of ~/.profile, silently dropping the user's startup configuration. Only append to bash profiles that already exist, matching the reporter's suggested fix. zsh and fish handling is unchanged. Adds bifrost/tests/profile-test which drives the real install script against an isolated HOME with network/binary steps stubbed to prove the absent ~/.bash_profile case does not create the file and that existing profiles receive the PATH update. Fixes #707
Contributor
❌ Eval Report for fe7e96a
|
Contributor
✅ Coverage Report for fe7e96a
|
Contributor
Benchmark for fe7e96aClick to view benchmark
|
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
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.
What changed? Why?
Fixes #707.
The
bifrost/installscript appended the PATH export to both~/.bashrcand~/.bash_profileusing>>. Because>>creates its target when absent, running the installer created~/.bash_profileon systems that did not have one. On Debian/Ubuntu/WSL,~/.profilenormally sources~/.bashrc, but a bash login shell reads~/.bash_profilefirst and stops there — so once the installer created that file, the user's~/.profile/~/.bashrcstartup configuration was silently dropped after the next login.The actual source matched the issue report (the bash
casebranch unconditionally wrote to both files). The fix loops over~/.bashrcand~/.bash_profileand only appends to a profile that already exists, so the installer never creates~/.bash_profileas a side effect. The existing PATH string, thezsh(~/.zshrc) andfish(config.fish) branches, and the "already on PATH" guard are all unchanged.Notes to reviewers
bifrost/install— replaced the two unconditional>>writes in the*/bash)branch with a loop that skips non-existent profiles ([ -f "$SHELL_PROFILE" ] || continue) and quotes the profile path. Only the bash branch changed.bifrost/tests/profile-test— new deterministic regression test (extensionless to match repo shell-script convention and avoid the*.shentry in.gitignore). It runs the real install script against an isolated$HOMEwithcurl/binary steps stubbed, so only the shell-profile logic executes.How has it been tested?
bash -n bifrost/installandbash -n bifrost/tests/profile-test— syntax valid.bash bifrost/tests/profile-test— 4/4 assertions pass:~/.bash_profileis not created,~/.bashrcreceives the PATH export,~/.bash_profileis still not created when only~/.bashrcexists,~/.bash_profilereceives the PATH export.installscript to confirm it is meaningful: it fails cases 1 and 3 (reproducing the reported bug) and passes after the fix.cargochecks are unaffected.