QA: The used scripts in nixpkgs are not sane

[Kreyren]

Describe the bug

Scripts used in shellHook, buildCommand, etc.. are not sane and can break runtime:

[kreyren@leonid:~]$ nix-shell -p shellcheck
...
[nix-shell:~]$ cat qa-NIXOS.sh
#!/bin/sh
# Taken from https://github.com/Kreyren/nixpkgs/blob/master/nixos/modules/installer/sd-card/sd-image.nix#L181 with nix bits removed
mkdir -p $out/nix-support $out/sd-image
export img=$out/sd-image/${config.sdImage.imageName}
echo "NIX_BIT" > $out/nix-support/system
if test -n "$compressImage"; then
  echo "file sd-image $img.zst" >> $out/nix-support/hydra-build-products
else
  echo "file sd-image $img" >> $out/nix-support/hydra-build-products
fi
echo "Decompressing rootfs image"
zstd -d --no-progress "NIX_BIT" -o ./root-fs.img
# Gap in front of the first partition, in MiB
gap=NIX_BIT
# Create the image file sized to fit /boot/firmware and /, plus slack for the gap.
rootSizeBlocks=$(du -B 512 --apparent-size ./root-fs.img | awk '{ print $1 }')
firmwareSizeBlocks=$((NIX_BIT * 1024 * 1024 / 512))
imageSize=$((rootSizeBlocks * 512 + firmwareSizeBlocks * 512 + gap * 1024 * 1024))
truncate -s $imageSize $img
# type=b is 'W95 FAT32', type=83 is 'Linux'.
# The "bootable" partition is where u-boot will look file for the bootloader
# information (dtbs, extlinux.conf file).
sfdisk $img <<EOF
    label: dos
    label-id: ${config.sdImage.firmwarePartitionID}
    start=''${gap}M, size=$firmwareSizeBlocks, type=b
    start=$((gap + NIX_BIT))M, type=83, bootable
