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

Commit Message

Josh Steadmon Oct. 8, 2024, 11:19 p.m. UTC

From: Calvin Wan <[e-mail]>

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 <[e-mail]>
Signed-off-by: Josh Steadmon <[e-mail]>
---
 Makefile   | 16 ++++++++++++++++
 t/Makefile | 16 ++++++++++++++++
 2 files changed, 32 insertions(+)

Comments

Junio C Hamano Oct. 8, 2024, 11:45 p.m. UTC | #1

Josh Steadmon <[e-mail]> writes:

> Add environment variable, INCLUDE_LIBGIT_RS, that when set,
> automatically builds and tests libgit-rs and libgit-rs-sys when `make
> all` is ran.

Is this unusual, or is it just like how other makefile macros like
say USE_NSEC (to cause the resulting Git to use subsecond mtimes)
are meant to be used to control the build?  IOW, shouldn't this be
documented near the top of the Makefile, e.g.

    diff --git i/Makefile w/Makefile
    index 41ad458aef..2b55fe9672 100644
    --- i/Makefile
    +++ w/Makefile
    @@ -392,6 +392,9 @@ include shared.mak
     # INSTALL_STRIP can be set to "-s" to strip binaries during installation,
     # if your $(INSTALL) command supports the option.
     #
    +# Define INCLUDE_LIBGIT_RS if you want your gostak to distim
    +# the doshes and ...
    +#
     # Define GENERATE_COMPILATION_DATABASE to "yes" to generate JSON compilation
     # database entries during compilation if your compiler supports it, using the
     # `-MJ` flag. The JSON entries will be placed in the `compile_commands/`

It might make sense to follow naming convention to call it NO_RUST
and flip its polarity.  Those who do not have or want libgit-rs and
friends can say NO_RUST but otherwise it gets built by default.  It
would give you a wider developer population coverage.

Thanks.

Junio C Hamano Oct. 9, 2024, 12:01 a.m. UTC | #2

Josh Steadmon <[e-mail]> writes:

> From: Calvin Wan <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Josh Steadmon <[e-mail]>
> ---
>  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.

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

Junio C Hamano Oct. 9, 2024, 12:10 a.m. UTC | #3

Josh Steadmon <[e-mail]> writes:

> From: Calvin Wan <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Josh Steadmon <[e-mail]>
> ---
>  Makefile   | 16 ++++++++++++++++
>  t/Makefile | 16 ++++++++++++++++
>  2 files changed, 32 insertions(+)

After 

    $ make INCLUDE_LIBGIT_RS=YesPlease

running either

    $ make INCLUDE_LIBGIT_RS=YesPlease distclean
    $ make distclean

leaves

    $ git clean -n -x
    Would remove contrib/libgit-rs/libgit-sys/libgitpub.a

behind.  We'd need to add a bit more to the Makefile, it seems.



 Makefile | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git i/Makefile w/Makefile
index 41ad458aef..2acb5353d1 100644
--- i/Makefile
+++ w/Makefile
@@ -392,6 +392,9 @@ include shared.mak
 # INSTALL_STRIP can be set to "-s" to strip binaries during installation,
 # if your $(INSTALL) command supports the option.
 #
+# Define INCLUDE_LIBGIT_RS if you want your gostak to distim
+# the doshes.
+#
 # Define GENERATE_COMPILATION_DATABASE to "yes" to generate JSON compilation
 # database entries during compilation if your compiler supports it, using the
 # `-MJ` flag. The JSON entries will be placed in the `compile_commands/`
@@ -771,6 +774,9 @@ PROGRAM_OBJS += shell.o
 .PHONY: program-objs
 program-objs: $(PROGRAM_OBJS)
 
+# libgit-rs stuff
+LIBGITPUB_A = contrib/libgit-rs/libgit-sys/libgitpub.a
+
 # Binary suffix, set to .exe for Windows builds
 X =
 
@@ -3708,6 +3714,7 @@ clean: profile-clean coverage-clean cocciclean
 	$(RM) po/git.pot po/git-core.pot
 	$(RM) git.res
 	$(RM) $(OBJECTS)
