| Message ID | 5fc66cdb1628e0c9e420f3f0455779d7471f46ee.1736971328.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce libgit-rs, a Rust wrapper around libgit.a | expand |
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.
On 2025.01.15 15:13, Junio C Hamano wrote: > 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. Later on in the series we add an INCLUDE_LIBGIT_RS variable to control builds and tests; I don't see any reason why we can't move that earlier in the series, so I'll do so in V7 and then look at what we need to do to make things more portable. > > 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.
diff --git a/.gitignore b/.gitignore index e82aa19df0..31d7e64287 100644 --- a/.gitignore +++ b/.gitignore @@ -250,3 +250,4 @@ Release/ /git.VC.db *.dSYM /contrib/buildsystems/out +/contrib/libgit-sys/target 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 export TCL_PATH TCLTK_PATH @@ -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 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 ifndef NO_PERL $(RM) -r perl/build/ endif @@ -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 $@ + +contrib/libgit-sys/hidden_symbol_export.o: contrib/libgit-sys/partial_symbol_export.o + $(OBJCOPY) --localize-hidden $^ $@ + +contrib/libgit-sys/libgitpub.a: contrib/libgit-sys/hidden_symbol_export.o + $(AR) $(ARFLAGS) $@ $^ diff --git a/contrib/libgit-sys/Cargo.lock b/contrib/libgit-sys/Cargo.lock new file mode 100644 index 0000000000..427a4c66b7 --- /dev/null +++ b/contrib/libgit-sys/Cargo.lock @@ -0,0 +1,69 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 3 + +[[package]] +name = "autocfg" +version = "1.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ace50bade8e6234aa140d9a2f552bbee1db4d353f69b8217bc503490fc1a9f26" + +[[package]] +name = "cc" +version = "1.1.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "57b6a275aa2903740dc87da01c62040406b8812552e97129a63ea8850a17c6e6" +dependencies = [ + "shlex", +] + +[[package]] +name = "libc" +version = "0.2.158" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d8adc4bb1803a324070e64a98ae98f38934d91957a99cfb3a43dcbc01bc56439" + +[[package]] +name = "libgit-sys" +version = "0.1.0" +dependencies = [ + "autocfg", + "libz-sys", + "make-cmd", +] + +[[package]] +name = "libz-sys" +version = "1.1.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d2d16453e800a8cf6dd2fc3eb4bc99b786a9b90c663b8559a5b1a041bf89e472" +dependencies = [ + "cc", + "libc", + "pkg-config", + "vcpkg", +] + +[[package]] +name = "make-cmd" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a8ca8afbe8af1785e09636acb5a41e08a765f5f0340568716c18a8700ba3c0d3" + +[[package]] +name = "pkg-config" +version = "0.3.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d231b230927b5e4ad203db57bbcbee2802f6bce620b1e4a9024a07d94e2907ec" + +[[package]] +name = "shlex" +version = "1.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0fda2ff0d084019ba4d7c6f371c95d8fd75ce3524c3cb8fb653a3023f6323e64" + +[[package]] +name = "vcpkg" +version = "0.2.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "accd4ea62f7bb7a82fe23066fb0957d48ef677f6eeb8215f372f52e48bb32426" diff --git a/contrib/libgit-sys/Cargo.toml b/contrib/libgit-sys/Cargo.toml new file mode 100644 index 0000000000..15b28c3585 --- /dev/null +++ b/contrib/libgit-sys/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "libgit-sys" +version = "0.1.0" +edition = "2021" +build = "build.rs" +links = "gitpub" +rust-version = "1.63" +description = "Native bindings to a portion of libgit" + +[lib] +path = "src/lib.rs" + +[dependencies] +libz-sys = "1.1.19" + +[build-dependencies] +autocfg = "1.4.0" +make-cmd = "0.1.0" 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. diff --git a/contrib/libgit-sys/build.rs b/contrib/libgit-sys/build.rs new file mode 100644 index 0000000000..b6c65193bc --- /dev/null +++ b/contrib/libgit-sys/build.rs @@ -0,0 +1,35 @@ +use std::env; +use std::path::PathBuf; + +pub fn main() -> std::io::Result<()> { + let ac = autocfg::new(); + ac.emit_has_path("std::ffi::c_char"); + + let crate_root = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap()); + let git_root = crate_root.join("../.."); + let dst = PathBuf::from(env::var_os("OUT_DIR").unwrap()); + + let make_output = make_cmd::gnu_make() + .env("DEVELOPER", "1") + .env_remove("PROFILE") + .current_dir(git_root.clone()) + .args([ + "CFLAGS=-fvisibility=hidden", + "contrib/libgit-sys/libgitpub.a", + ]) + .output() + .expect("Make failed to run"); + if !make_output.status.success() { + panic!( + "Make failed:\n stdout = {}\n stderr = {}\n", + String::from_utf8(make_output.stdout).unwrap(), + String::from_utf8(make_output.stderr).unwrap() + ); + } + std::fs::copy(crate_root.join("libgitpub.a"), dst.join("libgitpub.a"))?; + println!("cargo:rustc-link-search=native={}", dst.display()); + println!("cargo:rustc-link-lib=gitpub"); + println!("cargo:rerun-if-changed={}", git_root.display()); + + 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. + +#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 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 */ diff --git a/contrib/libgit-sys/src/lib.rs b/contrib/libgit-sys/src/lib.rs new file mode 100644 index 0000000000..d4853f3074 --- /dev/null +++ b/contrib/libgit-sys/src/lib.rs @@ -0,0 +1,46 @@ +#[cfg(has_std__ffi__c_char)] +use std::ffi::c_char; + +#[cfg(not(has_std__ffi__c_char))] +#[allow(non_camel_case_types)] +pub type c_char = i8; + +extern crate libz_sys; + +extern "C" { + pub fn libgit_user_agent() -> *const c_char; + pub fn libgit_user_agent_sanitized() -> *const c_char; +} + +#[cfg(test)] +mod tests { + use std::ffi::CStr; + + use super::*; + + #[test] + fn user_agent_starts_with_git() { + let c_str = unsafe { CStr::from_ptr(libgit_user_agent()) }; + let agent = c_str + .to_str() + .expect("User agent contains invalid UTF-8 data"); + assert!( + agent.starts_with("git/"), + r#"Expected user agent to start with "git/", got: {}"#, + agent + ); + } + + #[test] + fn sanitized_user_agent_starts_with_git() { + let c_str = unsafe { CStr::from_ptr(libgit_user_agent_sanitized()) }; + let agent = c_str + .to_str() + .expect("Sanitized user agent contains invalid UTF-8 data"); + assert!( + agent.starts_with("git/"), + r#"Expected user agent to start with "git/", got: {}"#, + agent + ); + } +}