From: Ezekiel Newren <ezekielnewren@gmail.com>
Date: Fri, 19 Sep 2025 20:11:01 GMT
Subject: Re: [PATCH v2 07/18] build: introduce rust
Message-ID: <CAH=ZcbBBkk2B3PxKf54MRnAmURMK8W7ofFZBRS=ZzkuDNWsY9w@mail.gmail.com>
In-Reply-To: <CAPig+cS_=YTBaCjn8-Th3yn3_k+a8_pMOmdv_Dq4S0tfp8BRCg@mail.gmail.com>

On Wed, Sep 17, 2025 at 2:26 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> Is ".idea" directory detritus from your chosen editor? If so, it
> probably ought not be added to this list since we don't otherwise
> ignore detritus from foreign tools such as that.

Yes. I use the Jetbrains IDE's CLion and RustRover for C and Rust
respectively. Jetbrains has an IDE for MANY languages and all of them
use .idea/ as the folder for IDE specific configuration. I'm fine with
keeping it out of .gitignore, but I wanted to know what the community
thought. Which I should have made it more clear that this patch series
was more of an RFC than proposed code to be merged.

> > diff --git a/Makefile b/Makefile
> > @@ -916,6 +924,51 @@ TEST_SHELL_PATH = $(SHELL_PATH)
> > +RUST_CRATES :=
> > +.PHONY: compile_rust rustclean
>
> This is an extremely tiny nit, but multi-word target names in the Git
> Makefile are almost universally spelled with a hyphen rather than an
> underscore, so "compile-rust" would be more consistent and
> appropriate.
>
> > +$(GIT_BUILD_DIR)/lib%.a:
> > +       ./build_rust.sh $(GIT_BUILD_DIR) $(RUST_BUILD_MODE) $*
>
> I wonder if this should follow the convention of most (but not quite
> all) other local shell script invocations in the Makefile of
> referencing SHELL_PATH; for instance:
>
>     $(QUIET_GEN)$(SHELL_PATH) ./build_rust.sh ...etc...
>
> with or without the $(QUIET_GEN) bit.
>
> > diff --git a/build_rust.sh b/build_rust.sh
> > @@ -0,0 +1,60 @@
> > +if [ "$dir_git_root" = "" ]; then
> > +  echo "did not specify the directory for the root of git"
> > +  exit 1
> > +fi
>
> Let's follow project style conventions for shell scripts:
>
>     if test "$dir_git_root" = ""
>     then
>         ...
>     fi
>
> or even:
>
>     if test -z "$dir_git_root"
>     then
>         ...
>     fi
>
> Same comment applies to the rest of the script.
>
> Also, should the above error message be sent to stderr rather than
> stdout? (Genuine question since I'm not sure if other scripts are
> necessarily consistent in that regard.)
>
> > +if [ "$rust_build_profile" = "release" ]; then
> > +  rust_args="--release"
> > +  export RUSTFLAGS=''
> > +elif [ "$rust_build_profile" = "debug" ]; then
> > +  rust_args=""
> > +  export RUSTFLAGS='-C debuginfo=2 -C opt-level=1 -C force-frame-pointers=yes'
> > +else
> > +  echo "illegal rust_build_profile value $rust_build_profile"
> > +  exit 1
> > +fi
>
> For maximum portability, we don't use the:
>
>     export FOO=bar
>
> idiom in this project. Instead, do this:
>
>     FOO=bar
>     export FOO
>
> The same comment applies to the remainder of the patch, as well.
>
> Also, the empty-string assignments to RUSTFLAGS and rust_args above
> inconsistently use a mix of single and double quotes. However, we also
> usually just assign nothingness in such cases rather than an empty
> string:
>
>     RUSTFLAGS=
>     export RUSTFLAGS
>     ...
>     rust_args=

I'll try to keep these in mind for the future.

> > +if rustup show active-toolchain | grep windows-msvc; then
> > +  libfile="${crate}.lib"
> > +  PATH="$(echo $PATH | tr ':' '\n' | grep -Ev "^(/mingw64/bin|/usr/bin)$" | paste -sd: -):/mingw64/bin:/usr/bin"
> > +fi
>
> Please add either an in-code comment or a sentence/paragraph to the
> commit message explaining why this PATH munging is needed.

I will amend the commit with something like:
On windows when building with msvc using shell scripts it looks for
link in /mingw64/bin|/usr/bin when it actually needs to look somewhere
else for the msvc linker program. Since removing these from PATH would
break everything else in the shell; move them to be at the end of
PATH.

> > +cd $dir_rust && cargo clean && pwd && USE_LINKING="false" cargo build -p $crate $rust_args; cd $dir_git_root
>
> When formatted like this, it is very easy to overlook the fact that
> the final `cd` is (correctly) not &&-chained with the preceding
> commands. Reformatting it like this would help:
>
>     cd $dir_rust &&
>         cargo clean &&
>         pwd &&
>         USE_LINKING="false" cargo build -p $crate $rust_args
>     cd $dir_git_root
>
> Alternatively, use a subshell and drop the final `cd`:
>
>     (
>         cd $dir_rust &&
>         cargo clean &&
>         pwd &&
>         USE_LINKING="false" cargo build -p $crate $rust_args
>     )
>
> By the way, is the `pwd` command needed? What is its purpose?
>
> > +rm $dst 2>/dev/null
>
> I think we typically use `rm -f` without redirect to /dev/null to do
> the "right thing".
>
> > +echo mv $src $dst
> > +mv $src $dst

pwd was for debugging and I forgot to take it out. I'll clean up my script.