Skip to content

Add res_init #585

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.

Sign up for GitHub

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

Merged
bors merged 8 commits into from
May 3, 2017
Merged

Add res_init #585

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ef20ddb
Add binding for res_init.
jonhoo Apr 27, 2017
f93cf62
OpenBSD can't -lresolv, Android shouldn't need it
jonhoo Apr 28, 2017
afebd98
Android doesn't expose res_init in header file
jonhoo Apr 29, 2017
b8f5838
OpenBSD also uses res_init like NetBSD?
jonhoo Apr 29, 2017
6eb8d1c
Treat bitrig and openbsd the same (@semarie)
jonhoo Apr 29, 2017
b3700e4
Blacklist > whitelist
jonhoo May 3, 2017
fcf5fc0
Avoid special-casing linking with resolv
jonhoo May 3, 2017
be7e45f
Skip res_init test on macOS/iOS
jonhoo May 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions libc-test/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ fn main() {
cfg.header("netinet/in.h");
cfg.header("netinet/ip.h");
cfg.header("netinet/tcp.h");
cfg.header("resolv.h");
cfg.header("pthread.h");
cfg.header("dlfcn.h");
cfg.header("signal.h");
Expand Down Expand Up @@ -468,6 +469,10 @@ fn main() {
// it's in a header file?
"endpwent" if android => true,

// Apparently it exists, but isn't defined in a header:
// https://mail.gnome.org/archives/commits-list/2013-May/msg01329.html
"res_init" if android => true,

_ => false,
}
});
Expand Down
33 changes: 30 additions & 3 deletions src/unix/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,9 +206,19 @@ pub const PRIO_MAX: ::c_int = 20;
cfg_if! {
if #[cfg(dox)] {
// on dox builds don't pull in anything
} else if #[cfg(all(not(stdbuild), feature = "use_std"))] {
} else if #[cfg(all(not(stdbuild),
feature = "use_std",
not(any(target_os = "macos",
target_os = "ios")
)))] {
// cargo build, don't pull in anything extra as the libstd dep
// already pulls in all libs.
} else if #[cfg(all(not(stdbuild), feature = "use_std"))] {
// except on macOS and iOS, where we must link with lib resolv
// for res_init, despite libsystem_info including it:
// http://blog.achernya.com/2013/03/os-x-has-silly-libsystem.html
#[link(name = "resolv")]

@alexcrichton alexcrichton May 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm a little wary of doing this, especially outside the stdbuild blocks. Just in terms of possible breakage on new platforms. I do realize though that this is required to link correctly as libc on crates.io otherwise doesn't pull in new libs (it relies on libc in std to do that).

Perhaps we could elide this section (or move it down below after the clause above) and then update the copy in libstd? We can rely on rust-lang/rust CI to block changes if it'd otherwise cause linkage problems. In the meantime we could just disable verification of the res_init symbol in libc as we already know it works!

@jonhoo jonhoo May 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow this? What do you mean by "move it down below after the clause above"? And by "update the copy in libstd"?

@alexcrichton alexcrichton May 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah sorry, basically what I mean is:

  • Ideally, no libs are added
  • If a lib must be added, it should be added without adding clauses to cfg_if!
  • If a lib must be added, disable tests in libc-test and we'll test that it works in rust-lang/rust instead

@jonhoo jonhoo May 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So we should simply not link with resolv on macOS/iOS, both on stdbuild and not(stdbuild) , and disable the test for res_init if apple?

@alexcrichton alexcrichton May 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's start off by disabling linking to resolv everywhere. Let's then only enable it in the not(stdbuild) blocks to ensure we don't accidentally break crates.io. By only enabling it in not(stdbuild) then we can't test it in this PR, but it's ok to leave it enabled on all platforms you have it defined on currently.

@jonhoo jonhoo May 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've removed all the linking with libresolv in fcf5fc0. As noted in my discussion comment though, it seems like res_init is just never tested any more following the res_init if android => true addition made in afebd98.

extern {}
} else if #[cfg(any(all(target_env = "musl", not(target_arch = "mips"))))] {
#[link(name = "c", kind = "static", cfg(target_feature = "crt-static"))]
#[link(name = "c", cfg(not(target_feature = "crt-static")))]
Expand All @@ -223,8 +233,12 @@ cfg_if! {
#[link(name = "m")]
extern {}
} else if #[cfg(any(target_os = "macos",
target_os = "ios",
target_os = "android",
target_os = "ios"))] {
#[link(name = "c")]
#[link(name = "m")]
#[link(name = "resolv")]
extern {}
} else if #[cfg(any(target_os = "android",
target_os = "openbsd",
target_os = "bitrig"))] {
#[link(name = "c")]
Expand Down Expand Up @@ -694,6 +708,19 @@ extern {
res: *mut *mut addrinfo) -> ::c_int;
pub fn freeaddrinfo(res: *mut addrinfo);
pub fn gai_strerror(errcode: ::c_int) -> *const ::c_char;
#[cfg_attr(all(unix,
not(target_os = "macos"),
not(target_os = "ios"),
not(target_os = "netbsd"),
not(target_os = "openbsd"),
not(target_os = "bitrig"),
not(target_os = "solaris"),
not(target_env = "musl")

@alexcrichton alexcrichton May 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps this could be converted to a whitelist? It's mostly just targeting Linux/android/FreebSD it looks like?

),
link_name = "__res_init")]
#[cfg_attr(any(target_os = "macos", target_os = "ios"),
link_name = "res_9_init")]
pub fn res_init() -> ::c_int;

#[cfg_attr(target_os = "netbsd", link_name = "__gmtime_r50")]
pub fn gmtime_r(time_p: *const time_t, result: *mut tm) -> *mut tm;
Expand Down