| Message ID | cover.1736971328.git.steadmon@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce libgit-rs, a Rust wrapper around libgit.a | expand |
Josh Steadmon <steadmon@google.com> writes: > Apologies for the long delay on V6; I am finally back after several > months of $DAYJOB firefighting, holidays, and sick leave. I should have > time to devote to this series again, but given the lack of feedback on > V5 I am hopeful that this will be the final iteration of this series. Thanks and welcome back ;-) Given the lack of feedback on the previous round, I hope we will see enthused support on the topic. Otherwise it is hard to tell if the previous lack of feedback was merely lack of interest, or lack of anything lacking in the series. > There is known NEEDSWORK, but I feel that they can be addressed in > follow-up changes, rather than in this series. If you feel otherwise, > please let me know: > > * Investigate alternative methods of managing symbol visibility & > renaming. > > * Figure out symbol versioning OK. Let's see what people find out.
On 2025-01-15 at 20:05:39, Josh Steadmon wrote: > Apologies for the long delay on V6; I am finally back after several > months of $DAYJOB firefighting, holidays, and sick leave. I should have > time to devote to this series again, but given the lack of feedback on > V5 I am hopeful that this will be the final iteration of this series. > > This series provides two small Rust wrapper libraries around parts of > Git: "libgit-sys", which exposes a few functions from libgit.a, and > "libgit", which provides a more Rust-friendly interface to some of those > functions. In addition to included unit tests, at $DAYJOB we have tested > building JJ[1] with our library and used it to replace some of the > libgit2-rs uses. > > [1] https://github.com/martinvonz/jj > > There is known NEEDSWORK, but I feel that they can be addressed in > follow-up changes, rather than in this series. If you feel otherwise, > please let me know: > > * Investigate alternative methods of managing symbol visibility & > renaming. > > * Figure out symbol versioning It looks like we're building a general Rust lib crate and a static library here, so symbol versioning isn't an issue. I expect in the future we may want to provide a shared library, in which case we will indeed want to do that, but I agree that can wait until later. In any event, I overall think this series is a nice improvement, and I am very enthusiastic about it. (This is mostly for the benefit of Junio, since I think the authors of this series already know that.) Once it lands, I do plan to build on it somewhat.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > It looks like we're building a general Rust lib crate and a static > library here, so symbol versioning isn't an issue. I expect in the > future we may want to provide a shared library, in which case we will > indeed want to do that, but I agree that can wait until later. > > In any event, I overall think this series is a nice improvement, and I > am very enthusiastic about it. (This is mostly for the benefit of > Junio, since I think the authors of this series already know that.) > Once it lands, I do plan to build on it somewhat. ;-) Thanks, I heard you.
Apologies for the long delay on V6; I am finally back after several months of $DAYJOB firefighting, holidays, and sick leave. I should have time to devote to this series again, but given the lack of feedback on V5 I am hopeful that this will be the final iteration of this series. This series provides two small Rust wrapper libraries around parts of Git: "libgit-sys", which exposes a few functions from libgit.a, and "libgit", which provides a more Rust-friendly interface to some of those functions. In addition to included unit tests, at $DAYJOB we have tested building JJ[1] with our library and used it to replace some of the libgit2-rs uses. [1] https://github.com/martinvonz/jj There is known NEEDSWORK, but I feel that they can be addressed in follow-up changes, rather than in this series. If you feel otherwise, please let me know: * Investigate alternative methods of managing symbol visibility & renaming. * Figure out symbol versioning Changes in V6: * Rebased onto current master, since V5 was several months old. * Move libgit-sys out of libgit-rs; while this sort of nesting is common in Rust crates with standalone repositories, it doesn't make as much sense when they're contained in the larger Git project's repo. * Standardize the naming of some of the Makefile targets to always include a dash in the "-rs" or "-sys" suffixes. * Clean up READMEs and crate descriptions in preparation for uploading to crates.io. Changes in V5: * When building with INCLUDE_LIBGIT_RS defined, add "-fvisibility=hidden" to CFLAGS. This allows us to manage symbol visibility in libgitpub.a without causing `make all` to rebuild from scratch due to changing CFLAGS. * Avoid using c_int in the higher-level Rust API. * Remove libgitpub.a and intermediate files with `make clean`. Changes in V4: * Drop V3 patch #3, which added wrappers around repository initialization and config access. These are not well-libified, and they are not necessary for JJ's proof-of-concept use case, so let's avoid exporting them for now. * Set a minimum supported Rust version of 1.63. Autodetect whether our Rust version has c_int and c_char types; if not, define them ourselves. * When building libgitpub.a via build.rs, set DEVELOPER=1 to catch additional errors at build time. * In build.rs, use the make_cmd crate to portable select the correct invocation of GNU Make. * Follow naming standards for _alloc() and _free() functions. * Use String instead of CString in higher-level API. * Move libgit_configset_alloc() and libgit_configset_free() out of upstream Git, to the libgitpub shim library. * In libgitpub, initialize libgit_config_set structs in the _alloc() function rather than with a separate _init() function. * Remove unnecessary comments in libgit-sys showing where the wrapped functions were originally defined. * Fix clippy lint: don't reborrow configfile path references. * Various typo fixes and `cargo fmt` fixes. Changes in V3: * Renamed cgit-rs to libgit-rs and cgit-sys to libgit-sys * Makefile cleanup, particularly adding config.mak options that developers can set to run Rust builds and tests by default (Patch 6) * Provide testdata configs for unit tests * ConfigSet API now uses &Path instead of &str -- more ergonomic for Rust users to pass in and errors out if the path string isn't UTF-8 * Fixed unresolved dependency on libz in Cargo.toml Calvin Wan (2): libgit: add higher-level libgit crate Makefile: add option to build and test libgit-rs and libgit-rs-sys Josh Steadmon (3): common-main: split init and exit code into new files libgit-sys: introduce Rust wrapper for libgit.a libgit-sys: also export some config_set functions .gitignore | 2 + Makefile | 44 +++++++++++ common-exit.c | 26 +++++++ common-init.c | 63 +++++++++++++++ common-init.h | 6 ++ common-main.c | 83 +------------------- contrib/libgit-rs/Cargo.lock | 77 ++++++++++++++++++ contrib/libgit-rs/Cargo.toml | 15 ++++ contrib/libgit-rs/README.md | 13 ++++ contrib/libgit-rs/build.rs | 4 + contrib/libgit-rs/src/lib.rs | 95 +++++++++++++++++++++++ contrib/libgit-rs/testdata/config1 | 2 + contrib/libgit-rs/testdata/config2 | 2 + contrib/libgit-rs/testdata/config3 | 2 + contrib/libgit-sys/Cargo.lock | 69 ++++++++++++++++ contrib/libgit-sys/Cargo.toml | 18 +++++ contrib/libgit-sys/README.md | 4 + contrib/libgit-sys/build.rs | 35 +++++++++ contrib/libgit-sys/public_symbol_export.c | 50 ++++++++++++ contrib/libgit-sys/public_symbol_export.h | 18 +++++ contrib/libgit-sys/src/lib.rs | 79 +++++++++++++++++++ t/Makefile | 16 ++++ 22 files changed, 642 insertions(+), 81 deletions(-) create mode 100644 common-exit.c create mode 100644 common-init.c create mode 100644 common-init.h create mode 100644 contrib/libgit-rs/Cargo.lock create mode 100644 contrib/libgit-rs/Cargo.toml create mode 100644 contrib/libgit-rs/README.md create mode 100644 contrib/libgit-rs/build.rs create mode 100644 contrib/libgit-rs/src/lib.rs create mode 100644 contrib/libgit-rs/testdata/config1 create mode 100644 contrib/libgit-rs/testdata/config2 create mode 100644 contrib/libgit-rs/testdata/config3 create mode 100644 contrib/libgit-sys/Cargo.lock create mode 100644 contrib/libgit-sys/Cargo.toml create mode 100644 contrib/libgit-sys/README.md create mode 100644 contrib/libgit-sys/build.rs create mode 100644 contrib/libgit-sys/public_symbol_export.c create mode 100644 contrib/libgit-sys/public_symbol_export.h create mode 100644 contrib/libgit-sys/src/lib.rs Range-diff against v5: 1: 1ae14207f6 = 1: ff6cd62397 common-main: split init and exit code into new files 2: 1ed010c378 ! 2: 5fc66cdb16 libgit-sys: introduce Rust wrapper for libgit.a @@ .gitignore: Release/ /git.VC.db *.dSYM /contrib/buildsystems/out -+/contrib/libgit-rs/libgit-sys/target ++/contrib/libgit-sys/target ## Makefile ## @@ Makefile: CURL_CONFIG = curl-config @@ Makefile: CURL_CONFIG = curl-config export TCL_PATH TCLTK_PATH -@@ Makefile: OBJECTS += $(XDIFF_OBJS) - OBJECTS += $(FUZZ_OBJS) - OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS) +@@ Makefile: OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS) OBJECTS += $(UNIT_TEST_OBJS) -+OBJECTS += contrib/libgit-rs/libgit-sys/public_symbol_export.o + 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 @@ Makefile: 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-rs/libgit-sys/target -+ $(RM) -r contrib/libgit-rs/libgit-sys/partial_symbol_export.o -+ $(RM) -r contrib/libgit-rs/libgit-sys/hidden_symbol_export.o -+ $(RM) -r contrib/libgit-rs/libgit-sys/libgitpub.a ++ $(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 -@@ Makefile: $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o \ - build-unit-tests: $(UNIT_TEST_PROGS) - unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X +@@ Makefile: $(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-rs/libgit-sys/partial_symbol_export.o: contrib/libgit-rs/libgit-sys/public_symbol_export.o libgit.a reftable/libreftable.a xdiff/lib.a ++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-rs/libgit-sys/hidden_symbol_export.o: contrib/libgit-rs/libgit-sys/partial_symbol_export.o ++contrib/libgit-sys/hidden_symbol_export.o: contrib/libgit-sys/partial_symbol_export.o + $(OBJCOPY) --localize-hidden $^ $@ + -+contrib/libgit-rs/libgit-sys/libgitpub.a: contrib/libgit-rs/libgit-sys/hidden_symbol_export.o ++contrib/libgit-sys/libgitpub.a: contrib/libgit-sys/hidden_symbol_export.o + $(AR) $(ARFLAGS) $@ $^ - ## contrib/libgit-rs/libgit-sys/Cargo.lock (new) ## + ## contrib/libgit-sys/Cargo.lock (new) ## @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. @@ contrib/libgit-rs/libgit-sys/Cargo.lock (new) +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "accd4ea62f7bb7a82fe23066fb0957d48ef677f6eeb8215f372f52e48bb32426" - ## contrib/libgit-rs/libgit-sys/Cargo.toml (new) ## + ## contrib/libgit-sys/Cargo.toml (new) ## @@ +[package] +name = "libgit-sys" @@ contrib/libgit-rs/libgit-sys/Cargo.toml (new) +build = "build.rs" +links = "gitpub" +rust-version = "1.63" ++description = "Native bindings to a portion of libgit" + +[lib] +path = "src/lib.rs" @@ contrib/libgit-rs/libgit-sys/Cargo.toml (new) +autocfg = "1.4.0" +make-cmd = "0.1.0" - ## contrib/libgit-rs/libgit-sys/README.md (new) ## + ## contrib/libgit-sys/README.md (new) ## @@ -+# cgit-info ++# libgit-sys + -+A small hacky proof-of-concept showing how to provide a Rust FFI for the Git -+library. -+ -+## Building -+ -+`cargo build` automatically builds and picks up on changes made to both -+the Rust wrapper and git.git code so there is no need to run `make` -+beforehand. -+ -+## Running -+ -+Assuming you don't make any changes to the Git source, you can just work from -+`contrib/cgit-rs` and use `cargo build` or `cargo run` as usual. ++A small proof-of-concept crate showing how to provide a Rust FFI to Git ++internals. - ## contrib/libgit-rs/libgit-sys/build.rs (new) ## + ## contrib/libgit-sys/build.rs (new) ## @@ +use std::env; +use std::path::PathBuf; @@ contrib/libgit-rs/libgit-sys/build.rs (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 git_root = crate_root.join("../.."); + let dst = PathBuf::from(env::var_os("OUT_DIR").unwrap()); + + let make_output = make_cmd::gnu_make() @@ contrib/libgit-rs/libgit-sys/build.rs (new) + .current_dir(git_root.clone()) + .args([ + "CFLAGS=-fvisibility=hidden", -+ "contrib/libgit-rs/libgit-sys/libgitpub.a", ++ "contrib/libgit-sys/libgitpub.a", + ]) + .output() + .expect("Make failed to run"); @@ contrib/libgit-rs/libgit-sys/build.rs (new) + Ok(()) +} - ## contrib/libgit-rs/libgit-sys/public_symbol_export.c (new) ## + ## contrib/libgit-sys/public_symbol_export.c (new) ## @@ +// 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-rs/libgit-sys/public_symbol_export.h" ++#include "contrib/libgit-sys/public_symbol_export.h" +#include "version.h" + +#pragma GCC visibility push(default) @@ contrib/libgit-rs/libgit-sys/public_symbol_export.c (new) + +#pragma GCC visibility pop - ## contrib/libgit-rs/libgit-sys/public_symbol_export.h (new) ## + ## contrib/libgit-sys/public_symbol_export.h (new) ## @@ +#ifndef PUBLIC_SYMBOL_EXPORT_H +#define PUBLIC_SYMBOL_EXPORT_H @@ contrib/libgit-rs/libgit-sys/public_symbol_export.h (new) + +#endif /* PUBLIC_SYMBOL_EXPORT_H */ - ## contrib/libgit-rs/libgit-sys/src/lib.rs (new) ## + ## contrib/libgit-sys/src/lib.rs (new) ## @@ +#[cfg(has_std__ffi__c_char)] +use std::ffi::c_char; 3: 00762b57d0 ! 3: 03f39b6c3a libgit-sys: also export some config_set functions @@ Commit message Signed-off-by: Calvin Wan <calvinwan@google.com> Signed-off-by: Josh Steadmon <steadmon@google.com> - ## contrib/libgit-rs/libgit-sys/public_symbol_export.c ## + ## contrib/libgit-sys/public_symbol_export.c ## @@ // avoids conflicts with other libraries such as libgit2. #include "git-compat-util.h" +#include "config.h" - #include "contrib/libgit-rs/libgit-sys/public_symbol_export.h" + #include "contrib/libgit-sys/public_symbol_export.h" #include "version.h" #pragma GCC visibility push(default) @@ contrib/libgit-rs/libgit-sys/public_symbol_export.c { return git_user_agent(); - ## contrib/libgit-rs/libgit-sys/public_symbol_export.h ## + ## contrib/libgit-sys/public_symbol_export.h ## @@ #ifndef PUBLIC_SYMBOL_EXPORT_H #define PUBLIC_SYMBOL_EXPORT_H @@ contrib/libgit-rs/libgit-sys/public_symbol_export.h const char *libgit_user_agent_sanitized(void); - ## contrib/libgit-rs/libgit-sys/src/lib.rs ## + ## contrib/libgit-sys/src/lib.rs ## @@ #[cfg(has_std__ffi__c_char)] -use std::ffi::c_char; 4: 4e5360931b ! 4: 65166ea0c0 libgit: add higher-level libgit crate @@ .gitignore: Release/ *.dSYM /contrib/buildsystems/out +/contrib/libgit-rs/target - /contrib/libgit-rs/libgit-sys/target + /contrib/libgit-sys/target ## Makefile ## @@ Makefile: 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-rs/libgit-sys/target -+ $(RM) -r contrib/libgit-rs/target contrib/libgit-rs/libgit-sys/target - $(RM) -r contrib/libgit-rs/libgit-sys/partial_symbol_export.o - $(RM) -r contrib/libgit-rs/libgit-sys/hidden_symbol_export.o - $(RM) -r contrib/libgit-rs/libgit-sys/libgitpub.a +- $(RM) -r contrib/libgit-sys/target ++ $(RM) -r contrib/libgit-rs/target 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 ## contrib/libgit-rs/Cargo.lock (new) ## @@ @@ contrib/libgit-rs/Cargo.toml (new) +path = "src/lib.rs" + +[dependencies] -+libgit-sys = { version = "0.1.0", path = "libgit-sys" } ++libgit-sys = { version = "0.1.0", path = "../libgit-sys" } + +[build-dependencies] +autocfg = "1.4.0" + ## contrib/libgit-rs/README.md (new) ## +@@ ++# libgit-rs ++ ++Proof-of-concept Git bindings for Rust. ++ ++```toml ++[dependencies] ++libgit = "0.1.0" ++``` ++ ++## Rust version requirements ++ ++libgit-rs should support Rust versions at least as old as the version included ++in Debian stable (currently 1.63). + ## contrib/libgit-rs/build.rs (new) ## @@ +pub fn main() { @@ contrib/libgit-rs/build.rs (new) + ac.emit_has_path("std::ffi::c_char"); +} - ## contrib/libgit-rs/libgit-sys/src/lib.rs ## -@@ -+use std::ffi::c_void; -+ - #[cfg(has_std__ffi__c_char)] - use std::ffi::{c_char, c_int}; - -@@ contrib/libgit-rs/libgit-sys/src/lib.rs: pub struct libgit_config_set { - } - - extern "C" { -+ pub fn free(ptr: *mut c_void); -+ - pub fn libgit_user_agent() -> *const c_char; - pub fn libgit_user_agent_sanitized() -> *const c_char; - - ## contrib/libgit-rs/src/lib.rs (new) ## @@ +use std::ffi::{c_void, CStr, CString}; @@ contrib/libgit-rs/testdata/config3 (new) @@ +[trace2] + eventNesting = 3 + + ## contrib/libgit-sys/src/lib.rs ## +@@ ++use std::ffi::c_void; ++ + #[cfg(has_std__ffi__c_char)] + use std::ffi::{c_char, c_int}; + +@@ contrib/libgit-sys/src/lib.rs: pub struct libgit_config_set { + } + + extern "C" { ++ pub fn free(ptr: *mut c_void); ++ + pub fn libgit_user_agent() -> *const c_char; + pub fn libgit_user_agent_sanitized() -> *const c_char; + 5: 15ce989de8 ! 5: 84706f0db7 Makefile: add option to build and test libgit-rs and libgit-rs-sys @@ Makefile: ifdef FSMONITOR_OS_SETTINGS ifeq ($(TCLTK_PATH),) NO_TCLTK = NoThanks endif -@@ Makefile: build-unit-tests: $(UNIT_TEST_PROGS) - unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X +@@ Makefile: 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 -+.PHONY: libgitrs-sys -+libgitrs-sys: ++.PHONY: libgit-sys ++libgit-sys: + $(QUIET)(\ -+ cd contrib/libgit-rs/libgit-sys && \ ++ cd contrib/libgit-sys && \ + cargo build \ + ) -+.PHONY: libgitrs -+libgitrs: ++.PHONY: libgit-rs ++libgit-rs: + $(QUIET)(\ + cd contrib/libgit-rs && \ + cargo build \ + ) +ifdef INCLUDE_LIBGIT_RS -+all:: libgitrs ++all:: libgit-rs +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 + 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-rs/libgit-sys/build.rs ## -@@ contrib/libgit-rs/libgit-sys/build.rs: pub fn main() -> std::io::Result<()> { + ## contrib/libgit-sys/build.rs ## +@@ contrib/libgit-sys/build.rs: pub fn main() -> std::io::Result<()> { .env_remove("PROFILE") .current_dir(git_root.clone()) .args([ - "CFLAGS=-fvisibility=hidden", + "INCLUDE_LIBGIT_RS=YesPlease", - "contrib/libgit-rs/libgit-sys/libgitpub.a", + "contrib/libgit-sys/libgitpub.a", ]) .output() @@ t/Makefile: perf: .PHONY: pre-clean $(T) aggregate-results clean valgrind perf \ check-chainlint clean-chainlint test-chainlint $(UNIT_TESTS) + -+.PHONY: libgitrs-sys-test -+libgitrs-sys-test: ++.PHONY: libgit-sys-test ++libgit-sys-test: + $(QUIET)(\ -+ cd ../contrib/libgit-rs/libgit-sys && \ ++ cd ../contrib/libgit-sys && \ + cargo test \ + ) -+.PHONY: libgitrs-test -+libgitrs-test: ++.PHONY: libgit-rs-test ++libgit-rs-test: + $(QUIET)(\ + cd ../contrib/libgit-rs && \ + cargo test \ + ) +ifdef INCLUDE_LIBGIT_RS -+all:: libgitrs-sys-test libgitrs-test ++all:: libgit-sys-test libgit-rs-test +endif base-commit: 757161efcca150a9a96b312d9e780a071e601a03