From: Josh Steadmon <steadmon@google.com>
Date: Wed, 18 Sep 2024 21:14:30 GMT
Subject: Re: [PATCH v3 2/6] libgit-sys: introduce Rust wrapper for libgit.a
Message-ID: <e5bl6h4jpw76tgr2eobqhyoq7nozkbz3p7u5ljfrwlgir7ycdu@ppxdg2tdmgly>
In-Reply-To: <CAPig+cQbPCWS4LVF7cdw_DD6K8dsrLTCmmhqkmGZpz1CO93vRQ@mail.gmail.com>

On 2024.09.06 18:39, Eric Sunshine wrote:
> 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.

Fixed in V4.

> > 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.

> > 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!