| Message ID | 65166ea0c077665c350a1a7b00dc3175be889d55.1736971328.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce libgit-rs, a Rust wrapper around libgit.a | expand |
On 2025-01-15 at 20:05:43, Josh Steadmon wrote: > From: Calvin Wan <calvinwan@google.com> > diff --git a/contrib/libgit-rs/Cargo.lock b/contrib/libgit-rs/Cargo.lock > new file mode 100644 > index 0000000000..a30c7c8d33 > --- /dev/null > +++ b/contrib/libgit-rs/Cargo.lock > @@ -0,0 +1,77 @@ > +# 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" > +version = "0.1.0" > +dependencies = [ > + "autocfg", > + "libgit-sys", > +] > + > +[[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" There are two possibilities here. The first is to check in the Cargo.lock, in which case all users will have to use these versions. That produces a more stable and reliable approach, but it has some downsides. Say, for instance, that a cool new platform or architecture is added to libc and we'd like to support it, but that version of libc requires a newer version of Rust. We then would have to hold off supporting that new platform due to compatibility reasons. But if we omitted the Cargo.lock, users could install any version that meets their needs. I believe Rust just got the ability to install only versions that honour the rust-version directive in 1.84, whereas older versions will try to use the latest version, even if that fails. So I think it's okay for now to use Cargo.lock, because that means that things will be better out of the box for users on older Rust. But we may want to drop it once 1.84 is older than our supported version. > diff --git a/contrib/libgit-rs/src/lib.rs b/contrib/libgit-rs/src/lib.rs > new file mode 100644 > index 0000000000..27b6fd63f1 > --- /dev/null > +++ b/contrib/libgit-rs/src/lib.rs > @@ -0,0 +1,95 @@ > +use std::ffi::{c_void, CStr, CString}; > +use std::path::Path; > + > +#[cfg(has_std__ffi__c_char)] > +use std::ffi::{c_char, c_int}; > + > +#[cfg(not(has_std__ffi__c_char))] > +#[allow(non_camel_case_types)] > +pub type c_char = i8; > + > +#[cfg(not(has_std__ffi__c_char))] > +#[allow(non_camel_case_types)] > +pub type c_int = i32; By making these `pub`, you're exporting them. We probably do not want to do that, since they are not part of our API. If we need them more generally in the code, let's put them in a module called `ffi` or such that's `pub(crate)`, and then use them from there. > +use libgit_sys::*; > + > +pub struct ConfigSet(*mut libgit_config_set); > +impl ConfigSet { I would suggest we place these in a module, such as `config`. We should expect to have a lot more things in our crate in the future and putting a little thought into this now will make it easier for users in the future. I'd also, of course, suggest documentation comments, since if we ever upload this to crates.io, people are overwhelmingly going to read only the docs and not the source, and right now we've said nothing about how this works or should work. > +#[cfg(test)] > +mod tests { > + use super::*; > + > + #[test] > + fn load_configs_via_configset() { > + let mut cs = ConfigSet::new(); > + cs.add_files(&[ > + Path::new("testdata/config1"), > + Path::new("testdata/config2"), > + Path::new("testdata/config3"), > + ]); > + // ConfigSet retrieves correct value > + assert_eq!(cs.get_int("trace2.eventTarget"), Some(1)); > + // ConfigSet respects last config value set > + assert_eq!(cs.get_int("trace2.eventNesting"), Some(3)); > + // ConfigSet returns None for missing key > + assert_eq!(cs.get_string("foo.bar"), None); > + } > +} I am, of course, delighted to see tests. This is a nice improvement in our code that we can take advantage of, and we can test both the C code and Rust code at the same time. And if, in the future, we decide that we'd like to implement a Rust-based version of this API to replace the C one, we've already written tests.
On 2025.01.21 00:00, brian m. carlson wrote: > On 2025-01-15 at 20:05:43, Josh Steadmon wrote: > > From: Calvin Wan <calvinwan@google.com> > > diff --git a/contrib/libgit-rs/Cargo.lock b/contrib/libgit-rs/Cargo.lock > > new file mode 100644 > > index 0000000000..a30c7c8d33 > > --- /dev/null > > +++ b/contrib/libgit-rs/Cargo.lock > > @@ -0,0 +1,77 @@ > > +# 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" > > +version = "0.1.0" > > +dependencies = [ > > + "autocfg", > > + "libgit-sys", > > +] > > + > > +[[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" > > There are two possibilities here. The first is to check in the > Cargo.lock, in which case all users will have to use these versions. > That produces a more stable and reliable approach, but it has some > downsides. > > Say, for instance, that a cool new platform or architecture is added to > libc and we'd like to support it, but that version of libc requires a > newer version of Rust. We then would have to hold off supporting that > new platform due to compatibility reasons. But if we omitted the > Cargo.lock, users could install any version that meets their needs. > > I believe Rust just got the ability to install only versions that honour > the rust-version directive in 1.84, whereas older versions will try to > use the latest version, even if that fails. So I think it's okay for > now to use Cargo.lock, because that means that things will be better out > of the box for users on older Rust. But we may want to drop it once > 1.84 is older than our supported version. OK, I'll add a TODO comment in Cargo.toml. > > diff --git a/contrib/libgit-rs/src/lib.rs b/contrib/libgit-rs/src/lib.rs > > new file mode 100644 > > index 0000000000..27b6fd63f1 > > --- /dev/null > > +++ b/contrib/libgit-rs/src/lib.rs > > @@ -0,0 +1,95 @@ > > +use std::ffi::{c_void, CStr, CString}; > > +use std::path::Path; > > + > > +#[cfg(has_std__ffi__c_char)] > > +use std::ffi::{c_char, c_int}; > > + > > +#[cfg(not(has_std__ffi__c_char))] > > +#[allow(non_camel_case_types)] > > +pub type c_char = i8; > > + > > +#[cfg(not(has_std__ffi__c_char))] > > +#[allow(non_camel_case_types)] > > +pub type c_int = i32; > > By making these `pub`, you're exporting them. We probably do not want > to do that, since they are not part of our API. Fixed for V7. > If we need them more generally in the code, let's put them in a module > called `ffi` or such that's `pub(crate)`, and then use them from there. Hmm, I guess we'd need to do that for libgit-sys as well? Or maybe not, since they're part of the API and thus we should just keep the current `pub type ...` setup? > > +use libgit_sys::*; > > + > > +pub struct ConfigSet(*mut libgit_config_set); > > +impl ConfigSet { > > I would suggest we place these in a module, such as `config`. We should > expect to have a lot more things in our crate in the future and putting > a little thought into this now will make it easier for users in the > future. ACK, will do for V7. > I'd also, of course, suggest documentation comments, since if we ever > upload this to crates.io, people are overwhelmingly going to read only > the docs and not the source, and right now we've said nothing about how > this works or should work. ACK. > > +#[cfg(test)] > > +mod tests { > > + use super::*; > > + > > + #[test] > > + fn load_configs_via_configset() { > > + let mut cs = ConfigSet::new(); > > + cs.add_files(&[ > > + Path::new("testdata/config1"), > > + Path::new("testdata/config2"), > > + Path::new("testdata/config3"), > > + ]); > > + // ConfigSet retrieves correct value > > + assert_eq!(cs.get_int("trace2.eventTarget"), Some(1)); > > + // ConfigSet respects last config value set > > + assert_eq!(cs.get_int("trace2.eventNesting"), Some(3)); > > + // ConfigSet returns None for missing key > > + assert_eq!(cs.get_string("foo.bar"), None); > > + } > > +} > > I am, of course, delighted to see tests. This is a nice improvement in > our code that we can take advantage of, and we can test both the C code > and Rust code at the same time. And if, in the future, we decide that > we'd like to implement a Rust-based version of this API to replace the C > one, we've already written tests. Thanks for the review and advice (both for this round and all the previous ones), it's much appreciated. > -- > brian m. carlson (they/them or he/him) > Toronto, Ontario, CA
diff --git a/.gitignore b/.gitignore index 31d7e64287..acdd8ce7c7 100644 --- a/.gitignore +++ b/.gitignore @@ -250,4 +250,5 @@ Release/ /git.VC.db *.dSYM /contrib/buildsystems/out +/contrib/libgit-rs/target /contrib/libgit-sys/target diff --git a/Makefile b/Makefile index 47e864a861..230d366457 100644 --- a/Makefile +++ b/Makefile @@ -3729,7 +3729,7 @@ 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-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 diff --git a/contrib/libgit-rs/Cargo.lock b/contrib/libgit-rs/Cargo.lock new file mode 100644 index 0000000000..a30c7c8d33 --- /dev/null +++ b/contrib/libgit-rs/Cargo.lock @@ -0,0 +1,77 @@ +# 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" +version = "0.1.0" +dependencies = [ + "autocfg", + "libgit-sys", +] + +[[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-rs/Cargo.toml b/contrib/libgit-rs/Cargo.toml new file mode 100644 index 0000000000..634435cd6c --- /dev/null +++ b/contrib/libgit-rs/Cargo.toml @@ -0,0 +1,15 @@ +[package] +name = "libgit" +version = "0.1.0" +edition = "2021" +build = "build.rs" +rust-version = "1.63" + +[lib] +path = "src/lib.rs" + +[dependencies] +libgit-sys = { version = "0.1.0", path = "../libgit-sys" } + +[build-dependencies] +autocfg = "1.4.0" diff --git a/contrib/libgit-rs/README.md b/contrib/libgit-rs/README.md new file mode 100644 index 0000000000..ff945e1ce2 --- /dev/null +++ b/contrib/libgit-rs/README.md @@ -0,0 +1,13 @@ +# 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). diff --git a/contrib/libgit-rs/build.rs b/contrib/libgit-rs/build.rs new file mode 100644 index 0000000000..f8bd01a690 --- /dev/null +++ b/contrib/libgit-rs/build.rs @@ -0,0 +1,4 @@ +pub fn main() { + let ac = autocfg::new(); + ac.emit_has_path("std::ffi::c_char"); +} diff --git a/contrib/libgit-rs/src/lib.rs b/contrib/libgit-rs/src/lib.rs new file mode 100644 index 0000000000..27b6fd63f1 --- /dev/null +++ b/contrib/libgit-rs/src/lib.rs @@ -0,0 +1,95 @@ +use std::ffi::{c_void, CStr, CString}; +use std::path::Path; + +#[cfg(has_std__ffi__c_char)] +use std::ffi::{c_char, c_int}; + +#[cfg(not(has_std__ffi__c_char))] +#[allow(non_camel_case_types)] +pub type c_char = i8; + +#[cfg(not(has_std__ffi__c_char))] +#[allow(non_camel_case_types)] +pub type c_int = i32; + +use libgit_sys::*; + +pub struct ConfigSet(*mut libgit_config_set); +impl ConfigSet { + pub fn new() -> Self { + unsafe { ConfigSet(libgit_configset_alloc()) } + } + + pub fn add_files(&mut self, files: &[&Path]) { + for file in files { + let pstr = file.to_str().expect("Invalid UTF-8"); + let rs = CString::new(pstr).expect("Couldn't convert to CString"); + unsafe { + libgit_configset_add_file(self.0, rs.as_ptr()); + } + } + } + + pub fn get_int(&mut self, key: &str) -> Option<i32> { + let key = CString::new(key).expect("Couldn't convert to CString"); + let mut val: c_int = 0; + unsafe { + if libgit_configset_get_int(self.0, key.as_ptr(), &mut val as *mut c_int) != 0 { + return None; + } + } + + Some(val.into()) + } + + pub fn get_string(&mut self, key: &str) -> Option<String> { + let key = CString::new(key).expect("Couldn't convert key to CString"); + let mut val: *mut c_char = std::ptr::null_mut(); + unsafe { + if libgit_configset_get_string(self.0, key.as_ptr(), &mut val as *mut *mut c_char) != 0 + { + return None; + } + let borrowed_str = CStr::from_ptr(val); + let owned_str = + String::from(borrowed_str.to_str().expect("Couldn't convert val to str")); + free(val as *mut c_void); // Free the xstrdup()ed pointer from the C side + Some(owned_str) + } + } +} + +impl Default for ConfigSet { + fn default() -> Self { + Self::new() + } +} + +impl Drop for ConfigSet { + fn drop(&mut self) { + unsafe { + libgit_configset_free(self.0); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn load_configs_via_configset() { + let mut cs = ConfigSet::new(); + cs.add_files(&[ + Path::new("testdata/config1"), + Path::new("testdata/config2"), + Path::new("testdata/config3"), + ]); + // ConfigSet retrieves correct value + assert_eq!(cs.get_int("trace2.eventTarget"), Some(1)); + // ConfigSet respects last config value set + assert_eq!(cs.get_int("trace2.eventNesting"), Some(3)); + // ConfigSet returns None for missing key + assert_eq!(cs.get_string("foo.bar"), None); + } +} diff --git a/contrib/libgit-rs/testdata/config1 b/contrib/libgit-rs/testdata/config1 new file mode 100644 index 0000000000..4e9a9d25d1 --- /dev/null +++ b/contrib/libgit-rs/testdata/config1 @@ -0,0 +1,2 @@ +[trace2] + eventNesting = 1 diff --git a/contrib/libgit-rs/testdata/config2 b/contrib/libgit-rs/testdata/config2 new file mode 100644 index 0000000000..b8d1eca423 --- /dev/null +++ b/contrib/libgit-rs/testdata/config2 @@ -0,0 +1,2 @@ +[trace2] + eventTarget = 1 diff --git a/contrib/libgit-rs/testdata/config3 b/contrib/libgit-rs/testdata/config3 new file mode 100644 index 0000000000..ca7b9a7c38 --- /dev/null +++ b/contrib/libgit-rs/testdata/config3 @@ -0,0 +1,2 @@ +[trace2] + eventNesting = 3 diff --git a/contrib/libgit-sys/src/lib.rs b/contrib/libgit-sys/src/lib.rs index dadb4e5f40..4bfc650450 100644 --- a/contrib/libgit-sys/src/lib.rs +++ b/contrib/libgit-sys/src/lib.rs @@ -1,3 +1,5 @@ +use std::ffi::c_void; + #[cfg(has_std__ffi__c_char)] use std::ffi::{c_char, c_int}; @@ -19,6 +21,8 @@ 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;