alexcrichton / curl-rust

Rust bindings to libcurl
MIT License
1k stars 234 forks source link

Add optional feature to build with C-ARES #427

Closed DBLouis closed 11 months ago

DBLouis commented 2 years ago

This PR adds a way to build with c-ares behind a optional cargo feature.

Issue #327

alexcrichton commented 2 years ago

Thanks! I don't know much about the c-ares-sys crate and would prefer to have that reviewed before pulling it in as a dependency, even if optional. It may take me at least personally a moment to review it, but if someone else beats me to it then I won't complain!

sagebind commented 2 years ago

Here's the relevant error from the CI build:

  cargo:warning=curl/lib/version.c:38:12: fatal error: ares.h: No such file or directory
  cargo:warning=   38 | #  include <ares.h>
  cargo:warning=      |            ^~~~~~~~
  cargo:warning=compilation terminated.
  exit status: 1

It looks like c-ares-sys does not export its headers, so this section of code never runs and the header is not found:

https://github.com/alexcrichton/curl-rust/blob/5772e4047a3a33562ad4a9292505eff29bd32221/curl-sys/build.rs#L244-L247

For this to work, we'll need to open a PR upstream to c-ares-sys first to add a cargo:include= output line to point to the path of the c-ares headers. Something like this will need to be added to their build.rs:

println!("cargo:include={}", c_ares_dir.join("include"));

This tells Cargo to export the include path to build scripts of crates depending on the exporting crate, so that it can be discovered via build scripts in the environment. Otherwise, the DEP_CARES_INCLUDE environment variable will not be defined.

A second problem is that it seems that c-ares-sys 5.3.0+ are already using 2021 Edition which causes compilation to fail on anything older than 1.56.0. Now this crate doesn't have a strict MSRV policy, but this is a pretty new version and probably could cause issues for users of our crate if we suddenly require such a new version. Though since this is behind the c-ares feature, maybe it isn't a major issue.

Alternatively we could pin to version =5.2.0 which works on older Rust versions.

DBLouis commented 2 years ago

@sagebind I'm not sure how to PR this change and backport it to 5.2.x. Maybe the maintainer would agree to revert to previous edition. Otherwise it needs a second branch I think but that might be annoying.

sagebind commented 2 years ago

Good point, those are conflicting asks aren't they? Hmm, not sure the best way to handle that.

Maybe the maintainer would agree to revert to previous edition.

Maybe that's the best route to go so that we can stick with the latest versions.