+	$(RM) $(LIBGITPUB_A)
 	$(RM) headless-git.o
 	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
@@ -3892,5 +3899,5 @@ contrib/libgit-rs/libgit-sys/partial_symbol_export.o: contrib/libgit-rs/libgit-s
 contrib/libgit-rs/libgit-sys/hidden_symbol_export.o: contrib/libgit-rs/libgit-sys/partial_symbol_export.o
 	$(OBJCOPY) --localize-hidden $^ $@
 
-contrib/libgit-rs/libgit-sys/libgitpub.a: contrib/libgit-rs/libgit-sys/hidden_symbol_export.o
+$(LIBGITPUB_A): contrib/libgit-rs/libgit-sys/hidden_symbol_export.o
 	$(AR) $(ARFLAGS) $@ $^

Randall S. Becker Oct. 9, 2024, 12:12 a.m. UTC | #4

On October 8, 2024 7:46 PM, Junio C Hamano wrote:
>Josh Steadmon <[e-mail]> writes:
>
>> Add environment variable, INCLUDE_LIBGIT_RS, that when set,
>> automatically builds and tests libgit-rs and libgit-rs-sys when `make
>> all` is ran.
>
>Is this unusual, or is it just like how other makefile macros like say
USE_NSEC (to
>cause the resulting Git to use subsecond mtimes) are meant to be used to
control
>the build?  IOW, shouldn't this be documented near the top of the Makefile,
e.g.
>
>    diff --git i/Makefile w/Makefile
>    index 41ad458aef..2b55fe9672 100644
>    --- i/Makefile
>    +++ w/Makefile
>    @@ -392,6 +392,9 @@ include shared.mak
>     # INSTALL_STRIP can be set to "-s" to strip binaries during
installation,
>     # if your $(INSTALL) command supports the option.
>     #
>    +# Define INCLUDE_LIBGIT_RS if you want your gostak to distim
>    +# the doshes and ...
>    +#
>     # Define GENERATE_COMPILATION_DATABASE to "yes" to generate JSON
>compilation
>     # database entries during compilation if your compiler supports it,
using the
>     # `-MJ` flag. The JSON entries will be placed in the
`compile_commands/`
>
>It might make sense to follow naming convention to call it NO_RUST and flip
its
>polarity.  Those who do not have or want libgit-rs and friends can say
NO_RUST but
>otherwise it gets built by default.  It would give you a wider developer
population
>coverage.

Some of us who do not have Rust (yet) approve this message. I hope our
situation
will change on having Rust on NonStop.

Josh Steadmon Oct. 9, 2024, 9:53 p.m. UTC | #5

On 2024.10.08 17:01, Junio C Hamano wrote:
> Josh Steadmon <[e-mail]> writes:
> 
> > From: Calvin Wan <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Josh Steadmon <[e-mail]>
> > ---
> >  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.
> 
> > 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.

