Skip to content

Add a LIBGIT2_NO_VENDOR environment variable to build.rs #939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

kpcyrd
Copy link
Contributor

@kpcyrd kpcyrd commented Mar 22, 2023

This does the following things:

  • To test for cargo features you'd usually use if cfg!(feature = "zlib-ng-compat") { ... } instead of accessing the environment variable, but since this environment variable could also be set from outside of cargo I'm generating cargo:rerun-if-env-changed= output to let cargo know it needs to rebuild if they're changed
  • I'm adding a LIBGIT2_NO_VENDOR environment variable, this variable is supposed to deny vendoring even if the cargo feature libgit2-sys/vendored is enabled by any crate. If the system libgit2 is not present or not compatible it errors the build to let the maintainer know something needs to be fixed. This option would be especially useful for Linux package maintainers who always want to link the system library to avoid "shadow copies" of libgit2 embedded in binaries that are not known to the security team.
  • Fix one clippy warning

The LIBGIT2_NO_VENDOR variable is analogous to OPENSSL_NO_VENDOR in openssl-sys: sfackler/rust-openssl#1253

There's currently no Error struct in the build.rs so I'm using a Result<(), ()>, if you prefer anything else just let me know.

@classabbyamp
Copy link

This would be very useful to void linux, as we've had issues with things not linking to our system libgit2, +1

@lovesegfault
Copy link

This needs a rebase

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Jul 3, 2023

I've rebased the patch

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test for cargo features you'd usually use if cfg!(feature = "zlib-ng-compat") { ... } instead of accessing the environment variable, but since this environment variable could also be set from outside of cargo I'm generating cargo:rerun-if-env-changed= output to let cargo know it needs to rebuild if they're changed

I am not quite understand the change here. Could we explain a bit more what “you” means here? git2 maintainers of git2 users?CARGO_FEATURE_* is something set by cargo for build script to inform external build tools. Cargo features behavior AFAIK is not affected by someone setting those env vars.

Fix one clippy warning

Should we do one thing per commit? (Or per pull request if you'd like to)

let ssh = env_var("CARGO_FEATURE_SSH").is_some();
let vendored = env_var("CARGO_FEATURE_VENDORED").is_some();
let zlib_ng_compat = env_var("CARGO_FEATURE_ZLIB_NG_COMPAT").is_some();
let no_vendor = env_var("LIBGIT2_NO_VENDOR").map_or(false, |s| s != "0");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should doc this new environment variable about why and when people should use it. Maybe in README.md or elsewhere.

@ehuss
Copy link
Contributor

ehuss commented Aug 5, 2023

Closing as completed from #966.

@ehuss ehuss closed this Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants