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

[Junio C Hamano <gitster@pobox.com>]

Josh Steadmon <steadmon@google.com> writes:

> 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.

> @@ -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.

>  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.

> @@ -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).

> 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.

> 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.

> +#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?

> 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.