Add setup-rust script and git hook

[kl]

The setup-rust helper script installs the default targets and components that each platform needs with rustup.

The setup-rust-post-checkout script is a git hook (needs to be copied to .git/hooks/post-checkout, which can be done manually or by running setup-rust install-hook) that detects when the rust-toolchain.toml file has changed and installs the default targets/components (and/or extra targets/components that the user has specified in the hook script).

So say we have a new desktop developer and they are setting up their environment. They could run the following:
scripts/setup-rust desktop && scripts/setup-rust install-hook desktop after which things Should Just Work.

---

This change is 

[linear]

DROID-1786 Add script and git hook to automate installation of Rust toolchain targets and components

[albin-mullvad]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions

---

scripts/setup-rust line 26 at r1 (raw file):

  log "Options:"
  log "--print-targets        Only print the targets that would be added"
  log "--print-components     Only print the components that would be added"

Do we need to distinguish these? Imo it would simlify things to just use a --dry-run working like this: https://unix.stackexchange.com/a/433806

Code quote:

  log "--print-targets        Only print the targets that would be added"
  log "--print-components     Only print the components that would be added"

---

scripts/setup-rust line 30 at r1 (raw file):

function setup_android {
  local targets="x86_64-linux-android i686-linux-android aarch64-linux-android armv7-linux-androideabi"

It would be nice to define the target/component list as variables at the top of the script, e.g. ANDROID_COMPONENTS="rust-analyzer" so that it's easy to get an overview of the platform components/targets without digging into the functions.

---

scripts/setup-rust line 80 at r1 (raw file):

}

if [ $# -eq 0 ]; then

I believe we usually put the input/argument parsing at the top in a main function structure that we call from the bottom of the script using a main "$@" call. I'm referring to the scripts where attempt a function structure. Apart from keeping this scripts consistent another benefit of doing so is that it's easy/quick to get an overview of the main entry points. Another benefit is to keep the argument parsing and usage print next to each other in order to help visually ensure they are up-to-date with each other. What do think about making such a change? I'll gladly discuss/explain further it outside of the review!

Example:

mullvadvpn-app/scripts/localization

Line 11 in ab3bd2b

function main {

[albin-mullvad]

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @kl)

---

scripts/setup-rust-post-checkout line 17 at r1 (raw file):

EXTRA_TARGETS=""
# Any extra components outside of the ones defined in `setup-rust` that should be installed.
EXTRA_COMPONENTS=""

Are these not more convenient to define in your shell environment to avoid hook modifications? 🤔 That would also allow for automatic hook updates, where the hook updates itself (or is symlinked).

Code quote:

# Set to "android", "desktop", or "ios" or leave empty.
# If empty this hook will not call the `setup-rust` script and only install the EXTRA_TARGETS
# and EXTRA_COMPONENTS.
PLATFORM=""
# Any extra targets outside of the ones defined in `setup-rust` that should be installed.
EXTRA_TARGETS=""
# Any extra components outside of the ones defined in `setup-rust` that should be installed.
EXTRA_COMPONENTS=""

---

scripts/setup-rust-post-checkout line 27 at r1 (raw file):

CHANGED=$(git diff "$1" "$2" --stat -- rust-toolchain.toml | wc -l)
if [ "$CHANGED" -gt 0 ];
then

This hook includes more complexity than I expected, such as all the wc based calculations. Is there any way we could simplify it?

[kl]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)

---

scripts/setup-rust line 26 at r1 (raw file):

Previously, albin-mullvad wrote…

Do we need to distinguish these? Imo it would simlify things to just use a --dry-run working like this: https://unix.stackexchange.com/a/433806

They are used by the hook to get the number of default targets/components specified for the given platform. This number is used to check if the already installed target/component count is less than the needed target/component count, and only if it is less we call rustup target add. This is done so that we only rustup target add when necessary, and not every time for example when switching between two branches that have different versions of Rust specified in rust-toolchain.toml

---

scripts/setup-rust line 30 at r1 (raw file):

Previously, albin-mullvad wrote…

It would be nice to define the target/component list as variables at the top of the script, e.g. ANDROID_COMPONENTS="rust-analyzer" so that it's easy to get an overview of the platform components/targets without digging into the functions.

