[PATCH] Makefile: build libgit-rs and libgit-sys serially

[PATCH] Makefile: build libgit-rs and libgit-sys serially

[David Aguilar]

The "cargo build" invocations in contrib/ cannot be run in parallel.

"make -JN" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings
and can trigger ld errors during the build.

The build errors are caused by two inner "make" invocations getting
triggered concurrently: once inside of libgit-sys and another inside of
libgit-rs.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 29a53520fd..286d3ba3b2 100644
--- a/Makefile
+++ b/Makefile
@@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:
 		cargo build \
 	)
 ifdef INCLUDE_LIBGIT_RS
-all:: libgit-sys libgit-rs
+all:: libgit-sys .WAIT libgit-rs
 endif
 
 LIBGIT_PUB_OBJS += contrib/libgit-sys/public_symbol_export.o
-- 
2.50.0.7.gec2f25360c

Re: [PATCH] Makefile: build libgit-rs and libgit-sys serially

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> The "cargo build" invocations in contrib/ cannot be run in parallel.
>
> "make -JN" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings
> and can trigger ld errors during the build.
>
> The build errors are caused by two inner "make" invocations getting
> triggered concurrently: once inside of libgit-sys and another inside of
> libgit-rs.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Don't we need a similar change to t/Makefile, or "cargo test" does
fine while "cargo build" cannot be run in parallel?

>
> diff --git a/Makefile b/Makefile
> index 29a53520fd..286d3ba3b2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:
>  		cargo build \
>  	)
>  ifdef INCLUDE_LIBGIT_RS
> -all:: libgit-sys libgit-rs
> +all:: libgit-sys .WAIT libgit-rs
>  endif
>  
>  LIBGIT_PUB_OBJS += contrib/libgit-sys/public_symbol_export.o

Re: [PATCH] Makefile: build libgit-rs and libgit-sys serially

[Kyle Lippincott]

On Tue, Aug 26, 2025 at 9:04 AM David Aguilar <davvid@gmail.com> wrote:
>
> The "cargo build" invocations in contrib/ cannot be run in parallel.
>
> "make -JN" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings
> and can trigger ld errors during the build.
>
> The build errors are caused by two inner "make" invocations getting
> triggered concurrently: once inside of libgit-sys and another inside of
> libgit-rs.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 29a53520fd..286d3ba3b2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:
>                 cargo build \
>         )
>  ifdef INCLUDE_LIBGIT_RS
> -all:: libgit-sys libgit-rs
> +all:: libgit-sys .WAIT libgit-rs

I'm not familiar enough with make or with rust, but do we need to
depend on both of these here? Wouldn't it be sufficient to say
libgit-rs depends on libgit-sys, and only explicitly depend on
libgit-rs in `all::`?

>  endif
>
>  LIBGIT_PUB_OBJS += contrib/libgit-sys/public_symbol_export.o
> --
> 2.50.0.7.gec2f25360c
>

RE: [PATCH] Makefile: build libgit-rs and libgit-sys serially

[rsbecker]

On August 26, 2025 1:45 PM, Kyle Lippincott wrote:
>On Tue, Aug 26, 2025 at 9:04 AM David Aguilar <davvid@gmail.com> wrote:
>>
>> The "cargo build" invocations in contrib/ cannot be run in parallel.
>>
>> "make -JN" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings
>> and can trigger ld errors during the build.
>>
>> The build errors are caused by two inner "make" invocations getting
>> triggered concurrently: once inside of libgit-sys and another inside
>> of libgit-rs.
>>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> ---
>>  Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 29a53520fd..286d3ba3b2 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:
>>                 cargo build \
>>         )
>>  ifdef INCLUDE_LIBGIT_RS
>> -all:: libgit-sys libgit-rs
>> +all:: libgit-sys .WAIT libgit-rs
>
>I'm not familiar enough with make or with rust, but do we need to depend on both
>of these here? Wouldn't it be sufficient to say libgit-rs depends on libgit-sys, and
>only explicitly depend on libgit-rs in `all::`?

