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: don’t expand aliases in develop stdenv setup #10688

Merged
merged 1 commit into from
May 20, 2024

Conversation

hraban
Copy link
Contributor

@hraban hraban commented May 13, 2024

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.:

$ 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.

Example of this bug occurring in this very repo:

mbp23:nix user$ nix develop
 ↳ mbp23:nix user$ dontUnpack=1
 ↳ mbp23:nix user$ genericBuild
Running phase: patchPhase
Running phase: autoreconfPhase
autoreconf: export WARNINGS=
autoreconf: Entering directory '.'
autoreconf: configure.ac: not using Gettext
autoreconf: running: aclocal --force
autoreconf: configure.ac: tracing
autoreconf: configure.ac: not using Libtool
autoreconf: configure.ac: not using Intltool
autoreconf: configure.ac: not using Gtkdoc
autoreconf: running: /nix/store/mn7d82dx9xfini8fv5c6f8b7xy9g4sjc-autoconf-2.71/bin/autoconf --force
configure.ac:1: warning: AC_INIT: not a literal: "m4_esyscmd(bash -c "echo -n $(cat ./.version)$VERSION_SUFFIX")"
autoreconf: running: /nix/store/mn7d82dx9xfini8fv5c6f8b7xy9g4sjc-autoconf-2.71/bin/autoheader --force
autoreconf: configure.ac: not using Automake
autoreconf: Leaving directory '.'
Running phase: updateAutotoolsGnuConfigScriptsPhase
Updating Autotools / GNU config script to a newer upstream version: ./config/config.sub
cp: overwrite './config/config.sub'?

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.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label May 13, 2024
@hraban
Copy link
Contributor Author

hraban commented May 13, 2024

@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";
Copy link
Contributor Author

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?

@hraban hraban changed the title feat: don’t expand aliases in develop stdenv setup fix: don’t expand aliases in develop stdenv setup May 13, 2024
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.
@hraban hraban marked this pull request as ready for review May 13, 2024 02:09
@hraban hraban requested a review from edolstra as a code owner May 13, 2024 02:09
@roberth
Copy link
Member

roberth commented May 20, 2024

Strategically we should be deferring the entire stdenv shell logic to Nixpkgs, but until we have #7501, we can be pragmatic about merging improvements.

@roberth roberth merged commit 20ed0c0 into NixOS:master May 20, 2024
9 checks passed
@roberth
Copy link
Member

roberth commented May 20, 2024

Thanks @hraban!

@hraban
Copy link
Contributor Author

hraban commented May 20, 2024

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, 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.

@Rahul75-dev
Copy link

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants