Skip to content

treewide: bash -> bashNonInteractive & bashInteractive -> bash#151227

Closed
Artturin wants to merge 3 commits intoNixOS:stagingfrom
Artturin:bashinteractivetobash
Closed

treewide: bash -> bashNonInteractive & bashInteractive -> bash#151227
Artturin wants to merge 3 commits intoNixOS:stagingfrom
Artturin:bashinteractivetobash

Conversation

@Artturin
Copy link
Member

stdenv changed due to
error: output '/nix/store/...-stdenv-linux' is not allowed to refer to the following paths:
/nix/store/...-ncurses-6.2
/nix/store/...-readline-8.1p0
/nix/store/...-bash-interactive-5.1-p12

Motivation for this change

Closes #59209

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment labels Dec 18, 2021
@Artturin Artturin force-pushed the bashinteractivetobash branch 3 times, most recently from c3d266e to 5af1ecc Compare December 18, 2021 20:26
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 8.has: package (new) This PR adds a new package labels Dec 18, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Dec 18, 2021
@Artturin Artturin requested a review from vcunat December 18, 2021 21:12
@Artturin
Copy link
Member Author

Artturin commented Dec 18, 2021

for some reason darwin stdenv.shellPackage broke

@Artturin
Copy link
Member Author

if i remove ${stdenv.shellPackage} from bash/5.1.nix then there is no error: attribute 'shellPackage' missing error

@Artturin
Copy link
Member Author

diff --git a/pkgs/shells/bash/5.1.nix b/pkgs/shells/bash/5.1.nix
index ef734ffdbc1..129bc5e29cb 100644
--- a/pkgs/shells/bash/5.1.nix
+++ b/pkgs/shells/bash/5.1.nix
@@ -91,11 +91,11 @@ stdenv.mkDerivation rec {
     rm -f $out/lib/bash/Makefile.inc
   '';
 
-  postFixup =
+  preFixup =
     if interactive
     then ''
       substituteInPlace "$out/bin/bashbug" \
-        --replace '${stdenv.shellPackage}/bin/sh' "$out/bin/sh"
+        --replace '/bin/sh' "$out/bin/sh"
     ''
     # most space is taken by locale data
     else ''

works as a workaround

@Artturin Artturin changed the base branch from master to staging December 18, 2021 23:32
@dnadlinger
Copy link
Contributor

Apologies for this substance-less drive-by comment, but I believe "noninteractive" would be the more widely used term.

@Artturin Artturin requested a review from Ericson2314 December 19, 2021 01:53
@hmenke
Copy link
Member

hmenke commented Dec 19, 2021

@dnadlinger No, yours is not just a substance-less drive-by comment. In fact this is a very important usability concern and the PR should not go ahead with the nonsensical made-up word “uninteractive”, but for sure use the valid dictionary word “noninteractive”.

currently bashInteractive has noninteractive bash in its dependencies
this fixes that
@Artturin Artturin force-pushed the bashinteractivetobash branch from 49d6b98 to dc813a0 Compare December 19, 2021 18:16
@Artturin Artturin changed the title treewide: bash -> bashUninteractive & bashInteractive -> bash treewide: bash -> bashNonInteractive & bashInteractive -> bash Dec 19, 2021
@Artturin Artturin force-pushed the bashinteractivetobash branch 2 times, most recently from b8728a1 to c7f066b Compare December 20, 2021 16:41
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe lowercase? It was lowercase before.

Suggested change
name = "bash-${optionalString (!interactive) "Noninteractive-"}${version}-p${toString (builtins.length upstreamPatches)}";
name = "bash-${optionalString (!interactive) "noninteractive-"}${version}-p${toString (builtins.length upstreamPatches)}";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same. (lowercase?)

stdenv changed due to
error: output '/nix/store/...-stdenv-linux' is not allowed to refer to the following paths:
         /nix/store/...-ncurses-6.2
         /nix/store/...-readline-8.1p0
         /nix/store/...-bash-interactive-5.1-p12
@Artturin Artturin force-pushed the bashinteractivetobash branch from c7f066b to 12a07be Compare December 21, 2021 21:04
@tomberek tomberek mentioned this pull request Feb 12, 2022
8 tasks
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 10, 2022
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/setting-for-using-bash-as-default-shell/23939/3

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 11, 2022
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 18, 2023
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Mar 9, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 9, 2024
@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@damccull
Copy link
Contributor

Is this going anywhere? After a rabbit hole of discovery on this bashInteractive issue, having it be the default would have made nixos way easier and better for this noob.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 23, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 1, 2025
@infinisil
Copy link
Member

#379368

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Feb 4, 2025
@Artturin Artturin closed this Feb 4, 2025
infinisil added a commit to tweag/nixpkgs that referenced this pull request Feb 4, 2025
The status quo of `bash` not being interactive is frustrating for many users,
because trying to use it interactively is just messed up, and
`bashInteractive` is not intuitive and barely discoverable.

This was brought to my (and many others) attention by @stahnma in his
[talk at CfgMgmtCamp 2025](https://cfp.cfgmgmtcamp.org/ghent2025/talk/YUVUTN/),
where he highlighted this as one of the frustrations he ran into when
learning Nix.

Why this is fine:
- No reason for not making interactive the default was given in the original commit (6c6ff6f), but probably it was due to the increase in closure size
- The closure size only increases by 6.9MiB (19.5%) today, with the
  added dependency on the store paths for readline and ncurses, which
  are needed on systems in almost all cases anyways
- If somebody really needs to get a more minimal system, they can use
  the newly-introduced `bashNonInteractive` instead now
- Though to apply it consistently, they'll need to do that in an
  overlay like
  ```
  final: prev: {
    bash = self.bashNonInteractive;
  }
  ```

  Or alternatively using the `system.replaceDependencies.replacements`
  NixOS option approach.

While there's also other such `*Interactive` packages that could use the
same treatment, `bash` is a great start.

This was already attempted before in
NixOS#151227, but was not continued for
unknown reason.

To avoid stdenv becoming bigger, all uses of bash in the (working)
stdenv's are changed to the explicitly non-interactive version here.

This commit will however still cause a mass rebuild for all packages (and reverse deps)
making use of the default bash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bash vs bashInteractive not discoverable

7 participants