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

treewide: `set -u` everywhere #72347

Merged
merged 13 commits into from Nov 5, 2019
Merged

treewide: `set -u` everywhere #72347

merged 13 commits into from Nov 5, 2019

Conversation

@Ericson2314
Copy link
Member

Ericson2314 commented Oct 31, 2019

2 years or so ago I made stdenv and cc-wrapper use set -u, which enormously helped with debugging. But I didn't want to change all of nixpkgs so I added lots of temporary set +u-ing.

Well, I think it's now time to bring the benefit to all of nixpkgs. Why? Because we have another big repo-wide refactor going on #72074. This will make that much easier to debug, and hopefully get some momentum going for stdenv tech debt fixing. Everyone chip in!

In particular, I think this is the easiest stdenv refactor we'd like to make, Why? This has the nice property that programs that continue to not fail have the same behavior. [Actually, this is not true with trap to catch failures or no set -e, but we use set -e and still fail after trap-ing.] That means it's very easy to debug; no getting a bunch of things working only to find some deeper dependency was built wrong and needing to backtrack, and no needing to debug complex failures.

I swear there was an issue for thus, but I couldn't find it.

Hydra job: https://hydra.nixos.org/jobset/nixpkgs/bash-no-undef-vars#tabs-evaluations

TODO:

  • Fix the broken things

  • Write release notes

@FRidh

This comment was marked as resolved.

Copy link
Member

FRidh commented Oct 31, 2019

How likely is it to break things and how much as been tested? (Nevermind, I see the Hydra job now...)

@Mic92

This comment has been minimized.

Copy link
Contributor

Mic92 commented Oct 31, 2019

This should be also mentioned in the release notes as it can break out-of-tree packages.

@Ericson2314 Ericson2314 changed the title treewide: `set -u` everywhere WIP: treewide: `set -u` everywhere Oct 31, 2019
@Ericson2314

This comment has been minimized.

Copy link
Member Author

Ericson2314 commented Oct 31, 2019

Wow, is there no way to mark a PR draft after the fact?!?! Embarrising, GitHub.

@Ericson2314

This comment has been minimized.

Copy link
Member Author

Ericson2314 commented Nov 1, 2019

Hydra reached the point where the legitimate failures were buried amidst all sorts of spurious failures. This makes it harder to use it. I think it might be ready to go to staging then, at least after this these newer evals get further? @globin @matthewbauer What do you think?

@Ericson2314 Ericson2314 changed the title WIP: treewide: `set -u` everywhere treewide: `set -u` everywhere Nov 2, 2019
@Ericson2314

This comment has been minimized.

Copy link
Member Author

Ericson2314 commented Nov 2, 2019

https://hydra.nixos.org/eval/1552118?compare=1551936&full=1#tabs-now-fail here is a comparison to the base commit, which is a nixpkgs-unstable eval.

@FRidh FRidh added this to WIP in Staging Nov 2, 2019
My earlier sed missed this because it already had `{..}`.
Staging automation moved this from WIP to Ready Nov 4, 2019
I don't want to just rebase this away because the original commit is
also in #72074.
@Ericson2314 Ericson2314 merged commit acd2d19 into staging Nov 5, 2019
1 check was pending
1 check was pending
grahamcofborg-eval Checking original out paths
Details
Staging automation moved this from Ready to Done Nov 5, 2019
@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Nov 5, 2019

Darwin fails on the bash-no-undef-vars branch https://hydra.nixos.org/build/105765822

@Ericson2314

This comment has been minimized.

Copy link
Member Author

Ericson2314 commented Nov 5, 2019

@FRidh that is a commit pushed post merge. The job is now just for #72812 . Do ping me with any/all set -u related failures on staging though, and I will try to watch it too.

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Nov 13, 2019

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

https://discourse.nixos.org/t/nixos-weekly-14-hercules-ci-launch-performance-improvements-in-nixpkgs-single-dependency-kubernetes-clusters/4769/1

@Mic92

This comment has been minimized.

Copy link
Contributor

Mic92 commented Nov 19, 2019

qt needs a fix: #73586 (comment) in pkgs/development/libraries/kde-frameworks/extra-cmake-modules/setup-hook.sh

untested patch:

diff --git a/pkgs/development/libraries/kde-frameworks/extra-cmake-modules/setup-hook.sh b/pkgs/development/libraries/kde-frameworks/extra-cmake-modules/setup-hook.sh
index 35982e86628..0ad185a16eb 100644
--- a/pkgs/development/libraries/kde-frameworks/extra-cmake-modules/setup-hook.sh
+++ b/pkgs/development/libraries/kde-frameworks/extra-cmake-modules/setup-hook.sh
@@ -77,6 +77,8 @@ ecmUnseenHostPath() {
 ecmHostPathHook() {
     ecmUnseenHostPath "$1" || return 0
 
+    declare -a qtWrapperArgs
+
     local xdgConfigDir="$1/etc/xdg"
     if [ -d "$xdgConfigDir" ]
     then
@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Nov 19, 2019

qtWrapperArgs is fixed with ef43c5f. Now there are issues left with the setup hooks of both plasma5 packages and kdeFrameworks packages.
pkgs/desktops/plasma-5/default.nix
pkgs/development/libraries/kde-frameworks/default.nix
Looked into this briefly, #73756

@Ericson2314

This comment has been minimized.

Copy link
Member Author

Ericson2314 commented Nov 20, 2019

Actually my commit could not be enough, but @Mic92's won't work either. The issue is bash says an undefined variable and and undefined variable are the same thing (which is terrible!!!).

The direct workaround is awful ${foo+${foo[@]}}, but thankfully there are other ways like ${#foo[@]} to get the same result.

@eraserhd eraserhd mentioned this pull request Nov 25, 2019
1 of 10 tasks complete
jtojnar referenced this pull request Dec 29, 2019
Bash takes an assignment of a string to an array variable:

local -a user_args
user_args="(foo bar)"

to mean appending the string to the array, not parsing the string into
an array as is the case when on the same line as the declaration:

local -a user_args="(foo bar)"

b063340 extracted the declaration before
the newly branched code block, causing string makeWrapperArgs being added
to the array verbatim.

Since local is function scoped, it does not matter if we move it inside
each of the branches so we fix it this way.
@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Dec 29, 2019

Hmm, it appears this is not enough. For example wrapPython setup hook is running with +o nounset: a6bb2ed#commitcomment-36605469

In fact, when I comment out

# Disable nounset for nix-shell.
set +u

and try to build python3.pkgs.setuptools, it fails on trying to build xz:

/nix/store/aknix5zw9cj7hd1m3h1d6nnmncl1vkvn-multiple-outputs.sh: line 55: shareDocName: unbound variable
@Ericson2314

This comment has been minimized.

Copy link
Member Author

Ericson2314 commented Dec 29, 2019

Indeed the work isn't done! https://github.com/NixOS/nixpkgs/blob/master/pkgs/stdenv/generic/setup.sh#L1307-L1308 needs to be guarded by IN_NIX_SHELL so it doesn't mess up normal builds.

I should have made an issue earlier when I realized this, but didn't have the time to immediately start fixing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.