Not all platforms can build libgit-rs, so inserting it into as a required component is not
a particularly friendly idea.

Re: [PATCH] Makefile: build libgit-rs and libgit-sys serially

[Kyle Lippincott]

On Tue, Aug 26, 2025 at 10:53 AM <rsbecker@nexbridge.com> wrote:
>
> On August 26, 2025 1:45 PM, Kyle Lippincott wrote:
> >On Tue, Aug 26, 2025 at 9:04 AM David Aguilar <davvid@gmail.com> wrote:
> >>
> >> The "cargo build" invocations in contrib/ cannot be run in parallel.
> >>
> >> "make -JN" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings
> >> and can trigger ld errors during the build.
> >>
> >> The build errors are caused by two inner "make" invocations getting
> >> triggered concurrently: once inside of libgit-sys and another inside
> >> of libgit-rs.
> >>
> >> Signed-off-by: David Aguilar <davvid@gmail.com>
> >> ---
> >>  Makefile | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/Makefile b/Makefile
> >> index 29a53520fd..286d3ba3b2 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:
> >>                 cargo build \
> >>         )
> >>  ifdef INCLUDE_LIBGIT_RS
> >> -all:: libgit-sys libgit-rs
> >> +all:: libgit-sys .WAIT libgit-rs
> >
> >I'm not familiar enough with make or with rust, but do we need to depend on both
> >of these here? Wouldn't it be sufficient to say libgit-rs depends on libgit-sys, and
> >only explicitly depend on libgit-rs in `all::`?
>
> Not all platforms can build libgit-rs, so inserting it into as a required component is not
> a particularly friendly idea.
>

That's not what I was suggesting. This is already in an `ifdef`, and
the line I was quoting was changing `all:: libgit-sys libgit-rs` to
`all:: libgit-sys .WAIT libgit-rs`. I'm wondering if we can instead
split the `libgit-sys libgit-rs:` from a few lines earlier into
`libgit-sys:` and `libgit-rs: libgit-sys` and then change this all
line to `all:: libgit-rs` (still behind `ifdef INCLUDE_LIBGIT_RS`).

RE: [PATCH] Makefile: build libgit-rs and libgit-sys serially

[rsbecker]

