Fix shellcheck lints in bash scripts

[Serock3]

Cleanup task. Fix shellcheck lints of our various bash scripts.

Each commit is named after a shellcheck lint. For some of them I have added extra commit descriptions with motivations for how I modified the code. One way to verify the changes of this commit could be to look at output of the CI pipeline steps that run the modified scripts.

Here is a one-liner I used to run shellcheck on the entire repo: fd . -t f -e .sh -E dist-assets -x shellcheck --severity=warning --format=gcc {}. I didn't have time to get to all the lints, some less trivial ones remain. Running the command after the fixes in this branch produces:

In ./ci/ios/buildserver-build-ios.sh line 96:
        tags=( $(run_git tag | grep "$TAG_PATTERN_TO_BUILD") )
               ^-- SC2207 (warning): Prefer mapfile or read -a to split command output (or quote to avoid splitting).
                                     ^-------------------^ SC2128 (warning): Expanding an array without an index only gives the first element.

For more information:
  https://www.shellcheck.net/wiki/SC2128 -- Expanding an array without an ind...
  https://www.shellcheck.net/wiki/SC2207 -- Prefer mapfile or read -a to spli...

In ./wireguard/libwg/build-android.sh line 54:
    mkdir -m 777 -p "$(dirname "$STRIPPED_LIB_PATH")"
          ^-- SC2174 (warning): When used with -p, -m only applies to the deepest directory.

For more information:
  https://www.shellcheck.net/wiki/SC2174 -- When used with -p, -m only applie...

Fixes DES-588.

---

This change is 

[linear]

DES-588 Fix shellcheck lints in bash scripts

[MarkusPettersson98]

I've reviewed everything except the iOS-related scripts, and it
Left one minor comment about trailing whitespace 😊

Reviewed 19 of 25 files at r1, all commit messages.
Reviewable status: 19 of 25 files reviewed, 1 unresolved discussion (waiting on @Serock3)

---

android/scripts/generate-pngs.sh line 50 at r1 (raw file):

    local dpi_config="$2"
    local destination_image
    

Some extra whitespace snuck in here it seems 😊

[pinkisemils]

Reviewed 4 of 25 files at r1.
Reviewable status: 21 of 25 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)

---

ci/ios/upload-app.sh line 6 at r2 (raw file):

VM_UPLOAD_IPA_PATH="/Volumes/My Shared Files/build-output/MullvadVPN.ipa"
API_KEY_PATH="$HOME/ci/app-store-connect-key.json"
cd ci/ || exit

Would it not be better to do a set -eu above?

[pinkisemils]

Reviewable status: 21 of 25 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)

---

ios/build.sh line 96 at r2 (raw file):

    -configuration Release \
    -derivedDataPath "$DERIVED_DATA_DIR" \
    "$@"

The lack of quotes here is intentional - so that all arguments are passed as they are, instead as a single argument.

---

ios/build-rust-library.sh line 56 at r2 (raw file):

        "$HOME"/.cargo/bin/cargo build -p "$FFI_TARGET" --lib --target aarch64-apple-ios
      else
        "$HOME"/.cargo/bin/cargo build -p "$FFI_TARGET" --lib $RELFLAG --target aarch64-apple-ios-sim

I wonder why $RELFLAG gets to not be quoted?

[pinkisemils]

Since we're fixing all the lints, shall we add shellcheck as a GitHub Action's job?

Reviewable status: 21 of 25 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)

[Serock3]

Markus made a linear issue about that https://linear.app/mullvad/issue/DES-593/run-shellcheck-in-ci

Reviewable status: 21 of 25 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98 and @pinkisemils)

---

android/scripts/generate-pngs.sh line 50 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Some extra whitespace snuck in here it seems 😊

Done.

---

ci/ios/upload-app.sh line 6 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Would it not be better to do a set -eu above?

I like your solution, it's much cleaner. However, on their website https://www.shellcheck.net/wiki/SC2164 it says

"Having a set -e command anywhere in the script will disable this message, even though it won't necessarily prevent the issue."

I can't think of a reason why set -eu wouldn't work, so if you agree then I will change to it.

---

ios/build.sh line 96 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

The lack of quotes here is intentional - so that all arguments are passed as they are, instead as a single argument.

According to https://www.shellcheck.net/wiki/SC2068, quoting $@ still expands into multiple arguments, but prevents globbing and word splitting of individual elements. If you are really sure I will disable the lint for that case.

---

ios/build-rust-library.sh line 56 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I wonder why $RELFLAG gets to not be quoted?

No idea why. My best guess is that shellcheck just misses it for some reason. The webpage for the lint (https://www.shellcheck.net/wiki/SC2086) doesn't seem to mention any relevant exceptions, so it should trigger.

Unless you're against it, I will add quotes there too.

[pinkisemils]

Reviewable status: 21 of 25 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98)

---

env.sh line 1 at r2 (raw file):

#!/usr/bin/env bash

This particular script was left without a shebang to allow shells other than bash to build the app, but I guess we can also not care 🤷

[pinkisemils]

Reviewable status: 21 of 25 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)

---

ci/ios/upload-app.sh line 6 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I like your solution, it's much cleaner. However, on their website https://www.shellcheck.net/wiki/SC2164 it says

"Having a set -e command anywhere in the script will disable this message, even though it won't necessarily prevent the issue."

I can't think of a reason why set -eu wouldn't work, so if you agree then I will change to it.

I see no good reason either. Let's have set -eu instead.

[pinkisemils]

Reviewable status: 21 of 25 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)

---

ios/build.sh line 96 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

According to https://www.shellcheck.net/wiki/SC2068, quoting $@ still expands into multiple arguments, but prevents globbing and word splitting of individual elements. If you are really sure I will disable the lint for that case.

I was not aware that "$@" was treated differently - let's quote it then!

---

ios/build-rust-library.sh line 56 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

No idea why. My best guess is that shellcheck just misses it for some reason. The webpage for the lint (https://www.shellcheck.net/wiki/SC2086) doesn't seem to mention any relevant exceptions, so it should trigger.

Unless you're against it, I will add quotes there too.

Add them for completion sake.

[Serock3]

Reviewable status: 20 of 30 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98 and @pinkisemils)

---

ci/ios/upload-app.sh line 6 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I see no good reason either. Let's have set -eu instead.

Done.

---

ios/build.sh line 96 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I was not aware that "$@" was treated differently - let's quote it then!

Done.

---

ios/build-rust-library.sh line 56 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Add them for completion sake.

Done.

---

env.sh line 1 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

This particular script was left without a shebang to allow shells other than bash to build the app, but I guess we can also not care 🤷

I reverted the change

[MarkusPettersson98]

Reviewed 1 of 1 files at r2, 6 of 8 files at r3, all commit messages.
Reviewable status: 27 of 30 files reviewed, 3 unresolved discussions (waiting on @pinkisemils)

[MarkusPettersson98]

Reviewable status: 27 of 30 files reviewed, 3 unresolved discussions (waiting on @pinkisemils)

[pinkisemils]

Reviewed 1 of 25 files at r1, 3 of 8 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[pinkisemils]

Reviewable status: complete! all files reviewed, all discussions resolved

[MarkusPettersson98]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[MarkusPettersson98]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[MarkusPettersson98]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved