Re: [PATCH v2 07/18] build: introduce rust

> > 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=

> > +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.

> > +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