Re: [PATCH v4 5/5] Makefile: add option to build and test libgit-rs and libgit-rs-sys

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 9, 2024, 00:01 UTC
Message-ID: <xmqq7caixhz0.fsf@gitster.g>
In-Reply-To: <2ed503216f8e14d7b516c488caf3c76ffcb15697.1728429158.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 16 quoted lines

> From: Calvin Wan <calvinwan@google.com>
>
> Add libgitrs, libgitrs-sys, libgitrs-test, and libgitrs-sys-test targets
> to their respective Makefiles so they can be built and tested without
> having to run cargo build/test.
>
> Add environment variable, INCLUDE_LIBGIT_RS, that when set,
> automatically builds and tests libgit-rs and libgit-rs-sys when `make
> all` is ran.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  Makefile   | 16 ++++++++++++++++
>  t/Makefile | 16 ++++++++++++++++
>  2 files changed, 32 insertions(+)

Interesting.

I tried

	$ make INCLUDE_LIBGIT_RS=YesPlease

which did not fail, and then did the same

	$ make INCLUDE_LIBGIT_RS=YesPlease

and was surprised to see that not only the libgit-sys part but everything was recompiled and rebuilt.

Show 14 quoted lines

> diff --git a/Makefile b/Makefile
> ...
> +.PHONY: libgitrs
> +libgitrs:
> +	$(QUIET)(\
> +		cd contrib/libgit-rs && \
> +		cargo build \
> +	)
> +ifdef INCLUDE_LIBGIT_RS
> +all:: libgitrs
> +endif
> +
>  contrib/libgit-rs/libgit-sys/partial_symbol_export.o: contrib/libgit-rs/libgit-sys/public_symbol_export.o libgit.a reftable/libreftable.a xdiff/lib.a
>  	$(LD) -r $^ -o $@

I can see libgitrs is a phony target designed to run every time it gets triggered, and I would imagine "cargo build" itself would avoid repeating unnecessary work, but I do not see this patch screwing up with the dependencies for other object files.

Is it fair to say this is still a WIP? Showing a WIP to others and asking for help is OK, but it is fair to make sure that others know what is expected of them.

Thanks.