Fix some nixos-rebuild lints #147449

l0b0 · 2021-11-26T02:18:10Z

Motivation for this change

Existing issues requesting ShellCheck compliance: #133088, #21166. Request from @Artturin.

Open questions

There's one warning-level issue still reported by ShellCheck:

In pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh line 93:
            mkdir -p -m 0755 "$(dirname "$profile")"
                     ^-- SC2174: When used with -p, -m only applies to the deepest directory.

The simplest "solution" would be to remove the -p, but I'm not sure whether I can assume that /nix/var/nix/profiles/system-profiles will always exist when running nixos-rebuild. The less simple solution would be to mkdir each of the ancestor directories in turn, but I'm not sure whether all of them should have mode 0755.

I'll take care of SSHOPTS in a separate PR, since it's a more risky change.

Things done

@Artturin ^

SuperSandro2000

I am not sure if it is worth switching all [ to [[. Also I personally would drop all the optional quotes inside the [[ and would also chain multiple up.

Maybe we should add a style guide entry before changing those to make sure we are all on the same track.

pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh

SuperSandro2000 · 2021-11-26T02:39:22Z

pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh

@@ -89,7 +90,7 @@ while [ "$#" -gt 0 ]; do
        fi
        if [ "$1" != system ]; then
            profile="/nix/var/nix/profiles/system-profiles/$1"
-            mkdir -p -m 0755 "$(dirname "$profile")"
+            mkdir -m 0755 "$(dirname "$profile")"


I am not sure if this path could be taken twice because if the directory would exist right now the command would error. Or is that the weird case in mkdir where -p is implied?

Sorry, I didn't mean to push that change. I got rid of this locally but forgot to force push.

You're right, -p also doesn't fail if the directory already exists. We could easily fix that case, but I guess the original question of the intention re. ancestor directories still stands.

SuperSandro2000 · 2021-11-26T02:43:28Z

pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh

        if [[ -z $flake ]]; then
            pathToConfig="$(nixBuild '<nixpkgs/nixos>' --no-out-link -A system "${extraBuildFlags[@]}")"
        else
            pathToConfig="$(nixFlakeBuild "$flake#$flakeAttr.config.system.build.toplevel" "${extraBuildFlags[@]}" "${lockFlags[@]}")"
        fi
        copyToTarget "$pathToConfig"
        targetHostCmd nix-env -p "$profile" --set "$pathToConfig"
-    elif [ "$action" = test -o "$action" = build -o "$action" = dry-build -o "$action" = dry-activate ]; then
+    elif [[ "$action" = test ]] || [[ "$action" = build ]] || [[ "$action" = dry-build ]] || [[ "$action" = dry-activate ]]; then


Suggested change

elif [[ "$action" = test ]] || [[ "$action" = build ]] || [[ "$action" = dry-build ]] || [[ "$action" = dry-activate ]]; then

elif [[ "$action" = test || "$action" = build || "$action" = dry-build || "$action" = dry-activate ]]; then

I think we should at least remove the optional ]] [[ in between. It makes the code shorter and for me personally way easier to understand. I would also be fine with keeping the -o to reduce the diff.

Artturin · 2021-11-26T20:53:22Z

@ofborg eval

Artturin

tested by checking out my system revision and applying this as a patch
git apply - <<< $(curl https://patch-diff.githubusercontent.com/raw/NixOS/nixpkgs/pull/147449.patch)

and rebuilding nix build ".#nixos-rebuild" && sudo ./result/bin/nixos-rebuild switch -I nixpkgs=$(pwd)

Artturin · 2021-11-28T19:58:11Z

doesn't need to target staging

The script already uses non-POSIX features like `[[` and `local`, so I assume it's safe to assume it shouldn't be treated as POSIX `sh`.

As per <https://github.com/koalaman/shellcheck/wiki/SC2166>.

l0b0 · 2021-11-29T03:17:30Z

Is the auto-request for reviews intentional or caused by the momentary massive diff between this branch and the target branch when switching from NixOS:staging to NixOS:master? If so, I'm sorry. And is it even possible to change the target branch without causing a notification storm? I tried changing the target branch a couple of seconds after pushing the rebased branch, and I don't expect there's any way to atomically change the target branch while pushing.

Artturin · 2021-11-29T03:19:37Z

reverse master and staging in this to go from staging to master

# finding the common merge base will avoid pinging all CODEOWNERs
common=$(git merge-base origin/master origin/staging)
git rebase --onto=$common HEAD~1 # or however many commits you want to pull
git push --force

then change the base branch in the github PR from master -> staging

instructions made by jonringer

l0b0 · 2021-11-29T21:03:20Z

[…] instructions made by jonringer

Is this somewhere front and centre in the contribution docs? Seems really important.

github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Nov 26, 2021

SuperSandro2000 reviewed Nov 26, 2021

View reviewed changes

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Nov 26, 2021

l0b0 force-pushed the fix-nixos-rebuild-lints branch 2 times, most recently from 6bdb8c3 to d2c5732 Compare November 26, 2021 02:58

ofborg bot added the ofborg-internal-error Ofborg encountered an error label Nov 26, 2021

l0b0 requested a review from SuperSandro2000 November 26, 2021 04:32

Artturin approved these changes Nov 28, 2021

View reviewed changes

l0b0 added 3 commits November 29, 2021 10:56

nixos-rebuild: Tell ShellCheck to treat script as Bash

dd56740

The script already uses non-POSIX features like `[[` and `local`, so I assume it's safe to assume it shouldn't be treated as POSIX `sh`.

nixos-rebuild: Use well-defined "and" and "or" syntax

4c5ac5a

As per <https://github.com/koalaman/shellcheck/wiki/SC2166>.

nixos-rebuild: Quote variables

311bfc8

l0b0 force-pushed the fix-nixos-rebuild-lints branch from d2c5732 to 311bfc8 Compare November 28, 2021 21:56

l0b0 requested review from aanderse, cdepillabout, etu, expipiplus1, FRidh, globin, jonringer, jtojnar, Ma27, maralorn, mmahut, RaghavSood, sternenseemann and talyz as code owners November 28, 2021 21:56

l0b0 changed the base branch from staging to master November 28, 2021 21:56

Artturin removed request for talyz, cdepillabout, mmahut, etu, jtojnar, expipiplus1, RaghavSood, globin, maralorn, FRidh, sternenseemann, Ma27, SuperSandro2000, jonringer and aanderse November 29, 2021 03:21

Artturin merged commit 82b9b42 into NixOS:master Nov 29, 2021

cole-h removed the ofborg-internal-error Ofborg encountered an error label Nov 29, 2021

l0b0 deleted the fix-nixos-rebuild-lints branch November 29, 2021 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some nixos-rebuild lints #147449

Fix some nixos-rebuild lints #147449

l0b0 commented Nov 26, 2021 •

edited

Loading

SuperSandro2000 left a comment

SuperSandro2000 Nov 26, 2021

l0b0 Nov 26, 2021

SuperSandro2000 Nov 26, 2021

Artturin commented Nov 26, 2021

Artturin left a comment

Artturin commented Nov 28, 2021

l0b0 commented Nov 29, 2021

Artturin commented Nov 29, 2021 •

edited

Loading

l0b0 commented Nov 29, 2021 •

edited

Loading

	elif [[ "$action" = test ]] \|\| [[ "$action" = build ]] \|\| [[ "$action" = dry-build ]] \|\| [[ "$action" = dry-activate ]]; then
	elif [[ "$action" = test \|\| "$action" = build \|\| "$action" = dry-build \|\| "$action" = dry-activate ]]; then

Fix some nixos-rebuild lints #147449

Fix some nixos-rebuild lints #147449

Conversation

l0b0 commented Nov 26, 2021 • edited Loading

Motivation for this change

Open questions

Things done

SuperSandro2000 left a comment

Choose a reason for hiding this comment

SuperSandro2000 Nov 26, 2021

Choose a reason for hiding this comment

l0b0 Nov 26, 2021

Choose a reason for hiding this comment

SuperSandro2000 Nov 26, 2021

Choose a reason for hiding this comment

Artturin commented Nov 26, 2021

Artturin left a comment

Choose a reason for hiding this comment

Artturin commented Nov 28, 2021

l0b0 commented Nov 29, 2021

Artturin commented Nov 29, 2021 • edited Loading

l0b0 commented Nov 29, 2021 • edited Loading

l0b0 commented Nov 26, 2021 •

edited

Loading

Artturin commented Nov 29, 2021 •

edited

Loading

l0b0 commented Nov 29, 2021 •

edited

Loading