-
Notifications
You must be signed in to change notification settings - Fork 15
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
Enable ZMQ_HAVE_STRLCPY for GLIBC >= 2.38 #29
Changes from 10 commits
5ee5d55
34021e4
06a44de
8f5952c
70b2fc3
1993bd8
c2d1e0a
6b9dbe1
4e43fea
b4eace2
d218084
036c063
2c4eeb3
073409c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,6 @@ | ||||||||||||||
use std::{ | ||||||||||||||
env, fs, | ||||||||||||||
fs::File, | ||||||||||||||
env, | ||||||||||||||
fs::{self, File}, | ||||||||||||||
io::Write, | ||||||||||||||
path::{Path, PathBuf}, | ||||||||||||||
}; | ||||||||||||||
|
@@ -56,6 +56,24 @@ where | |||||||||||||
Err(()) | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
#[cfg(target_env = "gnu")] | ||||||||||||||
mod glibc { | ||||||||||||||
use std::path::Path; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not against super imports, i just find them less explicit. In this case, I don't see the point since this is just one line. |
||||||||||||||
|
||||||||||||||
// Attempt to compile a c program that links to strlcpy from the std | ||||||||||||||
// library to determine whether glibc packages it. | ||||||||||||||
jean-airoldie marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
pub(crate) fn has_strlcpy() -> bool { | ||||||||||||||
Comment on lines
+66
to
+68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is intended to be a (developer-only) doc comment:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the benefit? It won't show up in the generated docs. Is a GUI code editor thing? |
||||||||||||||
let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("src/strlcpy.c"); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this pattern occurs more often, the CMake macro generates this file on the fly and doesn't even make any attempt to reproduce the signature faithfully. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. If in the future, if we need to do anything more complex, it would make sense to dynamically generate a file containing all the required links. For now I think its fine. |
||||||||||||||
|
||||||||||||||
cc::Build::new() | ||||||||||||||
.cargo_metadata(false) | ||||||||||||||
.warnings_into_errors(true) | ||||||||||||||
.file(path) | ||||||||||||||
.try_compile("has_strlcpy") | ||||||||||||||
.is_ok() | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just found the existence of pub(crate) fn has_strlcpy() -> bool {
let c_src = Path::new(env!("CARGO_MANIFEST_DIR")).join("src/strlcpy.c");
let bin =
PathBuf::from(env::var("OUT_DIR").unwrap()).join("has_strlcpy");
println!("cargo:rerun-if-changed={}", c_src.display());
cc::Build::new()
.warnings(false)
.get_compiler()
.to_command()
.arg(c_src)
.arg("-o")
.arg(bin)
.status()
.expect("failed to execute gcc")
.success()
} You can also revert to the older comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it need to link, given that the CMake scripts AFAIK don't do that either? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No we don't need to link as long as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warning flags might even get picked up from CMake does a bunch of tweaks to that: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They also use the address of the symbol with a cast to support generic cases (something we don't need to do now) to not have to replicate the function signature. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think linking is preferable to reduce the risk of false positives. |
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// The location of a library. | ||||||||||||||
#[derive(Debug, Clone)] | ||||||||||||||
pub struct LibLocation { | ||||||||||||||
|
@@ -338,6 +356,7 @@ impl Build { | |||||||||||||
build.include(out_includes); | ||||||||||||||
}; | ||||||||||||||
|
||||||||||||||
let mut has_strlcpy = false; | ||||||||||||||
if target.contains("windows") { | ||||||||||||||
// on windows vista and up we can use `epoll` through the `wepoll` lib | ||||||||||||||
add_c_sources( | ||||||||||||||
|
@@ -379,22 +398,31 @@ impl Build { | |||||||||||||
build.define("ZMQ_HAVE_UIO", "1"); | ||||||||||||||
|
||||||||||||||
if target.contains("android") { | ||||||||||||||
build.define("ZMQ_HAVE_STRLCPY", "1"); | ||||||||||||||
has_strlcpy = true; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
if target.contains("musl") { | ||||||||||||||
build.define("ZMQ_HAVE_STRLCPY", "1"); | ||||||||||||||
has_strlcpy = true; | ||||||||||||||
} | ||||||||||||||
} else if target.contains("apple") { | ||||||||||||||
create_platform_hpp_shim(&mut build); | ||||||||||||||
build.define("ZMQ_IOTHREAD_POLLER_USE_KQUEUE", "1"); | ||||||||||||||
build.define("ZMQ_POLL_BASED_ON_POLL", "1"); | ||||||||||||||
build.define("HAVE_STRNLEN", "1"); | ||||||||||||||
build.define("ZMQ_HAVE_STRLCPY", "1"); | ||||||||||||||
build.define("ZMQ_HAVE_UIO", "1"); | ||||||||||||||
build.define("ZMQ_HAVE_IPC", "1"); | ||||||||||||||
has_strlcpy = true; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
// https://github.com/jean-airoldie/zeromq-src-rs/issues/28 | ||||||||||||||
#[cfg(target_env = "gnu")] | ||||||||||||||
if !has_strlcpy && glibc::has_strlcpy() { | ||||||||||||||
Comment on lines
+429
to
+430
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this panic!(
"CARGO_CFG_TARGET_ENV: {:?} target_env=gnu?: {}",
env::var("CARGO_CFG_TARGET_ENV"),
cfg!(target_env = "gnu")
); And run with: cargo b --target x86_64-pc-windows-msvc -p testcrate Gives the following output:
confirms that. |
||||||||||||||
has_strlcpy = true; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
if has_strlcpy { | ||||||||||||||
build.define("ZMQ_HAVE_STRLCPY", "1"); | ||||||||||||||
} | ||||||||||||||
let out_dir = PathBuf::from(env::var("OUT_DIR").unwrap()); | ||||||||||||||
let lib_dir = out_dir.join("lib"); | ||||||||||||||
|
||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
#include <string.h> | ||
|
||
int main() { | ||
char buf[1]; | ||
(void)strlcpy(buf, "a", 1); | ||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'm really against importing
self
and then a few other items into scope, rather then also qualifying them with e.g.fs::File
if I really wanted to importfs
into scope.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have some logical reasoning behind that? I don't really care about that stuff personally.