Skip to content
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

Add optional feature to build with C-ARES #427

Closed
wants to merge 0 commits into from

Conversation

DBLouis
Copy link
Contributor

@DBLouis DBLouis commented Nov 23, 2021

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

Issue #327

@alexcrichton
Copy link
Owner

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
Copy link
Collaborator

sagebind commented Dec 5, 2021

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:

curl-rust/curl-sys/build.rs

Lines 244 to 247 in 5772e40

if let Some(path) = env::var_os("DEP_CARES_INCLUDE") {
let path = PathBuf::from(path);
cfg.include(path.join("include"));
}

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
Copy link
Contributor Author

DBLouis commented Dec 13, 2021

@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
Copy link
Collaborator

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.

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.

None yet

3 participants