{"thread":{"id":"64038","subject":"[PATCH] Makefile: build libgit-rs and libgit-sys serially","startedAt":"2025-08-26T16:04:41Z","lastAt":"2025-08-27T00:01:24Z","messageCount":9,"participants":["David Aguilar","Junio C Hamano","Kyle Lippincott","rsbecker@nexbridge.com"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"524971","messageId":"20250826160437.2539113-1-davvid@gmail.com","threadId":"64038","inReplyTo":null,"subject":"[PATCH] Makefile: build libgit-rs and libgit-sys serially","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2025-08-26T16:04:37Z","receivedAt":"2025-08-26T16:04:41Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The \"cargo build\" invocations in contrib/ cannot be run in parallel.\n\n\"make -JN\" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings\nand can trigger ld errors during the build.\n\nThe build errors are caused by two inner \"make\" invocations getting\ntriggered concurrently: once inside of libgit-sys and another inside of\nlibgit-rs.\n\nSigned-off-by: David Aguilar \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 29a53520fd..286d3ba3b2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:\n \t\tcargo build \\\n \t)\n ifdef INCLUDE_LIBGIT_RS\n-all:: libgit-sys libgit-rs\n+all:: libgit-sys .WAIT libgit-rs\n endif\n \n LIBGIT_PUB_OBJS += contrib/libgit-sys/public_symbol_export.o\n-- \n2.50.0.7.gec2f25360c\n\n"},{"id":"524979","messageId":"xmqq7byqkp3p.fsf@gitster.g","threadId":"64038","inReplyTo":"20250826160437.2539113-1-davvid@gmail.com","subject":"Re: [PATCH] Makefile: build libgit-rs and libgit-sys serially","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-26T16:46:34Z","receivedAt":"2025-08-26T16:46:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> The \"cargo build\" invocations in contrib/ cannot be run in parallel.\n>\n> \"make -JN\" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings\n> and can trigger ld errors during the build.\n>\n> The build errors are caused by two inner \"make\" invocations getting\n> triggered concurrently: once inside of libgit-sys and another inside of\n> libgit-rs.\n>\n> Signed-off-by: David Aguilar \n> ---\n> Makefile | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nDon't we need a similar change to t/Makefile, or \"cargo test\" does\nfine while \"cargo build\" cannot be run in parallel?\n\n>\n> diff --git a/Makefile b/Makefile\n> index 29a53520fd..286d3ba3b2 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:\n> \t\tcargo build \\\n> \t)\n> ifdef INCLUDE_LIBGIT_RS\n> -all:: libgit-sys libgit-rs\n> +all:: libgit-sys .WAIT libgit-rs\n> endif\n> \n> LIBGIT_PUB_OBJS += contrib/libgit-sys/public_symbol_export.o\n"},{"id":"524981","messageId":"CAO_smVjviMdpZyHFp4zJc62DJYAZxLAc5yw68C3U+c5wbwRziA@mail.gmail.com","threadId":"64038","inReplyTo":"20250826160437.2539113-1-davvid@gmail.com","subject":"Re: [PATCH] Makefile: build libgit-rs and libgit-sys serially","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2025-08-26T17:44:46Z","receivedAt":"2025-08-26T17:44:59Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Tue, Aug 26, 2025 at 9:04 AM David Aguilar wrote:\n>\n> The \"cargo build\" invocations in contrib/ cannot be run in parallel.\n>\n> \"make -JN\" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings\n> and can trigger ld errors during the build.\n>\n> The build errors are caused by two inner \"make\" invocations getting\n> triggered concurrently: once inside of libgit-sys and another inside of\n> libgit-rs.\n>\n> Signed-off-by: David Aguilar \n> ---\n> Makefile | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 29a53520fd..286d3ba3b2 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:\n> cargo build \\\n> )\n> ifdef INCLUDE_LIBGIT_RS\n> -all:: libgit-sys libgit-rs\n> +all:: libgit-sys .WAIT libgit-rs\n\nI'm not familiar enough with make or with rust, but do we need to\ndepend on both of these here? Wouldn't it be sufficient to say\nlibgit-rs depends on libgit-sys, and only explicitly depend on\nlibgit-rs in `all::`?\n\n> endif\n>\n> LIBGIT_PUB_OBJS += contrib/libgit-sys/public_symbol_export.o\n> --\n> 2.50.0.7.gec2f25360c\n>\n"},{"id":"524982","messageId":"014b01dc16b2$4a1dd0d0$de597270$@nexbridge.com","threadId":"64038","inReplyTo":"CAO_smVjviMdpZyHFp4zJc62DJYAZxLAc5yw68C3U+c5wbwRziA@mail.gmail.com","subject":"RE: [PATCH] Makefile: build libgit-rs and libgit-sys serially","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2025-08-26T17:53:05Z","receivedAt":"2025-08-26T17:53:22Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On August 26, 2025 1:45 PM, Kyle Lippincott wrote:\n>On Tue, Aug 26, 2025 at 9:04 AM David Aguilar wrote:\n>>\n>> The \"cargo build\" invocations in contrib/ cannot be run in parallel.\n>>\n>> \"make -JN\" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings\n>> and can trigger ld errors during the build.\n>>\n>> The build errors are caused by two inner \"make\" invocations getting\n>> triggered concurrently: once inside of libgit-sys and another inside\n>> of libgit-rs.\n>>\n>> Signed-off-by: David Aguilar \n>> ---\n>> Makefile | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/Makefile b/Makefile\n>> index 29a53520fd..286d3ba3b2 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:\n>> cargo build \\\n>> )\n>> ifdef INCLUDE_LIBGIT_RS\n>> -all:: libgit-sys libgit-rs\n>> +all:: libgit-sys .WAIT libgit-rs\n>\n>I'm not familiar enough with make or with rust, but do we need to depend on both\n>of these here? Wouldn't it be sufficient to say libgit-rs depends on libgit-sys, and\n>only explicitly depend on libgit-rs in `all::`?\n\nNot all platforms can build libgit-rs, so inserting it into as a required component is not\na particularly friendly idea.\n\n"},{"id":"524983","messageId":"CAO_smViKBsfzvqAfu587V_FXp=OPgu3yeO-P8qw19jgwizuUhg@mail.gmail.com","threadId":"64038","inReplyTo":"014b01dc16b2$4a1dd0d0$de597270$@nexbridge.com","subject":"Re: [PATCH] Makefile: build libgit-rs and libgit-sys serially","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2025-08-26T18:02:50Z","receivedAt":"2025-08-26T18:03:14Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Tue, Aug 26, 2025 at 10:53 AM wrote:\n>\n> On August 26, 2025 1:45 PM, Kyle Lippincott wrote:\n> >On Tue, Aug 26, 2025 at 9:04 AM David Aguilar wrote:\n> >>\n> >> The \"cargo build\" invocations in contrib/ cannot be run in parallel.\n> >>\n> >> \"make -JN\" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings\n> >> and can trigger ld errors during the build.\n> >>\n> >> The build errors are caused by two inner \"make\" invocations getting\n> >> triggered concurrently: once inside of libgit-sys and another inside\n> >> of libgit-rs.\n> >>\n> >> Signed-off-by: David Aguilar \n> >> ---\n> >> Makefile | 2 +-\n> >> 1 file changed, 1 insertion(+), 1 deletion(-)\n> >>\n> >> diff --git a/Makefile b/Makefile\n> >> index 29a53520fd..286d3ba3b2 100644\n> >> --- a/Makefile\n> >> +++ b/Makefile\n> >> @@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:\n> >> cargo build \\\n> >> )\n> >> ifdef INCLUDE_LIBGIT_RS\n> >> -all:: libgit-sys libgit-rs\n> >> +all:: libgit-sys .WAIT libgit-rs\n> >\n> >I'm not familiar enough with make or with rust, but do we need to depend on both\n> >of these here? Wouldn't it be sufficient to say libgit-rs depends on libgit-sys, and\n> >only explicitly depend on libgit-rs in `all::`?\n>\n> Not all platforms can build libgit-rs, so inserting it into as a required component is not\n> a particularly friendly idea.\n>\n\nThat's not what I was suggesting. This is already in an `ifdef`, and\nthe line I was quoting was changing `all:: libgit-sys libgit-rs` to\n`all:: libgit-sys .WAIT libgit-rs`. I'm wondering if we can instead\nsplit the `libgit-sys libgit-rs:` from a few lines earlier into\n`libgit-sys:` and `libgit-rs: libgit-sys` and then change this all\nline to `all:: libgit-rs` (still behind `ifdef INCLUDE_LIBGIT_RS`).\n"},{"id":"524985","messageId":"014f01dc16b3$fdcc8b70$f965a250$@nexbridge.com","threadId":"64038","inReplyTo":"CAO_smViKBsfzvqAfu587V_FXp=OPgu3yeO-P8qw19jgwizuUhg@mail.gmail.com","subject":"RE: [PATCH] Makefile: build libgit-rs and libgit-sys serially","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2025-08-26T18:05:16Z","receivedAt":"2025-08-26T18:05:27Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On August 26, 2025 2:03 PM, Kyle Lippincott wrote:\n>On Tue, Aug 26, 2025 at 10:53 AM wrote:\n>>\n>> On August 26, 2025 1:45 PM, Kyle Lippincott wrote:\n>> >On Tue, Aug 26, 2025 at 9:04 AM David Aguilar wrote:\n>> >>\n>> >> The \"cargo build\" invocations in contrib/ cannot be run in parallel.\n>> >>\n>> >> \"make -JN\" with INCLUDE_LIBGIT_RS enabled causes cargo lock\n>> >> warnings and can trigger ld errors during the build.\n>> >>\n>> >> The build errors are caused by two inner \"make\" invocations getting\n>> >> triggered concurrently: once inside of libgit-sys and another\n>> >> inside of libgit-rs.\n>> >>\n>> >> Signed-off-by: David Aguilar \n>> >> ---\n>> >> Makefile | 2 +-\n>> >> 1 file changed, 1 insertion(+), 1 deletion(-)\n>> >>\n>> >> diff --git a/Makefile b/Makefile\n>> >> index 29a53520fd..286d3ba3b2 100644\n>> >> --- a/Makefile\n>> >> +++ b/Makefile\n>> >> @@ -3989,7 +3989,7 @@ libgit-sys libgit-rs:\n>> >> cargo build \\\n>> >> )\n>> >> ifdef INCLUDE_LIBGIT_RS\n>> >> -all:: libgit-sys libgit-rs\n>> >> +all:: libgit-sys .WAIT libgit-rs\n>> >\n>> >I'm not familiar enough with make or with rust, but do we need to\n>> >depend on both of these here? Wouldn't it be sufficient to say\n>> >libgit-rs depends on libgit-sys, and only explicitly depend on libgit-rs in `all::`?\n>>\n>> Not all platforms can build libgit-rs, so inserting it into as a\n>> required component is not a particularly friendly idea.\n>>\n>\n>That's not what I was suggesting. This is already in an `ifdef`, and the line I was\n>quoting was changing `all:: libgit-sys libgit-rs` to\n>`all:: libgit-sys .WAIT libgit-rs`. I'm wondering if we can instead split the `libgit-sys\n>libgit-rs:` from a few lines earlier into `libgit-sys:` and `libgit-rs: libgit-sys` and then\n>change this all line to `all:: libgit-rs` (still behind `ifdef INCLUDE_LIBGIT_RS`).\n\nThank you. I appreciate this and the clarity it provides.\n\nRandall\n\n"},{"id":"525008","messageId":"20250826233525.2635432-1-davvid@gmail.com","threadId":"64038","inReplyTo":"xmqq7byqkp3p.fsf@gitster.g","subject":"[PATCH v2] Makefile: build libgit-rs and libgit-sys serially","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2025-08-26T23:35:25Z","receivedAt":"2025-08-26T23:35:28Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"\"make -JN\" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings\nand can trigger ld errors during the build.\n\nThe build errors are caused by two inner \"make\" invocations getting\ntriggered concurrently: once inside of libgit-sys and another inside of\nlibgit-rs.\n\nMake libgit-rs depend on libgit-sys so that \"make\" prevents them\nfrom running concurrently. Apply the same logic to the test invocations.\nUse cargo's \"--manifest-path\" option instead of \"cd\" in the recipes.\n\nSigned-off-by: David Aguilar \n---\n\nDifferences since v0:\n\n* The targets have been split apart into\nseparate targets so that the libgit-rs targets can be made to\ndepend on the libgit-sys targets.\n\n* cargo build/test --manifest-path is being used to simplify\nthe build recipe by eliminating the \"cd\" step, which would\nhave been duplicated in the split-out target.\n\n* t/Makefile has been updated to apply the same logic.\n\n Makefile | 11 +++++------\n t/Makefile | 14 ++++----------\n 2 files changed, 9 insertions(+), 16 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 29a53520fd..539e6907b4 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3983,13 +3983,12 @@ unit-tests: $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) t/helper/test-tool$X\n \t$(MAKE) -C t/ unit-tests\n \n .PHONY: libgit-sys libgit-rs\n-libgit-sys libgit-rs:\n-\t$(QUIET)(\\\n-\t\tcd contrib/$@ && \\\n-\t\tcargo build \\\n-\t)\n+libgit-sys:\n+\t$(QUIET)cargo build --manifest-path contrib/libgit-sys/Cargo.toml\n+libgit-rs: libgit-sys\n+\t$(QUIET)cargo build --manifest-path contrib/libgit-rs/Cargo.toml\n ifdef INCLUDE_LIBGIT_RS\n-all:: libgit-sys libgit-rs\n+all:: libgit-rs\n endif\n \n LIBGIT_PUB_OBJS += contrib/libgit-sys/public_symbol_export.o\ndiff --git a/t/Makefile b/t/Makefile\nindex 791e0a0978..29dd226c7d 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -190,15 +190,9 @@ perf:\n \n .PHONY: libgit-sys-test libgit-rs-test\n libgit-sys-test:\n-\t$(QUIET)(\\\n-\t\tcd ../contrib/libgit-sys && \\\n-\t\tcargo test \\\n-\t)\n-libgit-rs-test:\n-\t$(QUIET)(\\\n-\t\tcd ../contrib/libgit-rs && \\\n-\t\tcargo test \\\n-\t)\n+\t$(QUIET)cargo test --manifest-path ../contrib/libgit-sys/Cargo.toml\n+libgit-rs-test: libgit-sys-test\n+\t$(QUIET)cargo test --manifest-path ../contrib/libgit-rs/Cargo.toml\n ifdef INCLUDE_LIBGIT_RS\n-all:: libgit-sys-test libgit-rs-test\n+all:: libgit-rs-test\n endif\n-- \n2.50.0.7.ge90cf88798\n\n"},{"id":"525009","messageId":"CAO_smViX+EVyq5AzO3dwfcBGdenuZ1w89ksse=6MXYv8xi+q1g@mail.gmail.com","threadId":"64038","inReplyTo":"20250826233525.2635432-1-davvid@gmail.com","subject":"Re: [PATCH v2] Makefile: build libgit-rs and libgit-sys serially","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2025-08-26T23:48:07Z","receivedAt":"2025-08-26T23:48:20Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Tue, Aug 26, 2025 at 4:35 PM David Aguilar wrote:\n>\n> \"make -JN\" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings\n> and can trigger ld errors during the build.\n>\n> The build errors are caused by two inner \"make\" invocations getting\n> triggered concurrently: once inside of libgit-sys and another inside of\n> libgit-rs.\n>\n> Make libgit-rs depend on libgit-sys so that \"make\" prevents them\n> from running concurrently. Apply the same logic to the test invocations.\n> Use cargo's \"--manifest-path\" option instead of \"cd\" in the recipes.\n>\n> Signed-off-by: David Aguilar \n> ---\n>\n> Differences since v0:\n>\n> * The targets have been split apart into\n> separate targets so that the libgit-rs targets can be made to\n> depend on the libgit-sys targets.\n>\n> * cargo build/test --manifest-path is being used to simplify\n> the build recipe by eliminating the \"cd\" step, which would\n> have been duplicated in the split-out target.\n>\n> * t/Makefile has been updated to apply the same logic.\n>\n> Makefile | 11 +++++------\n> t/Makefile | 14 ++++----------\n> 2 files changed, 9 insertions(+), 16 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 29a53520fd..539e6907b4 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3983,13 +3983,12 @@ unit-tests: $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) t/helper/test-tool$X\n> $(MAKE) -C t/ unit-tests\n>\n> .PHONY: libgit-sys libgit-rs\n> -libgit-sys libgit-rs:\n> - $(QUIET)(\\\n> - cd contrib/$@ && \\\n> - cargo build \\\n> - )\n> +libgit-sys:\n> + $(QUIET)cargo build --manifest-path contrib/libgit-sys/Cargo.toml\n> +libgit-rs: libgit-sys\n> + $(QUIET)cargo build --manifest-path contrib/libgit-rs/Cargo.toml\n> ifdef INCLUDE_LIBGIT_RS\n> -all:: libgit-sys libgit-rs\n> +all:: libgit-rs\n> endif\n>\n> LIBGIT_PUB_OBJS += contrib/libgit-sys/public_symbol_export.o\n> diff --git a/t/Makefile b/t/Makefile\n> index 791e0a0978..29dd226c7d 100644\n> --- a/t/Makefile\n> +++ b/t/Makefile\n> @@ -190,15 +190,9 @@ perf:\n>\n> .PHONY: libgit-sys-test libgit-rs-test\n> libgit-sys-test:\n> - $(QUIET)(\\\n> - cd ../contrib/libgit-sys && \\\n> - cargo test \\\n> - )\n> -libgit-rs-test:\n> - $(QUIET)(\\\n> - cd ../contrib/libgit-rs && \\\n> - cargo test \\\n> - )\n> + $(QUIET)cargo test --manifest-path ../contrib/libgit-sys/Cargo.toml\n> +libgit-rs-test: libgit-sys-test\n> + $(QUIET)cargo test --manifest-path ../contrib/libgit-rs/Cargo.toml\n> ifdef INCLUDE_LIBGIT_RS\n> -all:: libgit-sys-test libgit-rs-test\n> +all:: libgit-rs-test\n> endif\n> --\n> 2.50.0.7.ge90cf88798\n\nThis version looks good to me, thanks!\n"},{"id":"525011","messageId":"xmqqh5xtfx9p.fsf@gitster.g","threadId":"64038","inReplyTo":"CAO_smViX+EVyq5AzO3dwfcBGdenuZ1w89ksse=6MXYv8xi+q1g@mail.gmail.com","subject":"Re: [PATCH v2] Makefile: build libgit-rs and libgit-sys serially","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-27T00:01:22Z","receivedAt":"2025-08-27T00:01:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Lippincott writes:\n\n> On Tue, Aug 26, 2025 at 4:35 PM David Aguilar wrote:\n>>\n>> \"make -JN\" with INCLUDE_LIBGIT_RS enabled causes cargo lock warnings\n>> and can trigger ld errors during the build.\n>>\n>> The build errors are caused by two inner \"make\" invocations getting\n>> triggered concurrently: once inside of libgit-sys and another inside of\n>> libgit-rs.\n>>\n>> Make libgit-rs depend on libgit-sys so that \"make\" prevents them\n>> from running concurrently. Apply the same logic to the test invocations.\n>> Use cargo's \"--manifest-path\" option instead of \"cd\" in the recipes.\n>> ....\n>> + $(QUIET)cargo test --manifest-path ../contrib/libgit-sys/Cargo.toml\n>> +libgit-rs-test: libgit-sys-test\n>> + $(QUIET)cargo test --manifest-path ../contrib/libgit-rs/Cargo.toml\n>> ifdef INCLUDE_LIBGIT_RS\n>> -all:: libgit-sys-test libgit-rs-test\n>> +all:: libgit-rs-test\n>> endif\n>> --\n>> 2.50.0.7.ge90cf88798\n>\n> This version looks good to me, thanks!\n\nThanks, both. Will apply.\n"}]}