Re: [PATCH v3 2/6] libgit-sys: introduce Rust wrapper for libgit.a
On 2024.09.06 18:39, Eric Sunshine wrote:
Show 22 quoted lines
> On Fri, Sep 6, 2024 at 6:21 PM Calvin Wan <calvinwan@google.com> wrote:
> > From: Josh Steadmon <steadmon@google.com>
>
> This is curious...
>
> > Co-authored-by: Kyle Lippincott <spectral@google.com>
> > Co-authored-by: Calvin Wan <calvinwan@google.com>
> > Signed-off-by: Calvin Wan <calvinwan@google.com>
> > Signed-off-by: Kyle Lippincott <spectral@google.com>
>
> ... since there is no mention of Josh here.
>
> > diff --git a/contrib/libgit-rs/libgit-sys/Cargo.toml b/contrib/libgit-rs/libgit-sys/Cargo.toml
> > @@ -0,0 +1,12 @@
> > +[package]
> > +name = "libgit-sys"
> > [...]
> > +[dependencies]
> > +libz-sys = "1.1.19"
> > \ No newline at end of file
>
> Let's give this file a proper line terminator.
Show 13 quoted lines
> > diff --git a/contrib/libgit-rs/libgit-sys/build.rs b/contrib/libgit-rs/libgit-sys/build.rs
> > @@ -0,0 +1,31 @@
> > +pub fn main() -> std::io::Result<()> {
> > + 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 = std::process::Command::new("make")
>
> Providing a mechanism for people to override this hardcoded spelling
> of "make" could be another item for your NEEDSWORK list; in
> particular, I'm thinking about platforms on which GNU "make" is
> installed as "gmake".Ack, will use make_cmd as Mike suggests.
Show 7 quoted lines
> > diff --git a/contrib/libgit-rs/libgit-sys/public_symbol_export.c b/contrib/libgit-rs/libgit-sys/public_symbol_export.c
> > @@ -0,0 +1,20 @@
> > +// 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
> > +// avoid conflicts with other libraries such as libgit2.
>
> s/avoid/avoids/
Fixed in V4.
Thanks for the review!