On 2024.10.08 17:01, Junio C Hamano wrote: > Josh Steadmon <steadmon@xxxxxxxxxx> writes: > > > From: Calvin Wan <calvinwan@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > Signed-off-by: Josh Steadmon <steadmon@xxxxxxxxxx> > > --- > > 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.
