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

stdenv: bump required Bash version from 4 to 5 #340765

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

tie
Copy link
Member

@tie tie commented Sep 9, 2024

Description of changes

Currently stdenv requires Bash 4.x that was released in 2009. This change bumps the required version to Bash 5.x (2019, 5 years ago).

See https://mywiki.wooledge.org/BashFAQ/061 for more details.

Using a relatively modern Bash version allows us to rely on newer features (e.g. ${var@a}) and remove workarounds for older quirks (e.g. https://stackoverflow.com/a/7577209, “old bash empty array problem”). Note that many setup hooks are using features added after 4.0 version, e.g. makeWrapper uses ${var@Q} from 4.4, but some even require Bash >5.0, e.g. cargoBuildHook uses ${var@U} from 5.1.

References:

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Currently stdenv requires Bash 4.x that was released in 2009. This
change bumps the required version to Bash 5.x (2019, 5 years ago).

See https://mywiki.wooledge.org/BashFAQ/061 for more details.

Using a relatively modern Bash version allows us to rely on newer
features (e.g. ${var@a}) and remove workarounds for older quirks (e.g.
https://stackoverflow.com/a/7577209, “old bash empty array problem”).
Note that many setup hooks are using features added after 4.0 version,
e.g. makeWrapper uses ${var@Q} from 4.4, but some even require >5.0,
e.g. cargoBuildHook uses ${var@U} from 5.1.
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Sep 9, 2024
@emilazy
Copy link
Member

emilazy commented Sep 9, 2024

Honestly it’s unclear to me why we even support Bash versions other than the one in Nixpkgs. I guess with direnv or similar you can end up loading stdenv stuff into a non‐Nixpkgs shell, but we already don’t support e.g. macOS’s Bash version. (In other words, 👍)

@tie
Copy link
Member Author

tie commented Sep 10, 2024

Honestly it’s unclear to me why we even support Bash versions other than the one in Nixpkgs.

IIRC nix-shell uses Bash from the user environment (i.e. PATH) in some cases. I don’t think that’s the case for nix develop though.

See also https://nix.dev/manual/nix/2.23/command-ref/nix-shell.html#environment-variables:~:text=Defaults%20to%20the%20bash,PATH%20if%20not%20found%2E

@tie tie marked this pull request as ready for review September 10, 2024 02:39
@tie tie requested a review from Ericson2314 as a code owner September 10, 2024 02:39
@tie tie requested a review from roberth September 10, 2024 02:40
@tie
Copy link
Member Author

tie commented Sep 10, 2024

cc @roberth in case you know of some other cases on the Nix side that this change may affect.

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I support this change. I do think that it deserves a mention in the stdenv docs, likely in the "Tools provided by stdenv" section.

@wolfgangwalther
Copy link
Contributor

If we use features that require bash 4.4 or bash 5, we can't claim to support bash 4, so 👍.

but some even require Bash >5.0, e.g. cargoBuildHook uses ${var@U} from 5.1.

Does this mean we need to check for at least 5.1, too?

@Mindavi
Copy link
Contributor

Mindavi commented Sep 10, 2024

cc @reckenrode for a view from the Darwin side? I think the main reason this was not done for long was due to Darwin, but I don't know any details.

@emilazy
Copy link
Member

emilazy commented Sep 10, 2024

macOS ships an ancient Bash claiming to be 3.2.57(1)-release, so I don’t think there’s a reason for it to block here. IIRC it might backport some Bash 4 features, but I doubt enough to make things actually work, and it would trigger the previous error anyway.

@reckenrode
Copy link
Contributor

reckenrode commented Sep 10, 2024

Apple ships an ancient Bash because Apple stopped updating GNU tools when they switched to GPLv3. Others have been replaced (e.g., libiconv, patch_cmds), but Apple continues to ship the ancient Bash along with Zsh instead of replacing it.

I don’t know why Darwin was an issue in the past. It’s possible that the bootstrap tools shipped Bash 4, but they were updated in #302387, so that should not be an issue. The bootstrap tools now have Bash 5.2. The Darwin bootstrap also rebuilds Bash very early and uses that in preference to the bootstrap tools Bash.

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Sep 10, 2024
@tie
Copy link
Member Author

tie commented Sep 11, 2024

but some even require Bash >5.0, e.g. cargoBuildHook uses ${var@U} from 5.1.

Does this mean we need to check for at least 5.1, too?

In this particular case, we can use ${var^^} (Bash 4.0) instead of ${var@U} 😅

Copy link
Contributor

@Mindavi Mindavi left a comment

Choose a reason for hiding this comment

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

Then I think this is good.

@roberth
Copy link
Member

roberth commented Sep 11, 2024

cc @roberth in case you know of some other cases on the Nix side that this change may affect.

I expect no issues despite Nix's bad coupling with stdenv, which it shouldn't have.
Which reminds me to subtly announce:

In Nixpkgs we now have pkgs.devShellTools which can generate a shell runner that is more contribution friendly because it does not come with the interface stability expectations that code built into Nix has. It already works quite well for non-structuredAttrs derivations.

@philiptaron philiptaron merged commit f4cd623 into NixOS:staging Sep 11, 2024
38 of 40 checks passed
@tie tie deleted the stdenv-bash-version branch September 12, 2024 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants