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.

Sign up for GitHub

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

Jump to bottom

stdenv: export minimum bash version for Nix #299490

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

stdenv: export minimum bash version for Nix #299490

wants to merge 1 commit into from

Conversation

iFreilicht
Copy link
Contributor

@iFreilicht iFreilicht commented Mar 27, 2024 •
edited
Loading

Relates to NixOS/nix#10263 and #296936

This will help external tools (Nix and direnv for now) perform the same version check setup.sh is already doing when building.

Description of changes

setup.sh now exports a new variable NIX_ENV_MIN_BASH_VERSION with the value 4. This is the single source of truth now.

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.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@iFreilicht iFreilicht requested a review from Ericson2314 as a code owner March 27, 2024 13:16
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Mar 27, 2024
@iFreilicht
stdenv: export minimum bash version for Nix
6cc2e99 
Relates to NixOS/nix#10263 and NixOS#296936

This will help external tools (Nix and direnv for now) perform the same
version check setup.sh is already doing when building.
@iFreilicht iFreilicht force-pushed the add-nix-min-bash-version-variable branch from 9d84daf to 6cc2e99 Compare March 27, 2024 13:19
@iFreilicht iFreilicht mentioned this pull request Mar 27, 2024
Open
@iFreilicht iFreilicht marked this pull request as draft March 28, 2024 22:45
@iFreilicht
Copy link
Contributor Author

Converted to draft until Nix Team makes a decision on NixOS/nix#10263.

thufschmitt pushed a commit to NixOS/nix that referenced this pull request Mar 29, 2024
develop: Allow shells to declare the minimal bash version they require
16175e0 
Allow derivations to export a `NIX_ENV_MIN_BASH_VERSION` that sets the
minimal (major) bash version that is required to parse their setup
script, and use that to gracefully fail if the current bash is too old.

Fix #10263 (from the Nix side at least, needs NixOS/nixpkgs#299490 from the Nixpkgs side to be useful in practice)
@thufschmitt thufschmitt mentioned this pull request Mar 29, 2024
Open
thufschmitt
thufschmitt reviewed Mar 29, 2024
View reviewed changes
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Looks good!

Companion PR on the Nix side: NixOS/nix#10359

For the sake of bikeshedding (mostly): Could/should this be exposed from the Nix language instead (meta.minBashVersion or something)? It would have the advantage of not polluting the scope even more than what we do.

@iFreilicht
Copy link
Contributor Author

Hmm, the main issue I see with this is how to keep that variable in sync with the check that setup.sh does itself, otherwise you'd run the risk of a future change silently breaking this feature again if we forget that the value is defined in two places.

We currently don't substitute any part of setup.sh (by default) AFAICT, the way to bring data in to that script is via environment variables, at which point the namespace pollution isn't solved at all. Doing string substitution on setup.sh is potentially possible, but that seems to make the whole thing a lot more complicated than it needs to be.

@iFreilicht iFreilicht marked this pull request as ready for review March 30, 2024 11:19
@thufschmitt
Copy link
Member

Hmm, the main issue I see with this is how to keep that variable in sync with the check that setup.sh does itself, otherwise you'd run the risk of a future change silently breaking this feature again if we forget that the value is defined in two places.

Erk, right :/
Let's forget about it then

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 1, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thufschmitt thufschmitt thufschmitt left review comments

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

Assignees
No one assigned
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iFreilicht @thufschmitt @wegank