Done

---

scripts/setup-rust line 80 at r1 (raw file):

Previously, albin-mullvad wrote…

I believe we usually put the input/argument parsing at the top in a main function structure that we call from the bottom of the script using a main "$@" call. I'm referring to the scripts where attempt a function structure. Apart from keeping this scripts consistent another benefit of doing so is that it's easy/quick to get an overview of the main entry points. Another benefit is to keep the argument parsing and usage print next to each other in order to help visually ensure they are up-to-date with each other. What do think about making such a change? I'll gladly discuss/explain further it outside of the review!

Example:

mullvadvpn-app/scripts/localization

Line 11 in ab3bd2b

function main {

Done

---

scripts/setup-rust-post-checkout line 17 at r1 (raw file):

Previously, albin-mullvad wrote…

Are these not more convenient to define in your shell environment to avoid hook modifications? 🤔 That would also allow for automatic hook updates, where the hook updates itself (or is symlinked).

Done

---

scripts/setup-rust-post-checkout line 27 at r1 (raw file):

Previously, albin-mullvad wrote…

This hook includes more complexity than I expected, such as all the wc based calculations. Is there any way we could simplify it?

The calculations are needed in order to prevent calling rustup target add more than strictly necessary (e.g. in cases where we already have all the targets/components installed but are switching between two branches that have different toolchain versions)

[albin-mullvad]

Reviewed 1 of 2 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions

---

scripts/setup-rust line 26 at r1 (raw file):

Previously, kl (Kalle Lindström) wrote…

They are used by the hook to get the number of default targets/components specified for the given platform. This number is used to check if the already installed target/component count is less than the needed target/component count, and only if it is less we call rustup target add. This is done so that we only rustup target add when necessary, and not every time for example when switching between two branches that have different versions of Rust specified in rust-toolchain.toml

Aha, I missed that part 👍

---

scripts/setup-rust-post-checkout line 17 at r1 (raw file):

Previously, kl (Kalle Lindström) wrote…

Done

Nice!

---

-- commits line 9 at r1:
Not sure what the best practice is, but maybe we should recommend symlinking in order to easily keep it up-to-date in case it changes over time? 🤔 Not sure how that would work on Windows though. This comment also affect the install-hook function.

Code quote:

  The setup-rust-post-checkout script is a git hook (needs to be
  copied to .git/hooks/post-checkout, which can be done manually or
  by running `setup-rust install-hook`) that detects when

[faern]

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @kl)

---

scripts/setup-rust-post-checkout line 27 at r1 (raw file):

Previously, kl (Kalle Lindström) wrote…

The calculations are needed in order to prevent calling rustup target add more than strictly necessary (e.g. in cases where we already have all the targets/components installed but are switching between two branches that have different toolchain versions)

You probably don't want to use the git porcelain commands for this, but rather the plumbing tools. You can do the above without wc with:

git diff-tree --exit-code $previous_commit..$new_commit -- rust-toolchain.toml

This exits with exit code 1 if the file has a diff between the commits. Exit code 0 if it has no change.

---

scripts/setup-rust-post-checkout line 10 at r2 (raw file):

set -eu

function main() {

Nitpick. But our coding guidelines says to not use parenthesis on functions in bash. https://github.com/mullvad/coding-guidelines/blob/main/bash.md#functions

[faern]

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @kl)

---

scripts/setup-rust-post-checkout line 27 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

You probably don't want to use the git porcelain commands for this, but rather the plumbing tools. You can do the above without wc with:

git diff-tree --exit-code $previous_commit..$new_commit -- rust-toolchain.toml

This exits with exit code 1 if the file has a diff between the commits. Exit code 0 if it has no change.

Also slap a --quiet on it to stay silent. Then you can work with exit codes only and not look at any text output. And --quiet implies --exit-code. So only --quiet is actually needed.

Plumbing tools are nice for programmatic use since they are more or less guaranteed to never change behavior.

[faern]

Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @kl)

---

scripts/setup-rust-post-checkout line 30 at r2 (raw file):

    if [ "$changed" -gt 0 ];
    then

Style nitpicking. But I suggest you study our bash coding guidelines: https://github.com/mullvad/coding-guidelines/blob/main/bash.md#formatting

