Enable ZMQ_HAVE_STRLCPY for GLIBC >= 2.38 #29

jean-airoldie · 2023-08-04T22:25:46Z

Fixes #28

@MarijnS95 This approach checks glibc version. This results in better compilation speed than the CMAKE CheckCXXSymbolExists approach which involves compiling multiple times. Please tell me if it fixes your issue.

This should work for any platform that uses glibc.

Open questions:

The cargo doc specify target_env == "" as being valid for historical reasons so I wonder if some GNU target might have that field empty.
https://doc.rust-lang.org/reference/conditional-compilation.html#target_env.

MarijnS95 · 2023-08-07T09:52:50Z

src/lib.rs

+
+        pub(crate) fn from_libc() -> Self {
+            // SAFETY Call to libc via ffi.
+            let ptr = unsafe { libc::gnu_get_libc_version() };


This appears to be checking libc on the current host that runs this "build script library", rather than testing if the headers/libraries intended for the target have a certain version?

This does check the host libc version. In case of static linking this works fine. In case of dynamic linking, from what I recall, the binary will be compiled with the host's glibc version and should not run on the target unless it has a equal or newer version of glibc.

[...] rather than testing if the headers/libraries intended for the target have a certain version?

Isn't it the same? If you target gnu, your gonna need to compile with glibc.

Here's few issues for reference on the equal or newer version requirement of glibc dynamic linking:

Rust binaries require a too recent glibc rust-lang/rust#57497

`GLIBC_2.18' not found -- required by rust-analyzer-linux rust-lang/rust-analyzer#4426

Rust programs require recent glibc due to __cxa_thread_atexit_impl rust-lang/rust#36826

If I'm wrong and the host libc can really differ from the target libc version, then i think the approach is to compile and run a program that outputs the libc::gnu_get_libc_version.

In case of static linking this works fine

In case of static linking your host has the static library of the target architecture, however;

then i think the approach is to compile and run a program that outputs the libc::gnu_get_libc_version.

This implies that your host can emulate the target architecture to run such a program.

The reason for the header check is because of the information about GLIBC in part comes from the sysroot.

Though afaik the sysroot/compiler is also available to set the GLIBC version and use older versions, in which case maybe the header emits the inline function as a fallback rather than the symbol? Or maybe older GLIBC header/library source is needed, I'm not too familiar with this.

I have a lot of experience with cross-compilation and building binaries that work in multiple glibc versions. I also worked in a project that runs on ARM and cross-compiles this crate. So this is my input:

glibc is forward-compatible. If you compile for a lower target glibc version, then the same binary works in systems with higher glibc version.

libc::gnu_get_libc_version gets the host glibc version, so cross-compilation will fail.

The classic workaround that autotools and cmake are using, is to actually invoke gcc and the source code will be a small C code that just calls function in question. If the compilation succeeds then the function is supported, otherwise it doesn't.

Based on the above the best approach is what I describe in the 3rd point.

Another option is to use __GLIBC__ and __GLIBC_MINOR__ and get their values with cc::Build::expand.

@oblique Thanks for confirming the above with additional context. Indeed, it should also be possible to extract the glibc version of te target sysroot via such environment variables.

The compilation test in C/C++ build-scripts is likely used in case a symbol is guarded by extra conditionals? Or more likely, to be interoperable across multiple libc standard library providers. Instead of hardcoding versions across platforms (for multiple symbols), this compile-test would for example run naturally on Android cross target instead of having to hardcode what we did in #20. Not saying that we should convert everything thus far to that though, but it's something to keep in mind.

I think the best is to be used when we are not sure. For example in Android we are sure that exists, so we don't need to check via compilation test.

For example in Android we are sure that exists, so we don't need to check via compilation test.

I haven't gone back very far at all though, but it seems very unlikely for anyone to go out of their way and build against even older versions of the NDK (which may still even contain the symbol).

jean-airoldie · 2023-08-07T16:34:55Z

@MarijnS95 I added a fallback for glibc dynamic compilation where we try to compile a simple c program linking against strlcpy in <string.h>.

jean-airoldie · 2023-08-07T20:54:39Z

@MarijnS95 Alright, now the lib does a compilation check to see if strlcpy is in the std lib, like you originally proposed. At least now I understand the reasoning. Could you please tell me if this fixes your issue, since I don't have an easy way to replicate it.

src/lib.rs

Cargo.toml

src/lib.rs

src/strlcpy.c

Co-authored-by: Yiannis Marangos <[email protected]>

oblique · 2023-08-07T22:25:23Z

I tested the build on the latest glibc and works. Also a friend who had ready an environment for cross-compilation tested it and works there too,

jean-airoldie · 2023-08-07T23:30:55Z

@oblique Cool thanks for the help. I'll wait on @MarijnS95 to merge since he opened the issue.

MarijnS95

Works for me on Arch Linux with glibc 2.28, many thanks!

Just have some minor code-cleanup comments.

Note that I don't currently use a GNU cross-compiling environment but thought about pointing it out regardless. I currently only cross-compile to Android which had this hardcoded for a long time (#20), though it looks like @oblique already took the task of testing cross-compiling to GNU targets on them, thanks!

MarijnS95 · 2023-08-08T22:16:26Z

src/lib.rs

@@ -56,6 +56,24 @@ where
    Err(())
 }

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


Path is already in scope in this file?

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.

MarijnS95 · 2023-08-08T22:17:30Z

src/lib.rs

-    env, fs,
-    fs::File,
+    env,
+    fs::{self, File},


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.

Do you have some logical reasoning behind that? I don't really care about that stuff personally.

MarijnS95 · 2023-08-08T22:18:10Z

src/lib.rs

+    // 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 {


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 {

What's the benefit? It won't show up in the generated docs. Is a GUI code editor thing?

MarijnS95 · 2023-08-08T22:20:53Z

src/lib.rs

+    // 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 {
+        let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("src/strlcpy.c");


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.

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.

src/lib.rs

oblique · 2023-08-09T07:56:14Z

src/lib.rs

+    pub(crate) fn has_strlcpy() -> bool {
+        let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("src/strlcpy.c");
+
+        cc::Build::new()
+            .cargo_metadata(false)
+            .warnings_into_errors(true)
+            .file(path)
+            .try_compile("has_strlcpy")
+            .is_ok()
+    }


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.

Does it need to link, given that the CMake scripts AFAIK don't do that either?

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.

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

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.

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.

I think linking is preferable to reduce the risk of false positives.

jean-airoldie · 2023-08-11T20:23:21Z

@oblique @MarijnS95 Thanks again for your time. I'll do a release.

s-nie · 2023-09-15T07:12:38Z

Thanks a lot for the quick fix for this!

EDIT: Disregard the following. It looks like the sccache was not correctly invalidated. Works fine without it.

I noticed that the error still appears in cross compilation on Linux for Windows.

Any idea why this would be? I have a vague memory of platform checks not working as expected because the build script still compiles on Linux and then runs "on Windows," or something like that. So I sometimes have to check the platform with std::env::var("CARGO_CFG_TARGET_OS").as_deref() == Ok("windows").

The output of cargo xwin build --release --target x86_64-pc-windows-msvc in rust-zmq is:

  --- stderr
  /home/sven/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zeromq-src-0.2.6+4.3.4/src/strlcpy.c(5,11): error: call to undeclared library function 'strlcpy' with type 'unsigned long long (char *, const char *, unsigned long long)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      (void)strlcpy(buf, "a", 1);
            ^
  /home/sven/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zeromq-src-0.2.6+4.3.4/src/strlcpy.c(5,11): note: include the header <string.h> or explicitly provide a declaration for 'strlcpy'
  1 error generated.


  error occurred: Command "sccache" "clang-cl" "-nologo" "-MD" "-O2" "-Z7" "-Brepro" "-m64" "--target=x86_64-pc-windows-msvc" "-Wno-unused-command-line-argument" "-fuse-ld=lld-link" "/imsvc/home/sven/.cache/cargo-xwin/xwin/crt/include" "/imsvc/home/sven/.cache/cargo-xwin/xwin/sdk/include/ucrt" "/imsvc/home/sven/.cache/cargo-xwin/xwin/sdk/include/um" "/imsvc/home/sven/.cache/cargo-xwin/xwin/sdk/include/shared" "-I" "/home/sven/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zeromq-src-0.2.6+4.3.4/vendor/include" "-I" "/home/sven/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zeromq-src-0.2.6+4.3.4/vendor/src" "-I" "/home/sven/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zeromq-src-0.2.6+4.3.4/vendor/src" "-I" "/home/sven/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zeromq-src-0.2.6+4.3.4/vendor/external/sha1" "-I" "/home/sven/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zeromq-src-0.2.6+4.3.4/vendor/external/wepoll" "-I" "/home/sven/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zeromq-src-0.2.6+4.3.4/vendor/builds/deprecated-msvc" "/GL-" "/EHsc" "-DZMQ_BUILD_TESTS=OFF" "-DZMQ_USE_CV_IMPL_STL11=1" "-DZMQ_STATIC=1" "-DZMQ_USE_BUILTIN_SHA1=1" "-DZMQ_HAVE_WS=1" "-DZMQ_IOTHREAD_POLLER_USE_EPOLL=1" "-DZMQ_POLL_BASED_ON_POLL=1" "-D_WIN32_WINNT=0x0600" "-DZMQ_HAVE_IPC=1" "-Fo/home/sven/rust-zmq/target/x86_64-pc-windows-msvc/release/build/zmq-sys-5c768056df47cfca/out/lib/c7dc53c602ec3030-wepoll.o" "-c" "--" "/home/sven/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zeromq-src-0.2.6+4.3.4/vendor/external/wepoll/wepoll.c" with args "clang-cl" did not execute successfully (status code exit status: 1).

oblique · 2023-09-15T08:23:00Z

@s-nie Can you create an example repo that reproduces the issues?

s-nie · 2023-09-15T08:24:22Z

I updated the comment, it was an issue with sccache. Sorry for the confusion.

oblique · 2023-09-15T08:34:44Z

I'm still confused :) Is there an issue when sccache is used in general? Or the issue was because of your setup?

s-nie · 2023-09-15T08:44:33Z

So I thought it was just an issue with cache invalidation in sccache. It keeping the pre-fix configuration around would have explained this. But I just tried again with sccache cleared and enabled, and the same issue came up again. My cargo configuration looks like this:

[build]
rustc-wrapper = "sccache"

The sccache version is 0.5.4.

Then, running cargo xwin build --release --target x86_64-pc-windows-msvc in rust-zmq should be enough to reproduce it, given the system has GLIBC 2.38.

jean-airoldie · 2023-09-15T08:49:15Z

Would you mind creating a new separate issue, because this is hard to follow since this is a pull request thread.

MarijnS95 · 2023-09-15T20:09:34Z

src/lib.rs

+        #[cfg(target_env = "gnu")]
+        if !has_strlcpy && glibc::has_strlcpy() {


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.

Enable ZMQ_HAVE_STRLCPY for GLIBC >= 2.38

5ee5d55

jean-airoldie force-pushed the glibc_2.38 branch from d49008c to 5ee5d55 Compare August 4, 2023 22:26

Rename GlibcVersion to Version

34021e4

MarijnS95 reviewed Aug 7, 2023

View reviewed changes

Add fallback for dynamically link glibc

06a44de

jean-airoldie added 3 commits August 7, 2023 12:43

Fix typo

8f5952c

Incorporate feedback

70b2fc3

Merge strlcpy checks

1993bd8

oblique reviewed Aug 7, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Incorporate oblique feedback

c2d1e0a

oblique reviewed Aug 7, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

oblique reviewed Aug 7, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Incorporate feedback

6b9dbe1

oblique reviewed Aug 7, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/strlcpy.c Outdated Show resolved Hide resolved

jean-airoldie and others added 2 commits August 7, 2023 18:00

Update src/strlcpy.c

4e43fea

Co-authored-by: Yiannis Marangos <[email protected]>

Update src/lib.rs

b4eace2

Co-authored-by: Yiannis Marangos <[email protected]>

MarijnS95 approved these changes Aug 8, 2023

View reviewed changes

Fix some std::fs imports

d218084

oblique reviewed Aug 8, 2023

View reviewed changes

src/lib.rs Show resolved Hide resolved

Clarify documentation

036c063

oblique reviewed Aug 9, 2023

View reviewed changes

jean-airoldie added 2 commits August 9, 2023 11:20

has_strlcpy now attempts to create & link and executable

2c4eeb3

Run rustfmt

073409c

jean-airoldie merged commit 2cc6e19 into master Aug 11, 2023

s-nie mentioned this pull request Sep 15, 2023

Cross-compilation with cargo xwin fails when sccache is enabled with GLIBC>=2.38 #30

Closed

MarijnS95 reviewed Sep 15, 2023

View reviewed changes

conanoc mentioned this pull request Oct 6, 2023

Bumping up zmq version to 0.10 hyperledger/indy-vdr#225

Open

jean-airoldie deleted the glibc_2.38 branch November 17, 2023 15:13

		#[cfg(target_env = "gnu")]
		if !has_strlcpy && glibc::has_strlcpy() {

Enable ZMQ_HAVE_STRLCPY for GLIBC >= 2.38 #29

Enable ZMQ_HAVE_STRLCPY for GLIBC >= 2.38 #29

Conversation

jean-airoldie commented Aug 4, 2023 • edited Loading

Open questions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oblique Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jean-airoldie commented Aug 7, 2023

jean-airoldie commented Aug 7, 2023

oblique commented Aug 7, 2023

jean-airoldie commented Aug 7, 2023

MarijnS95 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oblique Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jean-airoldie commented Aug 11, 2023

s-nie commented Sep 15, 2023 • edited Loading

oblique commented Sep 15, 2023 • edited Loading

s-nie commented Sep 15, 2023

oblique commented Sep 15, 2023

s-nie commented Sep 15, 2023 • edited Loading

jean-airoldie commented Sep 15, 2023 • edited Loading

MarijnS95 Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

jean-airoldie commented Aug 4, 2023 •

edited

Loading

oblique Aug 7, 2023 •

edited

Loading

MarijnS95 left a comment •

edited

Loading

oblique Aug 9, 2023 •

edited

Loading

s-nie commented Sep 15, 2023 •

edited

Loading

oblique commented Sep 15, 2023 •

edited

Loading

s-nie commented Sep 15, 2023 •

edited

Loading

jean-airoldie commented Sep 15, 2023 •

edited

Loading

MarijnS95 Sep 15, 2023 •

edited

Loading