Skip to content

stdenv: remove bash version compatibility hack#130596

Merged
happysalada merged 1 commit intoNixOS:stagingfrom
happysalada:stdenv_remove_bash_compatibility_hack
Aug 30, 2021
Merged

stdenv: remove bash version compatibility hack#130596
happysalada merged 1 commit intoNixOS:stagingfrom
happysalada:stdenv_remove_bash_compatibility_hack

Conversation

@happysalada
Copy link
Copy Markdown
Contributor

@happysalada happysalada commented Jul 19, 2021

Motivation for this change

This removes a compatibility hack accross all version of bash prior to 4.4
In my understanding we won't be using older versions of bash anymore, so this is safe to remove.
For more informations check https://stackoverflow.com/questions/7577052/bash-empty-array-expansion-with-set-u

Note that there are other occurrences of this in the nixpkgs (notably in cargo-hooks) just search for the pattern Array+" to find them. I want to remove those at the same time, but I'm not sure those other occurrences can be safely removed.

I think this PR is uncontroversial

This is coming from d685a68

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Relase 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.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jul 19, 2021
@happysalada happysalada mentioned this pull request Jul 19, 2021
Merged
11 tasks
@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. 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 Jul 19, 2021
@Mindavi
Copy link
Copy Markdown
Contributor

Mindavi commented Jul 19, 2021

Which version of bash is macos using again? Wasn't that 3.2? Does that matter for the stdenv?

(I don't know much about the stdenv, just some questions)

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 19, 2021

Thank you for the review!
I hadn't realised actually, but the default version of bash on macos is still 3.2 (it seems to be because of a licensing issue).
The stdenv being built with 4.4 so it shouldn't matter I think.
Inside the darwin stdenv there is an allowedRequisites part.

      allowedRequisites = (with pkgs; [
        xz.out
        xz.bin
        gmp.out
        gnumake
        findutils
        bzip2.out
        bzip2.bin
        zlib.out
        zlib.dev
        libffi.out
        coreutils
        ed
        diffutils
        gnutar
        gzip
        ncurses.out
        ncurses.dev
        ncurses.man
        gnused
        bash
        gawk
        gnugrep
        patch
        pcre.out
        gettext
        binutils.bintools
        darwin.binutils
        darwin.binutils.bintools
        curl.out
        openssl.out
        libssh2.out
        nghttp2.lib
        brotli.lib
        cc.expand-response-params
        libxml2.out
      ] ++ lib.optional haveKRB5 libkrb5
      ++ lib.optionals localSystem.isAarch64 [
        pkgs.updateAutotoolsGnuConfigScriptsHook
        pkgs.gnu-config
      ])
      ++ (with pkgs."${finalLlvmPackages}"; [
        libcxx
        libcxx.dev
        libcxxabi
        libcxxabi.dev
        llvm
        llvm.lib
        compiler-rt
        compiler-rt.dev
        clang-unwrapped
        libclang.dev
        libclang.lib
      ])
      ++ (with pkgs.darwin; [
        dyld
        Libsystem
        CF
        cctools
        ICU
        libiconv
        locale
        libtapi
      ] ++ lib.optional useAppleSDKLibs objc4
      ++ lib.optionals doSign [ postLinkSignHook sigtool signingUtils ]);

bash is included in there. That would make me think that stdenv ships with a shell and that we don't need to care about older version.
There might be more to it than my interpretation though, it would be good to know who we can ask more informations to.

@happysalada happysalada changed the base branch from master to staging July 23, 2021 03:18
@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 23, 2021

@vcunat when you have a moment, I would like to see if that PR builds correctly.
(I've tried the build jobs on master and it worked). Last time even if the tests passed, it caused a regression, so I want to make sure I'm not breaking something with this PR.
When you have a moment, no rush. (once this PR is passed, I can have a go at the PR that introduced breakage with newline in flags).

@happysalada happysalada mentioned this pull request Aug 3, 2021
Closed
11 tasks
@happysalada happysalada merged commit fd89fb6 into NixOS:staging Aug 30, 2021
@happysalada happysalada deleted the stdenv_remove_bash_compatibility_hack branch August 30, 2021 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

Assignees

No one assigned

Labels

6.topic: stdenv Standard environment 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@happysalada @Mindavi