From: Eric Sunshine <sunshine@sunshineco.com>
Date: Wed, 17 Sep 2025 08:01:45 GMT
Subject: Re: [PATCH v2 09/18] github workflows: install rust
Message-ID: <CAPig+cT-2-s-TcZ-2TQujLkn8Eh-EmYa9QWHWpw3iczDuX5mUQ@mail.gmail.com>
In-Reply-To: <fcdfc55fb7d7da7d65405486f5eec10e5892a028.1758071798.git.gitgitgadget@gmail.com>

On Tue, Sep 16, 2025 at 9:17 PM Ezekiel Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Prefer using actions-rs/toolchain@v1 where possible to install rustup,
> but for docker targets use a script to install rustup. Consolidate the
> Rust toolchain definitions in main.yaml. Use install-rust-toolchain.sh
> to ensure the correct toolchain is used. Five overrides are used in
> main.yaml:
>
>   * On Windows: Rust didn't resolve the bcrypt library on Windows
>     correctly until version 1.78.0. Also since rustup mis-identifies
>     the Rust toolchain, the Rust target triple must be set to
>     x86_64-pc-windows-gnu for make (win build), and
>     x86_64-pc-windows-msvc for meson (win+Meson build).
>   * MSVC builds: Rearrange PATH to look in /mingw64/bin and /usr/bin
>     last.

Please add an explanation as to why it is necessary to rearrange PATH.
I saw in patch [7/18] that your "build_rust.sh" does the same but the
reason is never spelled out (and it's still a mystery to me).

Also, this patch, [9/18], doesn't seem to touch PATH in the way
described here (unless I somehow overlooked it).

>   * On musl: libc differences, such as ftruncate64 vs ftruncate, were
>     not accounted for until Rust version 1.72.0. No older version of
>     Rust will work on musl for our needs.
>   * In a 32-bit docker container running on a 64-bit host, we need to
>     override the Rust target triple. This is because rustup asks the
>     kernel for the bitness of the system and it says 64, even though
>     the container is 32-bit. This also allows us to remove the
>     BITNESS environment variable in ci/lib.sh.
>
> The logic for selecting library names was initially provided in a patch
> from Johannes, but was reworked and squashed into this commit.
>
> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
> ---
> diff --git a/ci/install-rust-toolchain.sh b/ci/install-rust-toolchain.sh
> @@ -0,0 +1,30 @@
> +#!/bin/sh
> +
> +if [ "$CARGO_HOME" = "" ]; then
> +  echo >&2 "::error:: CARGO_HOME is not set"
> +  exit 2
> +fi

Let's follow project coding guidelines for shell scripts:

    if test "$CARGO_HOME" = ""
    then
        ..
    fi

or even:

    if test -z "$CARGO_HOME"
    then
        ...
    fi

Same comment applies to the remainder of this script and other scripts
in this patch.

> diff --git a/ci/install-rustup.sh b/ci/install-rustup.sh
> @@ -0,0 +1,25 @@
> +if [ ! -f $CARGO_HOME/env ]; then
> +  echo "PATH=$CARGO_HOME/bin:\$PATH" > $CARGO_HOME/env
> +fi

Style: drop space after '>' operator

> diff --git a/ci/lib.sh b/ci/lib.sh
> @@ -1,5 +1,6 @@
>  # Library of functions shared by all CI scripts
>
> +
>  if test true = "$GITHUB_ACTIONS"

Do we need the extra blank line introduced above the `if`?