Re: [PATCH v6 2/5] libgit-sys: introduce Rust wrapper for libgit.a

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 15, 2025, 23:13 UTC
Message-ID: <xmqqa5brwtzq.fsf@gitster.g>
In-Reply-To: <5fc66cdb1628e0c9e420f3f0455779d7471f46ee.1736971328.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 10 quoted lines

> diff --git a/Makefile b/Makefile
> index 27e68ac039..47e864a861 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -657,6 +657,8 @@ CURL_CONFIG = curl-config
>  GCOV = gcov
>  STRIP = strip
>  SPATCH = spatch
> +LD = ld
> +OBJCOPY = objcopy

This assumes GNU binutils is available. As long as our intention is to start the Rust support as an optional feature, that is OK. Hopefully the piece that requires $(OBJCOPY) is arranged to be easily opted out. Let's keep reading.

Show 5 quoted lines

> @@ -2731,6 +2733,7 @@ OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)
>  OBJECTS += $(UNIT_TEST_OBJS)
>  OBJECTS += $(CLAR_TEST_OBJS)
>  OBJECTS += $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
> +OBJECTS += contrib/libgit-sys/public_symbol_export.o

This is compiled for everybody, even for those whose platform cannot support Rust interface (or those who choose not to build it). As long as what is in the file is written portably, it is fine to have stubs and entry points that their build will not use.

Show 10 quoted lines

>  ifndef NO_CURL
>  	OBJECTS += http.o http-walker.o remote-curl.o
> @@ -3726,6 +3729,10 @@ clean: profile-clean coverage-clean cocciclean
>  	$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
>  	$(MAKE) -C Documentation/ clean
>  	$(RM) Documentation/GIT-EXCLUDED-PROGRAMS
> +	$(RM) -r contrib/libgit-sys/target
> +	$(RM) -r contrib/libgit-sys/partial_symbol_export.o
> +	$(RM) -r contrib/libgit-sys/hidden_symbol_export.o
> +	$(RM) -r contrib/libgit-sys/libgitpub.a

Which one of the above is a directory? The latter three smells like a regular file, so we shouldn't say "-r" there.

Show 7 quoted lines

> @@ -3887,3 +3894,12 @@ $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GITLIBS) GIT-
>  build-unit-tests: $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG)
>  unit-tests: $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) t/helper/test-tool$X
>  	$(MAKE) -C t/ unit-tests
> +
> +contrib/libgit-sys/partial_symbol_export.o: contrib/libgit-sys/public_symbol_export.o libgit.a reftable/libreftable.a xdiff/lib.a
> +	$(LD) -r $^ -o $@

OK. We build a "relocatable" object, which is unconditionally made as part of $(OBJECTS) above. Even without GNU binutils "ld", people hopefully can convince their linker to do the equivalent. I am not sure if it is healthy to assume that such a linker also uses "-r" for the feature, so we may have to make this rule more customizable, or make partial_symbol_export.o only conditionally part of $(OBJECTS) to allow them to opt out.

> +contrib/libgit-sys/hidden_symbol_export.o: contrib/libgit-sys/partial_symbol_export.o
> +	$(OBJCOPY) --localize-hidden $^ $@

Unlike the "public" thing, hidden_symbol_export.o was not made part of $(OBJECTS), so this part is arranged to allow people without $(OBJCOPY) to easily opt out of this part of the system, which is good.

> +contrib/libgit-sys/libgitpub.a: contrib/libgit-sys/hidden_symbol_export.o
> +	$(AR) $(ARFLAGS) $@ $^

Likewise, people can easily opt out of building "libgitpub.a", which is good (these targets are triggered only from build.rs).

Show 10 quoted lines

> diff --git a/contrib/libgit-sys/README.md b/contrib/libgit-sys/README.md
> new file mode 100644
> index 0000000000..c061cfcaf5
> --- /dev/null
> +++ b/contrib/libgit-sys/README.md
> @@ -0,0 +1,4 @@
> +# libgit-sys
> +
> +A small proof-of-concept crate showing how to provide a Rust FFI to Git
> +internals.

OK.

Show 9 quoted lines

> diff --git a/contrib/libgit-sys/public_symbol_export.c b/contrib/libgit-sys/public_symbol_export.c
> new file mode 100644
> index 0000000000..7cd5007902
> --- /dev/null
> +++ b/contrib/libgit-sys/public_symbol_export.c
> @@ -0,0 +1,21 @@
> +// Shim to publicly export Git symbols. These must be renamed so that the
> +// original symbols can be hidden. Renaming these with a "libgit_" prefix also
> +// avoids conflicts with other libraries such as libgit2.

Style.

Show 17 quoted lines

> +#include "git-compat-util.h"
> +#include "contrib/libgit-sys/public_symbol_export.h"
> +#include "version.h"
> +
> +#pragma GCC visibility push(default)
> +
> +const char *libgit_user_agent(void)
> +{
> +	return git_user_agent();
> +}
> +
> +const char *libgit_user_agent_sanitized(void)
> +{
> +	return git_user_agent_sanitized();
> +}
> +
> +#pragma GCC visibility pop

I do not think we would mind not having Rust binding support on platforms without GCC (and clang---I assume it would be aware of and react to that #pragma GCC the same way?). But do we allow this file to be left uncompiled when the build wants to opt out of Rust support?

Show 14 quoted lines

> diff --git a/contrib/libgit-sys/public_symbol_export.h b/contrib/libgit-sys/public_symbol_export.h
> new file mode 100644
> index 0000000000..a3372f93fa
> --- /dev/null
> +++ b/contrib/libgit-sys/public_symbol_export.h
> @@ -0,0 +1,8 @@
> +#ifndef PUBLIC_SYMBOL_EXPORT_H
> +#define PUBLIC_SYMBOL_EXPORT_H
> +
> +const char *libgit_user_agent(void);
> +
> +const char *libgit_user_agent_sanitized(void);
> +
> +#endif /* PUBLIC_SYMBOL_EXPORT_H */

OK.