Hmm, I think this may be an unfortunate interaction between Git's `make
all`, followed by libgit-sys's `build.rs` calling make again (with
different CFLAGS, specifically '-fvisibility=hidden') to build
libgitpub.a. So then the second `make all` has to rebuild everything due
to changing the CFLAGS back, and then libgit-sys has to rebuild
libgitpub.a once again.

Unfortunately, I don't see a way around this problem, at least with our
current approach for building libgitpub.a. We have to pass
'-fvisibility=hidden' when compiling each source file, not just at link
time, so I think the object files created when building vanilla Git will
necessarily differ from those created when building libgit-rs.

I think that means we'll need to drop this patch for now, and revisit
if/when we change our libgitpub.a strategy.

Josh Steadmon Oct. 9, 2024, 10:24 p.m. UTC | #6

On 2024.10.08 17:10, Junio C Hamano wrote:
> Josh Steadmon <[e-mail]> writes:
> 
> > From: Calvin Wan <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Josh Steadmon <[e-mail]>
> > ---
> >  Makefile   | 16 ++++++++++++++++
> >  t/Makefile | 16 ++++++++++++++++
> >  2 files changed, 32 insertions(+)
> 
> After 
> 
>     $ make INCLUDE_LIBGIT_RS=YesPlease
> 
> running either
> 
>     $ make INCLUDE_LIBGIT_RS=YesPlease distclean
>     $ make distclean
> 
> leaves
> 
>     $ git clean -n -x
>     Would remove contrib/libgit-rs/libgit-sys/libgitpub.a
> 
> behind.  We'd need to add a bit more to the Makefile, it seems.
> 
> 
> 
>  Makefile | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git i/Makefile w/Makefile
> index 41ad458aef..2acb5353d1 100644
> --- i/Makefile
> +++ w/Makefile
> @@ -392,6 +392,9 @@ include shared.mak
>  # INSTALL_STRIP can be set to "-s" to strip binaries during installation,
>  # if your $(INSTALL) command supports the option.
>  #
> +# Define INCLUDE_LIBGIT_RS if you want your gostak to distim
> +# the doshes.
> +#
>  # Define GENERATE_COMPILATION_DATABASE to "yes" to generate JSON compilation
>  # database entries during compilation if your compiler supports it, using the
>  # `-MJ` flag. The JSON entries will be placed in the `compile_commands/`
> @@ -771,6 +774,9 @@ PROGRAM_OBJS += shell.o
>  .PHONY: program-objs
>  program-objs: $(PROGRAM_OBJS)
>  
> +# libgit-rs stuff
> +LIBGITPUB_A = contrib/libgit-rs/libgit-sys/libgitpub.a
> +
>  # Binary suffix, set to .exe for Windows builds
>  X =
>  
> @@ -3708,6 +3714,7 @@ clean: profile-clean coverage-clean cocciclean
>  	$(RM) po/git.pot po/git-core.pot
>  	$(RM) git.res
>  	$(RM) $(OBJECTS)
> +	$(RM) $(LIBGITPUB_A)
>  	$(RM) headless-git.o
>  	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
>  	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
> @@ -3892,5 +3899,5 @@ contrib/libgit-rs/libgit-sys/partial_symbol_export.o: contrib/libgit-rs/libgit-s
>  contrib/libgit-rs/libgit-sys/hidden_symbol_export.o: contrib/libgit-rs/libgit-sys/partial_symbol_export.o
>  	$(OBJCOPY) --localize-hidden $^ $@
>  
> -contrib/libgit-rs/libgit-sys/libgitpub.a: contrib/libgit-rs/libgit-sys/hidden_symbol_export.o
> +$(LIBGITPUB_A): contrib/libgit-rs/libgit-sys/hidden_symbol_export.o
>  	$(AR) $(ARFLAGS) $@ $^

Done in V5.

Junio C Hamano Oct. 10, 2024, 12:52 a.m. UTC | #7

Josh Steadmon <[e-mail]> writes:

> Hmm, I think this may be an unfortunate interaction between Git's `make
> all`, followed by libgit-sys's `build.rs` calling make again (with
> different CFLAGS, specifically '-fvisibility=hidden') to build
> libgitpub.a. So then the second `make all` has to rebuild everything due
> to changing the CFLAGS back, and then libgit-sys has to rebuild
> libgitpub.a once again.

Ah, OK, if we need to compile in two different ways, then it is a
matter of giving a dedicated *.o build directory to each, and until
that happens, the object files for Git proper and libgit-sys would
try to stomp on each other.

I thought Patrick's build procedure update has out-of-tree build as
one of its goals, in which case we may be able to piggy-back on the
effort once it starts to stabilize.

Thanks.

Josh Steadmon Oct. 14, 2024, 8:13 p.m. UTC | #8

On 2024.10.09 17:52, Junio C Hamano wrote:
> Josh Steadmon <[e-mail]> writes:
> 
> > Hmm, I think this may be an unfortunate interaction between Git's `make
> > all`, followed by libgit-sys's `build.rs` calling make again (with
> > different CFLAGS, specifically '-fvisibility=hidden') to build
> > libgitpub.a. So then the second `make all` has to rebuild everything due
> > to changing the CFLAGS back, and then libgit-sys has to rebuild
> > libgitpub.a once again.
> 
> Ah, OK, if we need to compile in two different ways, then it is a
> matter of giving a dedicated *.o build directory to each, and until
> that happens, the object files for Git proper and libgit-sys would
> try to stomp on each other.
> 
> I thought Patrick's build procedure update has out-of-tree build as
> one of its goals, in which case we may be able to piggy-back on the
> effort once it starts to stabilize.
> 
> Thanks.

Actually, including '-fvisibility=hidden' by default doesn't break the
main build, so I think we can just add this to BASIC_CFLAGS if
INCLUDE_LIBGIT_RS is set. I'll do that (and add documentation as
requested) and restore this patch in V5.

Josh Steadmon Oct. 14, 2024, 8:19 p.m. UTC | #9

On 2024.10.08 16:45, Junio C Hamano wrote:
> Josh Steadmon <[e-mail]> writes:
> 
> > Add environment variable, INCLUDE_LIBGIT_RS, that when set,
> > automatically builds and tests libgit-rs and libgit-rs-sys when `make
> > all` is ran.
> 
> Is this unusual, or is it just like how other makefile macros like
> say USE_NSEC (to cause the resulting Git to use subsecond mtimes)
> are meant to be used to control the build?  IOW, shouldn't this be
> documented near the top of the Makefile, e.g.
> 
>     diff --git i/Makefile w/Makefile
>     index 41ad458aef..2b55fe9672 100644
>     --- i/Makefile
>     +++ w/Makefile
>     @@ -392,6 +392,9 @@ include shared.mak
>      # INSTALL_STRIP can be set to "-s" to strip binaries during installation,
>      # if your $(INSTALL) command supports the option.
>      #
>     +# Define INCLUDE_LIBGIT_RS if you want your gostak to distim
>     +# the doshes and ...
>     +#
>      # Define GENERATE_COMPILATION_DATABASE to "yes" to generate JSON compilation
>      # database entries during compilation if your compiler supports it, using the
>      # `-MJ` flag. The JSON entries will be placed in the `compile_commands/`
> 
> It might make sense to follow naming convention to call it NO_RUST
> and flip its polarity.  Those who do not have or want libgit-rs and
> friends can say NO_RUST but otherwise it gets built by default.  It
> would give you a wider developer population coverage.
> 
> Thanks.

For now I'd be more comfortable keeping it off by default. I don't want
to force those not interested in Rust to work around our in-progress
projects. Once it's more stable and we have CI I'd feel better about
turning it on by default (and maybe moving it out of contrib/ at that
point?).

Patch

diff --git a/Makefile b/Makefile
index abeee01d9e..41ad458aef 100644
--- a/Makefile
+++ b/Makefile
@@ -3870,6 +3870,22 @@  build-unit-tests: $(UNIT_TEST_PROGS)
 unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X
 	$(MAKE) -C t/ unit-tests
 
+.PHONY: libgitrs-sys
+libgitrs-sys:
+	$(QUIET)(\
+		cd contrib/libgit-rs/libgit-sys && \
+		cargo build \
+	)
+.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 $@
 
diff --git a/t/Makefile b/t/Makefile
index b2eb9f770b..066cb5c2b4 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -169,3 +169,19 @@  perf:
 
 .PHONY: pre-clean $(T) aggregate-results clean valgrind perf \
 	check-chainlint clean-chainlint test-chainlint $(UNIT_TESTS)
+
+.PHONY: libgitrs-sys-test
+libgitrs-sys-test:
+	$(QUIET)(\
+		cd ../contrib/libgit-rs/libgit-sys && \
+		cargo test \
+	)
+.PHONY: libgitrs-test
+libgitrs-test:
+	$(QUIET)(\
+		cd ../contrib/libgit-rs && \
+		cargo test \
+	)
+ifdef INCLUDE_LIBGIT_RS
+all:: libgitrs-sys-test libgitrs-test
+endif