-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: don’t expand aliases in develop stdenv setup #10688
Conversation
@SuperSandro2000 since you and I were discussing this on NixOS/nixpkgs#290775 : how do you feel about a solution directly embedded in the nix cli itself, like this? |
@@ -610,7 +610,7 @@ struct CmdDevelop : Common, MixEnvironment | |||
} | |||
|
|||
else { | |||
script = "[ -n \"$PS1\" ] && [ -e ~/.bashrc ] && source ~/.bashrc;\n" + script; | |||
script = "[ -n \"$PS1\" ] && [ -e ~/.bashrc ] && source ~/.bashrc;\nshopt -u expand_aliases\n" + script + "\nshopt -s expand_aliases\n"; |
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.
I may have gotten the point of that [ -n $PS1 ]
thing wrong--should it be included in that test or not?
This fixes NixOS/nixpkgs#290775 by not expanding aliases when sourcing the stdenv setup script. The way bash handles aliases is to expand them when a function is defined, not when it is used. I.e.: $ alias echo="echo bar " $ echo foo bar foo $ xyzzy() { echo foo; } $ shopt -u expand_aliases $ xyzzy bar foo $ xyzzy2() { echo foo; } $ xyzzy2 foo The problem is that ~/.bashrc is sourced before the stdenv setup, and bashrc commonly sets aliases for ‘cp’, ‘mv’ and ‘rm’ which you don’t want to take effect in the stdenv derivation builders. The original commit introducing this feature (5fd8cf7) even mentioned this very alias. The only way to avoid this is to disable aliases entirely while sourcing the stdenv setup, and reenable them afterwards.
Strategically we should be deferring the entire stdenv shell logic to Nixpkgs, but until we have #7501, we can be pragmatic about merging improvements. |
Thanks @hraban! |
THanks, and agreed. I have actually spent the last week trying to get this working directly in stdenv/setup.sh somehow instead of here but I just couldn't hack it. |
ok |
N.B.: I don't know anything about C++ so I'm ill equipped to judge the quality of this PR or write any tests for it. I don't even know how to cleanly reflow this now very long line. It's more of a POC to open the floor for discussion, than anything else, really.
Motivation
This fixes NixOS/nixpkgs#290775 by not expanding aliases when sourcing the stdenv setup script.
Context
The way bash handles aliases is to expand them when a function is defined, not when it is used. I.e.:
The problem is that ~/.bashrc is sourced before the stdenv setup, and bashrc commonly sets aliases for ‘cp’, ‘mv’ and ‘rm’ which you don’t want to take effect in the stdenv derivation builders. The original commit introducing this feature (5fd8cf7) even mentioned this very alias.
The only way to avoid this is to disable aliases entirely while sourcing the stdenv setup, and reenable them afterwards.
Example of this bug occurring in this very repo:
And now I have to confirm
cp
many many times to actually get anywhere.Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.