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

Enable ZMQ_HAVE_STRLCPY for GLIBC >= 2.38 #29

Merged
merged 14 commits into from
Aug 11, 2023
38 changes: 33 additions & 5 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{
env, fs,
fs::File,
env,
fs::{self, File},
Copy link
Contributor

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 import fs into scope.

Copy link
Owner Author

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.

io::Write,
path::{Path, PathBuf},
};
Expand Down Expand Up @@ -56,6 +56,24 @@ where
Err(())
}

#[cfg(target_env = "gnu")]
mod glibc {
use std::path::Path;
Copy link
Contributor

Choose a reason for hiding this comment

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

Path is already in scope in this file?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The 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
// Attempt to compile a c program that links to strlcpy from the std
// library to determine whether glibc packages it.
pub(crate) fn has_strlcpy() -> bool {
/// Attempt to compile a c program that links to strlcpy from the [`std`]
/// library to determine whether glibc packages it.
pub(crate) fn has_strlcpy() -> bool {

Copy link
Owner Author

Choose a reason for hiding this comment

The 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");
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Owner Author

Choose a reason for hiding this comment

The 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()
}
Copy link

Choose a reason for hiding this comment

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

I just found the existence of get_compiler. With that we can actually link and create an executable, which is more correct. So this is better:

    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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link

Choose a reason for hiding this comment

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

No we don't need to link as long as strlcpy.c produces only the warning we are interested in. If in the future gcc releases a new warning appears, then build will fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Warning flags might even get picked up from cc-rs-defined environment variables, skewing the result?

CMake does a bunch of tweaks to that:
https://github.com/Kitware/CMake/blob/master/Modules/CheckSymbolExists.cmake

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link

@oblique oblique Aug 9, 2023

Choose a reason for hiding this comment

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

Warning flags might even get picked up from cc-rs-defined environment variables, skewing the result?

Yes. cc-rs is aware of CFLAGS and CFLAGS_<target> environment variables.

Copy link
Owner Author

Choose a reason for hiding this comment

The 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 {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Copy link
Contributor

@MarijnS95 MarijnS95 Sep 15, 2023

Choose a reason for hiding this comment

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

I think this cfg checks the target_env of the host. Adding this snippet right above:

        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:

  thread 'main' panicked at 'CARGO_CFG_TARGET_ENV: Ok("msvc") target_env=gnu?: true', src/lib.rs:429:9

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");

Expand Down
7 changes: 7 additions & 0 deletions src/strlcpy.c
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;
}