EOF
# Copy the rootfs into the SD image
eval $(partx $img -o START,SECTORS --nr 2 --pairs)
dd conv=notrunc if=./root-fs.img of=$img seek=$START count=$SECTORS
# Create a FAT32 /boot/firmware partition of suitable size into firmware_part.img
eval $(partx $img -o START,SECTORS --nr 1 --pairs)
truncate -s $((SECTORS * 512)) firmware_part.img
faketime "1970-01-01 00:00:00" mkfs.vfat -i NIX_BIT -n NIX_BIT firmware_part.img
# Populate the files intended for /boot/firmware
mkdir firmware
NIX_BIT
# Copy the populated /boot/firmware into the SD image
(cd firmware; mcopy -psvm -i ../firmware_part.img ./* ::)
# Verify the FAT partition before copying it.
fsck.vfat -vn firmware_part.img
dd conv=notrunc if=firmware_part.img of=$img seek=$START count=$SECTORS
${config.sdImage.postBuildCommands}
if test -n "$compressImage"; then
    zstd -T$NIX_BUILD_CORES --rm $img
fi
[nix-shell:~]$ shellcheck qa-NIXOS.sh 

In qa-NIXOS.sh line 3:
mkdir -p $out/nix-support $out/sd-image
         ^--^ SC2154: out is referenced but not assigned.
         ^--^ SC2086: Double quote to prevent globbing and word splitting.
                          ^--^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
mkdir -p "$out"/nix-support "$out"/sd-image


...


In qa-NIXOS.sh line 5:
echo "NIX_BIT" > $out/nix-support/system
                 ^--^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
echo "NIX_BIT" > "$out"/nix-support/system


In qa-NIXOS.sh line 6:
if test -n "$compressImage"; then
            ^------------^ SC2154: compressImage is referenced but not assigned.


In qa-NIXOS.sh line 7:
  echo "file sd-image $img.zst" >> $out/nix-support/hydra-build-products
                                   ^--^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
  echo "file sd-image $img.zst" >> "$out"/nix-support/hydra-build-products


In qa-NIXOS.sh line 9:
  echo "file sd-image $img" >> $out/nix-support/hydra-build-products
                               ^--^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
  echo "file sd-image $img" >> "$out"/nix-support/hydra-build-products


In qa-NIXOS.sh line 19:
truncate -s $imageSize $img
                       ^--^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
truncate -s $imageSize "$img"


In qa-NIXOS.sh line 23:
sfdisk $img <<EOF
       ^--^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
sfdisk "$img" <<EOF


In qa-NIXOS.sh line 30:
eval $(partx $img -o START,SECTORS --nr 2 --pairs)
     ^-- SC2046: Quote this to prevent word splitting.
             ^--^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
eval $(partx "$img" -o START,SECTORS --nr 2 --pairs)


In qa-NIXOS.sh line 31:
dd conv=notrunc if=./root-fs.img of=$img seek=$START count=$SECTORS
                                    ^--^ SC2086: Double quote to prevent globbing and word splitting.
                                              ^----^ SC2086: Double quote to prevent globbing and word splitting.
                                                           ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
dd conv=notrunc if=./root-fs.img of="$img" seek="$START" count="$SECTORS"


In qa-NIXOS.sh line 33:
eval $(partx $img -o START,SECTORS --nr 1 --pairs)
     ^-- SC2046: Quote this to prevent word splitting.
             ^--^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
eval $(partx "$img" -o START,SECTORS --nr 1 --pairs)


In qa-NIXOS.sh line 40:
(cd firmware; mcopy -psvm -i ../firmware_part.img ./* ::)
 ^---------^ SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

Did you mean: 
(cd firmware || exit; mcopy -psvm -i ../firmware_part.img ./* ::)


In qa-NIXOS.sh line 43:
dd conv=notrunc if=firmware_part.img of=$img seek=$START count=$SECTORS
                                        ^--^ SC2086: Double quote to prevent globbing and word splitting.
                                                  ^----^ SC2086: Double quote to prevent globbing and word splitting.
                                                               ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
dd conv=notrunc if=firmware_part.img of="$img" seek="$START" count="$SECTORS"


In qa-NIXOS.sh line 46:
    zstd -T$NIX_BUILD_CORES --rm $img
           ^--------------^ SC2086: Double quote to prevent globbing and word splitting.
                                 ^--^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
    zstd -T"$NIX_BUILD_CORES" --rm "$img"

For more information:
  https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt...
  https://www.shellcheck.net/wiki/SC2154 -- compressImage is referenced but n...
  https://www.shellcheck.net/wiki/SC2164 -- Use 'cd ... || exit' or 'cd ... |...

Proposal

Integrate a CI that checks the scripts against shellcheck

[aanderse]

Wasn't it @happysalada who was doing some really awesome work in this area recently?

[happysalada]

@Kreyren thank you for submitting this!
There are many trivial fixes that can probably be applied quickly.
Some less trivial fixes require quite a large rebuild job in hydra.
If the fix is trivial, a PR can probably be approved and merged in a day
If the fix is a little more complicated, I tried to group them into batches and ask the hydra team for help.
Some of that stuff takes some time, so you have to be willing to fight the long fight.
If you have the heart for any PRs, my advice is to make them relatively small so they can get merged quickly.
You can always tag me on a PR and I'll help you get it merged.

Last but not least. The shell stuff is doing some funny dynamic things with variables and shellcheck provides false positives, so you'll have to filter a bit.

The main reason why people haven't implemented some of those changes is that it can break things in unexpected ways.
For example I tried having a PR where the split on spaces for flags was explicit rather than implict. It broke some packages that had newlines in the flags array.

Anyways, happy to help any efforts in improving those scripts!

[wmertens]

$out is defined but shellcheck doesn't know about it. It also doesn't have spaces so doesn't need quoting.

Same for compressImage I suppose?

So the majority of these messages are bogus.

cd || exit 1 wouldn't hurt I guess

[Et7f3]

/nix/store can be changed so $out can have space ? One use case I see is someone with /nix/ pointing to ssd but he wants to use hist hdd and unluckily it has space/stange chars. This person could fix his mount point and thus benefit from cachix but I don't know how the installer choose disk to use.

Also when someone is modifying a package if all variable are quoted he will most likely quote his new variable and we get better software. And to avoid rebuild I would suggest if you change a script then fix all issue in this script (the quoting included) but don't force a rebuild just for quoting $out

[7c6f434c]

/nix/store can be changed so $out can have space ?

No, in some places of stdenv there are non-trivial design issues that need to be solved for that to work, just quoting $out everywhere is not enough

[nixos-discourse]

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

https://discourse.nixos.org/t/how-is-bash-expanded-in-nixpkgs/23560/1