Makefile update libgit.a: Include xdiff and reftable in libgit.a #2065
+23
−39
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.2065.git.git.1759341748.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> The Rust compiler only needs to know how to link against libgit.a in 2 cases
> that I can think of:
>
> * Rust unit tests
> * Rust defining the main function
>
> Otherwise Rust can be compiled without linking, and then Makefile and Meson
> can use Cargo's produced static lib files to build Git.
It is a bit unclear why two (or three) are so much more hassle than
one, but OK. Allowing both build systems to agree on the same set
of artifacts is very much desirable, and if meson based build rolls
everythning into a single library archive, the the other one should
do the same.
Of course we could run "ar" ourselves and combine the three into a
single library archive, but as an approach, what you have here is a
perfectly fine, and more preferrable, way to achieve the goal of
ending up with a single archive file.
This topic, however, especially its first step, had caused rather
unpleasant textual conflicts when merged to 'seen' (I didn't check
which other topic was the most heavily conflicting, though). I may
attempt to get a clean merge again tomorrow, but due to time
pressure, tonight's 'seen' was done without these patches merged.
Thanks. |
| export PERL_PATH | ||
| export PYTHON_PATH | ||
|
|
||
| TEST_SHELL_PATH = $(SHELL_PATH) |
There was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
On Wed, Oct 01, 2025 at 06:02:27PM +0000, Ezekiel Newren via GitGitGadget wrote:
> diff --git a/Makefile b/Makefile
> index e8fad803be..d89ba03286 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1397,8 +1396,7 @@ XDIFF_OBJS += xdiff/xmerge.o
> XDIFF_OBJS += xdiff/xpatience.o
> XDIFF_OBJS += xdiff/xprepare.o
> XDIFF_OBJS += xdiff/xutils.o
> -.PHONY: xdiff-objs
> -xdiff-objs: $(XDIFF_OBJS)
The removal of the `xdiff-objs` target isn't mentioned or justified in
the commit message. I personally don't mind that this target goes away,
as I don't really have a use case for it anyway. But in theory it could
continue to exist. So I'd either retain it, or explain why it goes away.
In case it goes away, is there still a reason to have the separate
XDIFF_OBJS variable? Can't we add these objects to `LIB_OBJS` directly?
PatrickThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Patrick Steinhardt <ps@pks.im> writes:
> On Wed, Oct 01, 2025 at 06:02:27PM +0000, Ezekiel Newren via GitGitGadget wrote:
>> diff --git a/Makefile b/Makefile
>> index e8fad803be..d89ba03286 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1397,8 +1396,7 @@ XDIFF_OBJS += xdiff/xmerge.o
>> XDIFF_OBJS += xdiff/xpatience.o
>> XDIFF_OBJS += xdiff/xprepare.o
>> XDIFF_OBJS += xdiff/xutils.o
>> -.PHONY: xdiff-objs
>> -xdiff-objs: $(XDIFF_OBJS)
>
> The removal of the `xdiff-objs` target isn't mentioned or justified in
> the commit message. I personally don't mind that this target goes away,
> as I don't really have a use case for it anyway. But in theory it could
> continue to exist. So I'd either retain it, or explain why it goes away.
>
> In case it goes away, is there still a reason to have the separate
> XDIFF_OBJS variable? Can't we add these objects to `LIB_OBJS` directly?
Doing it this way lets us still keep the "logical" organization to
tell which object is which, even though we may lose physical
distinction by throwing all objects in a single library archive.
Incidentally this would slightly reduce the patch noise and make the
result more merge friendly when other topics are in flight that
touch these (like adding a new file or two to REFTABLE_OBJS), but
with the movement of these lines in [1/3], that benefit is
diminished.
There was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
On Thu, Oct 02, 2025 at 06:31:33AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > On Wed, Oct 01, 2025 at 06:02:27PM +0000, Ezekiel Newren via GitGitGadget wrote:
> >> diff --git a/Makefile b/Makefile
> >> index e8fad803be..d89ba03286 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -1397,8 +1396,7 @@ XDIFF_OBJS += xdiff/xmerge.o
> >> XDIFF_OBJS += xdiff/xpatience.o
> >> XDIFF_OBJS += xdiff/xprepare.o
> >> XDIFF_OBJS += xdiff/xutils.o
> >> -.PHONY: xdiff-objs
> >> -xdiff-objs: $(XDIFF_OBJS)
> >
> > The removal of the `xdiff-objs` target isn't mentioned or justified in
> > the commit message. I personally don't mind that this target goes away,
> > as I don't really have a use case for it anyway. But in theory it could
> > continue to exist. So I'd either retain it, or explain why it goes away.
> >
> > In case it goes away, is there still a reason to have the separate
> > XDIFF_OBJS variable? Can't we add these objects to `LIB_OBJS` directly?
>
> Doing it this way lets us still keep the "logical" organization to
> tell which object is which, even though we may lose physical
> distinction by throwing all objects in a single library archive.
Well, I guess the logical organization still exists due to all the files
living in "xdiff/" and "reftable/", respectively. So I'm not sure that's
a definitive win.
But in any case, I don't have any strong feelings here. I mostly
wondered whether we can simplify the build infra even further.
PatrickThere was a problem hiding this comment.
On the Git mailing list, Ezekiel Newren wrote (reply to this):
On Thu, Oct 2, 2025 at 9:33 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Thu, Oct 02, 2025 at 06:31:33AM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > On Wed, Oct 01, 2025 at 06:02:27PM +0000, Ezekiel Newren via GitGitGadget wrote:
> > >> diff --git a/Makefile b/Makefile
> > >> index e8fad803be..d89ba03286 100644
> > >> --- a/Makefile
> > >> +++ b/Makefile
> > >> @@ -1397,8 +1396,7 @@ XDIFF_OBJS += xdiff/xmerge.o
> > >> XDIFF_OBJS += xdiff/xpatience.o
> > >> XDIFF_OBJS += xdiff/xprepare.o
> > >> XDIFF_OBJS += xdiff/xutils.o
> > >> -.PHONY: xdiff-objs
> > >> -xdiff-objs: $(XDIFF_OBJS)
> > >
> > > The removal of the `xdiff-objs` target isn't mentioned or justified in
> > > the commit message. I personally don't mind that this target goes away,
> > > as I don't really have a use case for it anyway. But in theory it could
> > > continue to exist. So I'd either retain it, or explain why it goes away.
> > >
> > > In case it goes away, is there still a reason to have the separate
> > > XDIFF_OBJS variable? Can't we add these objects to `LIB_OBJS` directly?
> >
> > Doing it this way lets us still keep the "logical" organization to
> > tell which object is which, even though we may lose physical
> > distinction by throwing all objects in a single library archive.
>
> Well, I guess the logical organization still exists due to all the files
> living in "xdiff/" and "reftable/", respectively. So I'm not sure that's
> a definitive win.
>
> But in any case, I don't have any strong feelings here. I mostly
> wondered whether we can simplify the build infra even further.
My preference is the same as yours Patrick. In my Introduce Rust v2
series (that I dropped) I did it the way that you described. I changed
how I did things because of Junio's suggestion. I think doing it
Patrick's way would be more consistent because in Meson the
`libgit_sources` variable includes all C files that are part of
libgit. That variable includes the sources for reftable and xdiff.
snippet from meson.build:
libgit_sources = [
...
'reftable/basics.c',
'reftable/error.c',
'reftable/block.c',
'reftable/blocksource.c',
'reftable/iter.c',
'reftable/merged.c',
'reftable/pq.c',
'reftable/record.c',
'reftable/stack.c',
'reftable/system.c',
'reftable/table.c',
'reftable/tree.c',
'reftable/writer.c',
...
'xdiff/xdiffi.c',
'xdiff/xemit.c',
'xdiff/xhistogram.c',
'xdiff/xmerge.c',
'xdiff/xpatience.c',
'xdiff/xprepare.c',
'xdiff/xutils.c',
]
I will go with your preference Junio. Do you prefer your way or Patrick's way?There was a problem hiding this comment.
On the Git mailing list, Ezekiel Newren wrote (reply to this):
On Wed, Oct 1, 2025 at 11:47 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Wed, Oct 01, 2025 at 06:02:27PM +0000, Ezekiel Newren via GitGitGadget wrote:
> > diff --git a/Makefile b/Makefile
> > index e8fad803be..d89ba03286 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1397,8 +1396,7 @@ XDIFF_OBJS += xdiff/xmerge.o
> > XDIFF_OBJS += xdiff/xpatience.o
> > XDIFF_OBJS += xdiff/xprepare.o
> > XDIFF_OBJS += xdiff/xutils.o
> > -.PHONY: xdiff-objs
> > -xdiff-objs: $(XDIFF_OBJS)
>
> The removal of the `xdiff-objs` target isn't mentioned or justified in
> the commit message. I personally don't mind that this target goes away,
> as I don't really have a use case for it anyway. But in theory it could
> continue to exist. So I'd either retain it, or explain why it goes away.
If I understand correctly, the PHONY target xdiff-objs is to make sure
that xdiff/lib.a is always built. But if it's folded into libgit.a
then its purpose becomes moot. I will update my commit message to make
this clear.There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Ezekiel Newren <ezekielnewren@gmail.com> writes:
> I will go with your preference Junio. Do you prefer your way or Patrick's way?
With step [1/3] shuffling lines around, the advantage of keeping
XDIFF_OBJS and REFTABLE_OBJS to make the result merge-friendly
disappeared, so I am perfectly fine to throw everything into
LIB_OBJS.
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Ezekiel Newren wrote (reply to this):
On Thu, Oct 2, 2025 at 1:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Ezekiel Newren <ezekielnewren@gmail.com> writes:
>
> > I will go with your preference Junio. Do you prefer your way or Patrick's way?
>
> With step [1/3] shuffling lines around, the advantage of keeping
> XDIFF_OBJS and REFTABLE_OBJS to make the result merge-friendly
> disappeared, so I am perfectly fine to throw everything into
> LIB_OBJS.
Sounds good. I'll do that in my next version.|
User |
| export PERL_PATH | ||
| export PYTHON_PATH | ||
|
|
||
| TEST_SHELL_PATH = $(SHELL_PATH) |
There was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
On Wed, Oct 01, 2025 at 06:02:28PM +0000, Ezekiel Newren via GitGitGadget wrote:
> diff --git a/Makefile b/Makefile
> index d89ba03286..4c63045443 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1411,9 +1410,9 @@ REFTABLE_OBJS += reftable/system.o
> REFTABLE_OBJS += reftable/table.o
> REFTABLE_OBJS += reftable/tree.o
> REFTABLE_OBJS += reftable/writer.o
> +LIB_OBJS += $(REFTABLE_OBJS)
>
> -# reftable lib may in turn depend on what is in libgit.a
> -GITLIBS = common-main.o $(LIB_FILE) $(REFTABLE_LIB) $(LIB_FILE)
> +GITLIBS = common-main.o $(LIB_FILE)
> EXTLIBS =
>
> GIT_USER_AGENT = git/$(GIT_VERSION)
Same question here as on the preceding commit: do we even need
REFTABLE_OBJS anymore?
Other than that these patches look sensible to me, thanks. Even without
Rust they simplify our build infra a bit, so I think that landing them
independently of Rust is a good thing.
Thanks!
PatrickThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Patrick Steinhardt <ps@pks.im> writes:
> On Wed, Oct 01, 2025 at 06:02:28PM +0000, Ezekiel Newren via GitGitGadget wrote:
>> diff --git a/Makefile b/Makefile
>> index d89ba03286..4c63045443 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1411,9 +1410,9 @@ REFTABLE_OBJS += reftable/system.o
>> REFTABLE_OBJS += reftable/table.o
>> REFTABLE_OBJS += reftable/tree.o
>> REFTABLE_OBJS += reftable/writer.o
>> +LIB_OBJS += $(REFTABLE_OBJS)
>>
>> -# reftable lib may in turn depend on what is in libgit.a
>> -GITLIBS = common-main.o $(LIB_FILE) $(REFTABLE_LIB) $(LIB_FILE)
>> +GITLIBS = common-main.o $(LIB_FILE)
>> EXTLIBS =
>>
>> GIT_USER_AGENT = git/$(GIT_VERSION)
>
> Same question here as on the preceding commit: do we even need
> REFTABLE_OBJS anymore?
Same answer as before.
> Other than that these patches look sensible to me, thanks. Even without
> Rust they simplify our build infra a bit, so I think that landing them
> independently of Rust is a good thing.
Thanks.There was a problem hiding this comment.
On the Git mailing list, Ezekiel Newren wrote (reply to this):
On Wed, Oct 1, 2025 at 11:49 PM Patrick Steinhardt <ps@pks.im> wrote:
> Other than that these patches look sensible to me, thanks. Even without
> Rust they simplify our build infra a bit, so I think that landing them
> independently of Rust is a good thing.
I agree. I've been trying to find and fix things that don't depend on
Rust, but will need to be changed to make the adoption of Rust
smoother.
Thanks for your feedback.|
On the Git mailing list, Ezekiel Newren wrote (reply to this): On Wed, Oct 1, 2025 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > The Rust compiler only needs to know how to link against libgit.a in 2 cases
> > that I can think of:
> >
> > * Rust unit tests
> > * Rust defining the main function
> >
> > Otherwise Rust can be compiled without linking, and then Makefile and Meson
> > can use Cargo's produced static lib files to build Git.
>
> It is a bit unclear why two (or three) are so much more hassle than
> one, but OK. Allowing both build systems to agree on the same set
> of artifacts is very much desirable, and if meson based build rolls
> everything into a single library archive, the the other one should
> do the same.
>
> Of course we could run "ar" ourselves and combine the three into a
> single library archive, but as an approach, what you have here is a
> perfectly fine, and more preferable, way to achieve the goal of
> ending up with a single archive file.
In order for Cargo to tell the Rust compiler how to link against the C
archives they need to be specified in the build.rs file for each
crate. That would look something like this:
tree gitcore/
gitcore/
├── build.rs
├── Cargo.toml
└── src
├── do_that.rs
├── do_this.rs
└── lib.rs
Where gitcore/build.rs would look something like this:
fn main() {
...
println!("cargo:rustc-link-search=native={}", git_build_dir);
println!("cargo:rustc-link-lib=static=git");
if let Some(built_with_makefile) = std::env::var("BUILT_WITH_MAKEFILE") {
println!("cargo:rustc-link-search=native={}",
git_build_dir.join("xdiff"));
println!("cargo:rustc-link-lib=static=xdiff");
println!("cargo:rustc-link-search=native={}",
git_build_dir.join("reftable"));
println!("cargo:rustc-link-lib=static=reftable");
}
}
But `cargo build` is invoked by a shell script by both Makefile and
Meson, and build.rs would need to have an environment variable set.
Something like BUILT_WITH_MAKEFILE for only when build_rust.sh is
invoked by makefile and then you'd have to remember to update both
Makefile and every build.rs if the library files produced by Makefile
are ever changed. It seems a lot cleaner to hardcode just libgit.a and
leave it up to the build systems to ensure that everything inside that
static library contains everything that is needed.
> This topic, however, especially its first step, had caused rather
> unpleasant textual conflicts when merged to 'seen' (I didn't check
> which other topic was the most heavily conflicting, though). I may
> attempt to get a clean merge again tomorrow, but due to time
> pressure, tonight's 'seen' was done without these patches merged.
I wonder if Patrick's approach would cause fewer merge conflicts. If I
add xdiff and reftable objects to LIB_OBJS directly then I don't need
to bother with the move commit. |
|
This branch is now known as |
|
This patch series was integrated into seen via d30e0f3. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
> This topic, however, especially its first step, had caused rather
> unpleasant textual conflicts when merged to 'seen' (I didn't check
> which other topic was the most heavily conflicting, though). I may
> attempt to get a clean merge again tomorrow, but due to time
> pressure, tonight's 'seen' was done without these patches merged.
I think I have sorted it out. I'll push out 'seen' with this topic
at the tip, so could you please check the resulting Makefile for any
funny mismerges?
Thanks. |
|
On the Git mailing list, Ezekiel Newren wrote (reply to this): On Thu, Oct 2, 2025 at 3:02 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > This topic, however, especially its first step, had caused rather
> > unpleasant textual conflicts when merged to 'seen' (I didn't check
> > which other topic was the most heavily conflicting, though). I may
> > attempt to get a clean merge again tomorrow, but due to time
> > pressure, tonight's 'seen' was done without these patches merged.
>
> I think I have sorted it out. I'll push out 'seen' with this topic
> at the tip, so could you please check the resulting Makefile for any
> funny mismerges?
Looks like it merged correctly. I'd like to release a v2 where objects
are directly added to LIB_OBJS.
Here is what I ran to compare it to what I created:
git rev-parse next
a91ca5db0318b6fda5a6721ee843f56e7e2fadfc
git range-diff d30e0f3024^1..d30e0f3024^2
next..merge_xdiff_and_reftable_with_libgit_v1
1: 40d798afad ! 1: fdcf5a0de8 make: move xdiff and reftable
objects before GITLIBS
@@ Commit message
with --color-moved.
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
- Signed-off-by: Junio C Hamano <gitster@pobox.com>
## Makefile ##
@@ Makefile: CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
2: 22d1910ec8 ! 2: 28e7fd27b6 make: delete XDIFF_LIB, add xdiff to LIB_OBJS
@@ Commit message
included in Meson's libgit.a.
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
- Signed-off-by: Junio C Hamano <gitster@pobox.com>
## Makefile ##
@@ Makefile: export PYTHON_PATH
3: 9f1670c048 ! 3: 8549f63415 make: delete REFTABLE_LIB, add
reftable to LIB_OBJS
@@ Commit message
reftable will be turned into a Rust crate.
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
- Signed-off-by: Junio C Hamano <gitster@pobox.com>
## Makefile ##
@@ Makefile: export PYTHON_PATH |
8549f63 to
74e76af
Compare
In a future patch series the 'xdiff' Rust crate will be added. Delete the creation of the static library file for xdiff to avoid a name conflict. This also moves toward the goal of Rust only needing to link against libgit.a. Changes to Meson are not required as the xdiff library is already included in Meson's libgit.a. xdiff-objs was a historical make target to allow building just the objects in xdiff. Since it was defined in terms of XDIFF_OBJS (which no longer exists) this convenience make target no longer makes sense. Remove it. Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
Same idea as the previous commit except that I don't know when or if reftable will be turned into a Rust crate. Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
74e76af to
9031610
Compare
|
/submit |
|
Submitted as pull.2065.v2.git.git.1759447647.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes in v2:
>
> * Add xdiff and reftable objects directly to LIB_OBJS.
> * Explain why xdiff-objs is removed.
Both changes look sensible. Will queue. Thanks. |
|
This patch series was integrated into seen via 73d8bfc. |
|
Inputdata=("googprice"),"daily",2018,1,1") |
|
This patch series was integrated into seen via 0846535. |
|
This patch series was integrated into seen via 216485f. |
|
This patch series was integrated into seen via 4c85548. |
|
There was a status update in the "Cooking" section about the branch Instead of three library archives (one for git, one for reftable, and one for xdiff), roll everything into a single libgit.a archive. This would help later effort to FFI into Rust. Will merge to 'next'? source: <pull.2065.v2.git.git.1759447647.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 89d2f8c. |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Fri, Oct 03, 2025 at 09:43:09AM -0700, Junio C Hamano wrote:
> "Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Changes in v2:
> >
> > * Add xdiff and reftable objects directly to LIB_OBJS.
> > * Explain why xdiff-objs is removed.
>
> Both changes look sensible. Will queue. Thanks.
Agreed, this version looks good to me. Thanks!
Patrick |
|
This patch series was integrated into seen via 62f584b. |
|
There was a status update in the "Cooking" section about the branch Instead of three library archives (one for git, one for reftable, and one for xdiff), roll everything into a single libgit.a archive. This would help later effort to FFI into Rust. Will merge to 'next'. source: <pull.2065.v2.git.git.1759447647.gitgitgadget@gmail.com> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes in v2:
Original cover letter:
Add xdiff and reftable to the static library libgit.a that Makefile produces. Meson does not require any changes since it already includes those libraries. The motivation is to simplify Rust's job of linking against the C code by requiring it to
only link against a single static library (libgit.a).
The Rust compiler only needs to know how to link against libgit.a in 2 cases that I can think of:
Otherwise Rust can be compiled without linking, and then Makefile and Meson can use Cargo's produced static lib files to build Git.
Note: The flag -fPIE or -fPIC is required for Makefile to build libgit.a in a way that Cargo can use. It has been deliberately omitted from the Makefile, for now, since Rust isn't part of Git (yet).
cc: Patrick Steinhardt ps@pks.im