On August 26, 2025 2:03 PM, Kyle Lippincott wrote:
>On Tue, Aug 26, 2025 at 10:53 AM <rsbecker@nexbridge.com> wrote:
>>
>> On August 26, 2025 1:45 PM, Kyle Lippincott wrote:
>> >On Tue, Aug 26, 2025 at 9:04 AM David Aguilar <davvid@gmail.com> wrote:
>> >>
>> >> The "cargo build" invocations in contrib/ cannot be run in parallel.
>> >>
>> >> "make -JN" with INCLUDE_LIBGIT_RS enabled causes cargo lock
>> >> warnings and can trigger ld errors during the build.
>> >>
>> >> The build errors are caused by two inner "make" invocations getting
>> >> triggered concurrently: once inside of libgit-sys and another
>> >> inside of libgit-rs.
>> >>
>> >> Signed-off-by: David Aguilar <davvid@gmail.com>
>> >> ---
>> >>  Makefile | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/Makefile b/Makefile
>> >> index 29a53520fd..286d3ba3b2 100644
>> >> --- a/Makefile
>> >> +++ b/Makefile
>> >> @@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:
>> >>                 cargo build \
>> >>         )
>> >>  ifdef INCLUDE_LIBGIT_RS
>> >> -all:: libgit-sys libgit-rs
>> >> +all:: libgit-sys .WAIT libgit-rs
>> >
>> >I'm not familiar enough with make or with rust, but do we need to
>> >depend on both of these here? Wouldn't it be sufficient to say
>> >libgit-rs depends on libgit-sys, and only explicitly depend on libgit-rs in `all::`?
>>
>> Not all platforms can build libgit-rs, so inserting it into as a
>> required component is not a particularly friendly idea.
>>
>
>That's not what I was suggesting. This is already in an `ifdef`, and the line I was
>quoting was changing `all:: libgit-sys libgit-rs` to
>`all:: libgit-sys .WAIT libgit-rs`. I'm wondering if we can instead split the `libgit-sys
>libgit-rs:` from a few lines earlier into `libgit-sys:` and `libgit-rs: libgit-sys` and then
>change this all line to `all:: libgit-rs` (still behind `ifdef INCLUDE_LIBGIT_RS`).

Thank you. I appreciate this and the clarity it provides.

Randall

[PATCH v2] Makefile: build libgit-rs and libgit-sys serially

[David Aguilar]

"make -JN" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings
and can trigger ld errors during the build.

The build errors are caused by two inner "make" invocations getting
triggered concurrently: once inside of libgit-sys and another inside of
libgit-rs.

Make libgit-rs depend on libgit-sys so that "make" prevents them
from running concurrently. Apply the same logic to the test invocations.
Use cargo's "--manifest-path" option instead of "cd" in the recipes.

Signed-off-by: David Aguilar <davvid@gmail.com>
---

Differences since v0:

* The targets have been split apart into
separate targets so that the libgit-rs targets can be made to
depend on the libgit-sys targets.

* cargo build/test --manifest-path is being used to simplify
the build recipe by eliminating the "cd" step, which would
have been duplicated in the split-out target.

* t/Makefile has been updated to apply the same logic.

 Makefile   | 11 +++++------
 t/Makefile | 14 ++++----------
 2 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/Makefile b/Makefile
index 29a53520fd..539e6907b4 100644
--- a/Makefile
+++ b/Makefile
@@ -3983,13 +3983,12 @@ unit-tests: $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) t/helper/test-tool$X
 	$(MAKE) -C t/ unit-tests
 
 .PHONY: libgit-sys libgit-rs
-libgit-sys libgit-rs:
-	$(QUIET)(\
-		cd contrib/$@ && \
-		cargo build \
-	)
+libgit-sys:
+	$(QUIET)cargo build --manifest-path contrib/libgit-sys/Cargo.toml
+libgit-rs: libgit-sys
+	$(QUIET)cargo build --manifest-path contrib/libgit-rs/Cargo.toml
 ifdef INCLUDE_LIBGIT_RS
-all:: libgit-sys libgit-rs
+all:: libgit-rs
 endif
 
 LIBGIT_PUB_OBJS += contrib/libgit-sys/public_symbol_export.o
diff --git a/t/Makefile b/t/Makefile
index 791e0a0978..29dd226c7d 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -190,15 +190,9 @@ perf:
 
 .PHONY: libgit-sys-test libgit-rs-test
 libgit-sys-test:
-	$(QUIET)(\
-		cd ../contrib/libgit-sys && \
-		cargo test \
-	)
-libgit-rs-test:
-	$(QUIET)(\
-		cd ../contrib/libgit-rs && \
-		cargo test \
-	)
+	$(QUIET)cargo test --manifest-path ../contrib/libgit-sys/Cargo.toml
+libgit-rs-test: libgit-sys-test
+	$(QUIET)cargo test --manifest-path ../contrib/libgit-rs/Cargo.toml
 ifdef INCLUDE_LIBGIT_RS
-all:: libgit-sys-test libgit-rs-test
+all:: libgit-rs-test
 endif
-- 
2.50.0.7.ge90cf88798

Re: [PATCH v2] Makefile: build libgit-rs and libgit-sys serially

[Kyle Lippincott]

On Tue, Aug 26, 2025 at 4:35 PM David Aguilar <davvid@gmail.com> wrote:
>
> "make -JN" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings
> and can trigger ld errors during the build.
>
> The build errors are caused by two inner "make" invocations getting
> triggered concurrently: once inside of libgit-sys and another inside of
> libgit-rs.
>
> Make libgit-rs depend on libgit-sys so that "make" prevents them
> from running concurrently. Apply the same logic to the test invocations.
> Use cargo's "--manifest-path" option instead of "cd" in the recipes.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>
> Differences since v0:
>
> * The targets have been split apart into
> separate targets so that the libgit-rs targets can be made to
> depend on the libgit-sys targets.
>
> * cargo build/test --manifest-path is being used to simplify
> the build recipe by eliminating the "cd" step, which would
> have been duplicated in the split-out target.
>
> * t/Makefile has been updated to apply the same logic.
>
>  Makefile   | 11 +++++------
>  t/Makefile | 14 ++++----------
>  2 files changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 29a53520fd..539e6907b4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3983,13 +3983,12 @@ unit-tests: $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) t/helper/test-tool$X
>         $(MAKE) -C t/ unit-tests
>
>  .PHONY: libgit-sys libgit-rs
> -libgit-sys libgit-rs:
> -       $(QUIET)(\
> -               cd contrib/$@ && \
> -               cargo build \
> -       )
> +libgit-sys:
> +       $(QUIET)cargo build --manifest-path contrib/libgit-sys/Cargo.toml
> +libgit-rs: libgit-sys
> +       $(QUIET)cargo build --manifest-path contrib/libgit-rs/Cargo.toml
>  ifdef INCLUDE_LIBGIT_RS
> -all:: libgit-sys libgit-rs
> +all:: libgit-rs
>  endif
>
>  LIBGIT_PUB_OBJS += contrib/libgit-sys/public_symbol_export.o
> diff --git a/t/Makefile b/t/Makefile
> index 791e0a0978..29dd226c7d 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -190,15 +190,9 @@ perf:
>
>  .PHONY: libgit-sys-test libgit-rs-test
>  libgit-sys-test:
> -       $(QUIET)(\
> -               cd ../contrib/libgit-sys && \
> -               cargo test \
> -       )
> -libgit-rs-test:
> -       $(QUIET)(\
> -               cd ../contrib/libgit-rs && \
> -               cargo test \
> -       )
> +       $(QUIET)cargo test --manifest-path ../contrib/libgit-sys/Cargo.toml
> +libgit-rs-test: libgit-sys-test
> +       $(QUIET)cargo test --manifest-path ../contrib/libgit-rs/Cargo.toml
>  ifdef INCLUDE_LIBGIT_RS
> -all:: libgit-sys-test libgit-rs-test
> +all:: libgit-rs-test
>  endif
> --
> 2.50.0.7.ge90cf88798

This version looks good to me, thanks!

Re: [PATCH v2] Makefile: build libgit-rs and libgit-sys serially

[Junio C Hamano]

Kyle Lippincott <spectral@google.com> writes:

> On Tue, Aug 26, 2025 at 4:35 PM David Aguilar <davvid@gmail.com> wrote:
>>
>> "make -JN" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings
>> and can trigger ld errors during the build.
>>
>> The build errors are caused by two inner "make" invocations getting
>> triggered concurrently: once inside of libgit-sys and another inside of
>> libgit-rs.
>>
>> Make libgit-rs depend on libgit-sys so that "make" prevents them
>> from running concurrently. Apply the same logic to the test invocations.
>> Use cargo's "--manifest-path" option instead of "cd" in the recipes.
>> ....
>> +       $(QUIET)cargo test --manifest-path ../contrib/libgit-sys/Cargo.toml
>> +libgit-rs-test: libgit-sys-test
>> +       $(QUIET)cargo test --manifest-path ../contrib/libgit-rs/Cargo.toml
>>  ifdef INCLUDE_LIBGIT_RS
>> -all:: libgit-sys-test libgit-rs-test
>> +all:: libgit-rs-test
>>  endif
>> --
>> 2.50.0.7.ge90cf88798
>
> This version looks good to me, thanks!

Thanks, both.  Will apply.