[faern]

Reviewable status: 1 of 2 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @kl)

---

scripts/setup-rust-post-checkout line 50 at r2 (raw file):

        fi

        if [ "$installed_targets_count" -lt "$required_targets_count" ]; then

Do we really need to count targets and only install things when we have fewer targets? Adding a target that already exists seems to be an extremely cheap operation already. And we will only trigger these things when rust-toolchain.toml changes anyway, which will not be very common. So I think we can affort calling rustup target add ... a few more times and reduce complexity of this hook by a lot?

It's also not clear to me how the count of targets is a stable check. You can have lots of targets, but just not the correct ones, and it would prevent this hook from installing the correct targets.

[faern]

Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @albin-mullvad and @kl)

---

scripts/setup-rust line 65 at r2 (raw file):

function setup {
  case "$4" in

You are in general using a lot of local variable definitions, which is great! I think this function could use some as well. Knowing what $1, $2 etc are by name would make it much more readable.

We frequently start our bash functions by assigning all arguments to local variables, to give them reasonable names.

[kl]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @faern)

---

scripts/setup-rust line 65 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

You are in general using a lot of local variable definitions, which is great! I think this function could use some as well. Knowing what $1, $2 etc are by name would make it much more readable.

We frequently start our bash functions by assigning all arguments to local variables, to give them reasonable names.

Done

---

scripts/setup-rust-post-checkout line 27 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Also slap a --quiet on it to stay silent. Then you can work with exit codes only and not look at any text output. And --quiet implies --exit-code. So only --quiet is actually needed.

Plumbing tools are nice for programmatic use since they are more or less guaranteed to never change behavior.

Done

---

scripts/setup-rust-post-checkout line 30 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Style nitpicking. But I suggest you study our bash coding guidelines: https://github.com/mullvad/coding-guidelines/blob/main/bash.md#formatting

Done.

---

scripts/setup-rust-post-checkout line 50 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Do we really need to count targets and only install things when we have fewer targets? Adding a target that already exists seems to be an extremely cheap operation already. And we will only trigger these things when rust-toolchain.toml changes anyway, which will not be very common. So I think we can affort calling rustup target add ... a few more times and reduce complexity of this hook by a lot?

It's also not clear to me how the count of targets is a stable check. You can have lots of targets, but just not the correct ones, and it would prevent this hook from installing the correct targets.

Done.

[kl]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @faern)

---

scripts/setup-rust-post-checkout line 10 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Nitpick. But our coding guidelines says to not use parenthesis on functions in bash. https://github.com/mullvad/coding-guidelines/blob/main/bash.md#functions

Done.

[faern]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions

---

scripts/setup-rust line 4 at r3 (raw file):

#
# Installs the default toolchains and components for different platforms.
# To use this script rustup must be installed first.

I think we should mention this script/the hook in some documentation. Currently it's not very discoverable. Probably BuildInstructions.md in the root (but that is desktop specific)?

---

scripts/setup-rust line 66 at r3 (raw file):

    local PLATFORM=$1
    local TARGETS=$2
    local COMPONENTS=$3

Very nitpicky, and we are a bit inconsistent in the repo overall, but these should ideally be lowercase.

---

scripts/setup-rust line 75 at r3 (raw file):

    rustup target add $2
    # shellcheck disable=SC2086
    rustup component add $3

You have already assigned the arguments to local variables, use them by that name instead of argument number.

[faern]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kl)

---

android/BuildInstructions.md line 154 at r4 (raw file):

- Install Android targets
  ```bash
  ./scripts/setup-rust android

This is awesome! 🤩 Great work. Nice to replace the hardcoded list with a script that can be updated without touching the docs.

[faern]

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

[kl]

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

---

scripts/setup-rust line 4 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think we should mention this script/the hook in some documentation. Currently it's not very discoverable. Probably BuildInstructions.md in the root (but that is desktop specific)?

Done.

---

scripts/setup-rust line 66 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Very nitpicky, and we are a bit inconsistent in the repo overall, but these should ideally be lowercase.

Done.

---

scripts/setup-rust line 75 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

You have already assigned the arguments to local variables, use them by that name instead of argument number.

Done.