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 support for c-ares 1.13.0 #80

Merged
merged 9 commits into from
Nov 11, 2023
Merged

add support for c-ares 1.13.0 #80

merged 9 commits into from
Nov 11, 2023

Conversation

zh-jq
Copy link
Contributor

@zh-jq zh-jq commented Oct 30, 2023

step3 as described in #74.

The oldest support c-ares version is 1.13.0, which is available in RHEL 8.
I can compile on Debian 10 hosts which have c-ares 1.14.0 with all of the vendored feature flags.

@zh-jq
Copy link
Contributor Author

zh-jq commented Oct 30, 2023

And to add CI tests, it may be better to add a standalone c-ares-src crate which can be used to select old versions of c-ares.

@zh-jq zh-jq changed the title add support for c-ares 1.14.0 add support for c-ares 1.13.0 Oct 30, 2023
@dimbleby
Copy link
Owner

dimbleby commented Nov 3, 2023

thanks for this, sorry it's taken me a few days to get to look at it - and then it's taken me a little time to understand properly what you've done, too!

I think more needs to be done in ffi.rs, it is going to need to understand the c-ares version too

  • on ares_options the fields resolvconfpath, hosts_path and udp_max_queries have all been added (at 1.15, 1.19, 1.20)
  • on ares_addrinfo, name was added at 1.18

So at the moment it's possible for people - including potentially this library - to try and use those fields, but if the underlying c-ares version is old then that is accessing unowned memory.

Say if you need help re-generating ffi.rs.

  • I have a slightly annoying scheme at the moment where it is autogenerated with rust-bindgen, but then I apply minor patches manually: this is encoded in generate-ffi.sh.
  • It's a bit tedious when the patch needs to change, I don't really have a better way than to (i) autogenerate an unpatched "before", (ii) manually create the "after", (iii) recreate the patch (iv) re-run generate-ffi.sh to make sure I got it right

I'll also leave some minor comments on the code changes

build.rs Outdated Show resolved Hide resolved
#include <ares_version.h>

#define VERSION2(a, b, c) RUST_VERSION_C_ARES_##a##_##b##_##c
#define VERSION(a, b, c) VERSION2(a, b, c)
Copy link
Owner

Choose a reason for hiding this comment

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

I guess my C must be rusty - if you'll forgive the pun. What's the point of having both VERSION and VERSION2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to take the value of ARES_VERSION_MAJOR these macros

Copy link
Owner

@dimbleby dimbleby Nov 6, 2023

Choose a reason for hiding this comment

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

I don't follow.

Why isn't a single macro #define VERSION(a, b, c) RUST_VERSION_C_ARES_##a##_##b##_##c enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would expand to
RUST_VERSION_C_ARES_ARES_VERSION_MAJOR_ARES_VERSION_MINOR_ARES_VERSION_PATCH,
where the value of ARES_ARES_VERSION_MAJOR is not expanded.

c-ares-sys/build/main.rs Outdated Show resolved Hide resolved
#include <ares_version.h>

#define VERSION2(a, b, c) RUST_VERSION_C_ARES_##a##_##b##_##c
#define VERSION(a, b, c) VERSION2(a, b, c)
Copy link
Owner

@dimbleby dimbleby Nov 6, 2023

Choose a reason for hiding this comment

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

I don't follow.

Why isn't a single macro #define VERSION(a, b, c) RUST_VERSION_C_ARES_##a##_##b##_##c enough?

c-ares-sys/src/ffi.rs Outdated Show resolved Hide resolved
c-ares-sys/src/ffi.rs Outdated Show resolved Hide resolved
@dimbleby
Copy link
Owner

dimbleby commented Nov 6, 2023

if the #[cfg(cares1_17)] on the CAA-related things are necessary then similar is necessary on lots of other things too: ares_getaddrinfo and URI related things and ares_free_string and maybe more that I missed.

That would be tedious, if we can convince ourselves that we don't need to gate any of those then I'd be happier

@zh-jq
Copy link
Contributor Author

zh-jq commented Nov 7, 2023

yes it's unnecessary to gate away caa things in ffi, but I think maybe it's better to keep this sync with c-ares?
I'm OK with both the compile error (which is simple) and the feature gate (sync with c-ares).

@dimbleby
Copy link
Owner

dimbleby commented Nov 7, 2023

providing we are happy that we are not allowing any unsafe behaviour - which I think is the case - I prefer the simple approach with fewer #[cfg()] in src/ffi.rs, please. Keeping the patches small will make maintenance that little bit easier when c-ares changes.

In practice, I would be very surprised to learn about anyone using c-ares-sys directly - it may be that only you and I even notice the difference. If anyone does later show up with a reason to go the other way, I can revisit that decision.

@zh-jq
Copy link
Contributor Author

zh-jq commented Nov 8, 2023

It should be ready now

@dimbleby
Copy link
Owner

dimbleby commented Nov 8, 2023

thanks - looks good I think.

I'll likely leave it a day or three: to give myself a chance to remember anything I've forgotten about, and to play with it a bit more. Maybe cut a release at the weekend.

@zh-jq
Copy link
Contributor Author

zh-jq commented Nov 8, 2023

OK. I will update dimbleby/c-ares-resolver#28 once the new release available.

@dimbleby
Copy link
Owner

dimbleby commented Nov 8, 2023

cargo is mysteriously printing a warning containing the text expando.c during the windows build. Any ideas?

eg https://github.com/dimbleby/rust-c-ares/actions/runs/6792983111/job/18467106231?pr=80#step:8:62

edit: rust-lang/cc-rs#896. Probably not much to be done about that here.

@dimbleby dimbleby merged commit 658060d into dimbleby:main Nov 11, 2023
7 checks passed
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