| Message ID | b84a8210a05c2358dc29f24a56adcbeaa90c8654.1723054623.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce cgit-rs, a Rust wrapper around libgit.a | expand |
On 2024-08-07 at 18:21:29, Josh Steadmon wrote: > diff --git a/contrib/cgit-rs/src/lib.rs b/contrib/cgit-rs/src/lib.rs > index dc46e7ff42..df350e758f 100644 > --- a/contrib/cgit-rs/src/lib.rs > +++ b/contrib/cgit-rs/src/lib.rs > @@ -1,6 +1,17 @@ > -use libc::c_char; > +use libc::{c_char, c_int}; > > extern "C" { > + pub fn libgit_setup_git_directory() -> *const c_char; > + > + // From config.c > + pub fn libgit_config_get_int(key: *const c_char, dest: *mut c_int) ->c_int; > + > + // From repository.c > + pub fn libgit_initialize_the_repository(); > + > + // From parse.c > + pub fn libgit_parse_maybe_bool(val: *const c_char) -> c_int; > + > // From version.c > pub fn libgit_user_agent() -> *const c_char; > pub fn libgit_user_agent_sanitized() -> *const c_char; I think it would be helpful if we didn't expose the raw C API in Rust. Nobody is going to want to write code that uses that in Rust. If we _do_ expose that, it should be in a separate cgit-sys crate, which is the customary naming, that exposes only that and then we can offer nicer wrappers around it. > diff --git a/contrib/cgit-rs/src/main.rs b/contrib/cgit-rs/src/main.rs > index 1794e3f43e..c5f8644fca 100644 > --- a/contrib/cgit-rs/src/main.rs > +++ b/contrib/cgit-rs/src/main.rs > @@ -1,4 +1,4 @@ > -use std::ffi::CStr; > +use std::ffi::{CStr, CString}; > > fn main() { > println!("Let's print some strings provided by Git"); > @@ -7,4 +7,25 @@ fn main() { > println!("git_user_agent() = {:?}", c_str); > println!("git_user_agent_sanitized() = {:?}", > unsafe { CStr::from_ptr(cgit::libgit_user_agent_sanitized()) }); > + > + println!("\nNow try passing args"); > + let test_arg = CString::new("test_arg").unwrap(); > + println!("git_parse_maybe_bool(...) = {:?}", > + unsafe { cgit::libgit_parse_maybe_bool(test_arg.as_ptr()) }); > + > + println!("\nCan we get an int out of our config??"); > + unsafe { > + cgit::libgit_initialize_the_repository(); > + cgit::libgit_setup_git_directory(); > + let mut val: libc::c_int = 0; > + let key = CString::new("trace2.eventNesting").unwrap(); > + cgit::libgit_config_get_int( > + key.as_ptr(), > + &mut val as *mut i32 > + ); > + println!( > + "git_config_get_int(\"trace2.eventNesting\") -> {:?}", > + val > + ); > + }; > } It seems like this code wants to be a set of tests and maybe it would be helpful to write it as a few instead. For example, we can assume that our user-agent starts with `git/` assuming it hasn't been overridden, so maybe we could write that as a test, or at least that we got a valid C string.
On 2024.08.07 21:26, brian m. carlson wrote: > On 2024-08-07 at 18:21:29, Josh Steadmon wrote: > > diff --git a/contrib/cgit-rs/src/lib.rs b/contrib/cgit-rs/src/lib.rs > > index dc46e7ff42..df350e758f 100644 > > --- a/contrib/cgit-rs/src/lib.rs > > +++ b/contrib/cgit-rs/src/lib.rs > > @@ -1,6 +1,17 @@ > > -use libc::c_char; > > +use libc::{c_char, c_int}; > > > > extern "C" { > > + pub fn libgit_setup_git_directory() -> *const c_char; > > + > > + // From config.c > > + pub fn libgit_config_get_int(key: *const c_char, dest: *mut c_int) ->c_int; > > + > > + // From repository.c > > + pub fn libgit_initialize_the_repository(); > > + > > + // From parse.c > > + pub fn libgit_parse_maybe_bool(val: *const c_char) -> c_int; > > + > > // From version.c > > pub fn libgit_user_agent() -> *const c_char; > > pub fn libgit_user_agent_sanitized() -> *const c_char; > > I think it would be helpful if we didn't expose the raw C API in Rust. > Nobody is going to want to write code that uses that in Rust. > > If we _do_ expose that, it should be in a separate cgit-sys crate, which > is the customary naming, that exposes only that and then we can offer > nicer wrappers around it. Yeah, that's a known NEEDSWORK that I forgot to mention in the cover letter. I'm also going to get in touch soon with the gitoxide & libgit2-rs folks to see if there's any joint work that we can do in terms of defining nicer Rust APIs. Semi-relatedly, I was wondering if you might be able to answer a cargo / crates.io question: for a crate such as cgit-rs, which is not located in the root of its VCS repo, will cargo balk at downloading the full git.git worktree? Our build.rs expects the full Git source to be available at "../.." relative to the crate root. We've currently only tested consuming cgit-rs in JJ via a local path rather than through crates.io. libgit2-rs avoids this issue by including the C source of libgit2 as a submodule, but I'd prefer for cgit-rs to live in the git.git repo if at all possible. > > diff --git a/contrib/cgit-rs/src/main.rs b/contrib/cgit-rs/src/main.rs > > index 1794e3f43e..c5f8644fca 100644 > > --- a/contrib/cgit-rs/src/main.rs > > +++ b/contrib/cgit-rs/src/main.rs > > @@ -1,4 +1,4 @@ > > -use std::ffi::CStr; > > +use std::ffi::{CStr, CString}; > > > > fn main() { > > println!("Let's print some strings provided by Git"); > > @@ -7,4 +7,25 @@ fn main() { > > println!("git_user_agent() = {:?}", c_str); > > println!("git_user_agent_sanitized() = {:?}", > > unsafe { CStr::from_ptr(cgit::libgit_user_agent_sanitized()) }); > > + > > + println!("\nNow try passing args"); > > + let test_arg = CString::new("test_arg").unwrap(); > > + println!("git_parse_maybe_bool(...) = {:?}", > > + unsafe { cgit::libgit_parse_maybe_bool(test_arg.as_ptr()) }); > > + > > + println!("\nCan we get an int out of our config??"); > > + unsafe { > > + cgit::libgit_initialize_the_repository(); > > + cgit::libgit_setup_git_directory(); > > + let mut val: libc::c_int = 0; > > + let key = CString::new("trace2.eventNesting").unwrap(); > > + cgit::libgit_config_get_int( > > + key.as_ptr(), > > + &mut val as *mut i32 > > + ); > > + println!( > > + "git_config_get_int(\"trace2.eventNesting\") -> {:?}", > > + val > > + ); > > + }; > > } > > It seems like this code wants to be a set of tests and maybe it would > be helpful to write it as a few instead. For example, we can assume > that our user-agent starts with `git/` assuming it hasn't been > overridden, so maybe we could write that as a test, or at least that we > got a valid C string. Agreed. > -- > brian m. carlson (they/them or he/him) > Toronto, Ontario, CA
On 2024-08-07 at 23:14:43, Josh Steadmon wrote: > Semi-relatedly, I was wondering if you might be able to answer a cargo / > crates.io question: for a crate such as cgit-rs, which is not located in > the root of its VCS repo, will cargo balk at downloading the full > git.git worktree? Our build.rs expects the full Git source to be > available at "../.." relative to the crate root. We've currently only > tested consuming cgit-rs in JJ via a local path rather than through > crates.io. The documentation[0] says this: Cargo fetches the git repository at that location and traverses the file tree to find Cargo.toml file for the requested crate anywhere inside the git repository. For example, regex-lite and regex-syntax are members of rust-lang/regex repo and can be referred to by the repo’s root URL (https://github.com/rust-lang/regex.git) regardless of where in the file tree they reside. If you want to test, you should be able to push it to a repository somewhere, even if that's a local path, and specify like so to test: ---- [dependencies] cgit-rs = { version = "0.1.0", git = "file:///path/to/git.git", branch = "cgit-rs" } ---- and see if it finds it. [0] https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html
diff --git a/contrib/cgit-rs/public_symbol_export.c b/contrib/cgit-rs/public_symbol_export.c index 3d1cd6cc4f..ab3401ac40 100644 --- a/contrib/cgit-rs/public_symbol_export.c +++ b/contrib/cgit-rs/public_symbol_export.c @@ -2,11 +2,35 @@ // original symbols can be hidden. Renaming these with a "libgit_" prefix also // avoid conflicts with other libraries such as libgit2. +#include "git-compat-util.h" #include "contrib/cgit-rs/public_symbol_export.h" +#include "attr.h" +#include "config.h" +#include "setup.h" #include "version.h" #pragma GCC visibility push(default) +const char *libgit_setup_git_directory(void) +{ + return setup_git_directory(); +} + +int libgit_config_get_int(const char *key, int *dest) +{ + return git_config_get_int(key, dest); +} + +void libgit_initialize_the_repository(void) +{ + initialize_the_repository(); +} + +int libgit_parse_maybe_bool(const char *val) +{ + return git_parse_maybe_bool(val); +} + const char *libgit_user_agent(void) { return git_user_agent(); @@ -17,4 +41,7 @@ const char *libgit_user_agent_sanitized(void) return git_user_agent_sanitized(); } +const char *libgit_attr__true = git_attr__true; +const char *libgit_attr__false = git_attr__false; + #pragma GCC visibility pop diff --git a/contrib/cgit-rs/public_symbol_export.h b/contrib/cgit-rs/public_symbol_export.h index a3372f93fa..97e8e871ba 100644 --- a/contrib/cgit-rs/public_symbol_export.h +++ b/contrib/cgit-rs/public_symbol_export.h @@ -1,6 +1,14 @@ #ifndef PUBLIC_SYMBOL_EXPORT_H #define PUBLIC_SYMBOL_EXPORT_H +const char *libgit_setup_git_directory(void); + +int libgit_config_get_int(const char *key, int *dest); + +void libgit_initialize_the_repository(void); + +int libgit_parse_maybe_bool(const char *val); + const char *libgit_user_agent(void); const char *libgit_user_agent_sanitized(void); diff --git a/contrib/cgit-rs/src/lib.rs b/contrib/cgit-rs/src/lib.rs index dc46e7ff42..df350e758f 100644 --- a/contrib/cgit-rs/src/lib.rs +++ b/contrib/cgit-rs/src/lib.rs @@ -1,6 +1,17 @@ -use libc::c_char; +use libc::{c_char, c_int}; extern "C" { + pub fn libgit_setup_git_directory() -> *const c_char; + + // From config.c + pub fn libgit_config_get_int(key: *const c_char, dest: *mut c_int) ->c_int; + + // From repository.c + pub fn libgit_initialize_the_repository(); + + // From parse.c + pub fn libgit_parse_maybe_bool(val: *const c_char) -> c_int; + // From version.c pub fn libgit_user_agent() -> *const c_char; pub fn libgit_user_agent_sanitized() -> *const c_char; diff --git a/contrib/cgit-rs/src/main.rs b/contrib/cgit-rs/src/main.rs index 1794e3f43e..c5f8644fca 100644 --- a/contrib/cgit-rs/src/main.rs +++ b/contrib/cgit-rs/src/main.rs @@ -1,4 +1,4 @@ -use std::ffi::CStr; +use std::ffi::{CStr, CString}; fn main() { println!("Let's print some strings provided by Git"); @@ -7,4 +7,25 @@ fn main() { println!("git_user_agent() = {:?}", c_str); println!("git_user_agent_sanitized() = {:?}", unsafe { CStr::from_ptr(cgit::libgit_user_agent_sanitized()) }); + + println!("\nNow try passing args"); + let test_arg = CString::new("test_arg").unwrap(); + println!("git_parse_maybe_bool(...) = {:?}", + unsafe { cgit::libgit_parse_maybe_bool(test_arg.as_ptr()) }); + + println!("\nCan we get an int out of our config??"); + unsafe { + cgit::libgit_initialize_the_repository(); + cgit::libgit_setup_git_directory(); + let mut val: libc::c_int = 0; + let key = CString::new("trace2.eventNesting").unwrap(); + cgit::libgit_config_get_int( + key.as_ptr(), + &mut val as *mut i32 + ); + println!( + "git_config_get_int(\"trace2.eventNesting\") -> {:?}", + val + ); + }; }