-
Notifications
You must be signed in to change notification settings - Fork 108
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
feat: Fix and cover tests for target x86_64-unknown-linux-musl #140
Conversation
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
fn write_thread_name(current_thread: libc::pthread_t, name: &mut [libc::c_char]) { | ||
write_thread_name_fallback(current_thread, name); | ||
} | ||
|
||
#[cfg(any(target_os = "linux", target_os = "macos"))] | ||
#[cfg(all(any(target_os = "linux", target_os = "macos"), target_env = "gnu"))] |
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.
musl added pthread_getname_np
after 1.2.3, and it's also included in the rust libc 🤔
I don't know whether upgrading musl would be a heavy burden for users, but I recommanded to upgrade it.
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.
This part is a bit tricky. Even on musl 1.2.3, it still doesn't work correctly.
cargo build
works, but cargo test
will have the following error:
= note: /usr/bin/ld: /home/xuanwo/Code/tikv/pprof-rs/target/x86_64-unknown-linux-musl/debug/deps/pprof-f3595f784aae57b5.1iodjb8b0079y0t7.rcgu.o: in function `pprof::profiler::write_thread_name':
/home/xuanwo/Code/tikv/pprof-rs/src/profiler.rs:194: undefined reference to `pthread_getname_np'
collect2: error: ld returned 1 exit status
= help: some `extern` functions couldn't be found; some native libraries may need to be installed or have their path specified
= note: use the `-l` flag to specify native libraries to link
= note: use the `cargo:rustc-link-lib` directive to specify the native libraries to link with Cargo (see https://doc.rust-lang.org/cargo/reference/build-scripts.html#cargorustc-link-libkindname)
Any ideas about this?
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.
@YangKeao Hi, can you take a look at this PR again? I'm willing to change the code if there are other ways to address this issue.
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.
It's pretty wired. I run nm libc.a |grep pthread|grep getname
in the alpine, and got:
0000000000000000 T pthread_getname_np
But I came across the same issue you faced. I will approve and merge this PR to make it work under musl (and I'll investigate further why it cannot link with this symbol)
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.
LGTM
fn write_thread_name(current_thread: libc::pthread_t, name: &mut [libc::c_char]) { | ||
write_thread_name_fallback(current_thread, name); | ||
} | ||
|
||
#[cfg(any(target_os = "linux", target_os = "macos"))] | ||
#[cfg(all(any(target_os = "linux", target_os = "macos"), target_env = "gnu"))] |
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.
It's pretty wired. I run nm libc.a |grep pthread|grep getname
in the alpine, and got:
0000000000000000 T pthread_getname_np
But I came across the same issue you faced. I will approve and merge this PR to make it work under musl (and I'll investigate further why it cannot link with this symbol)
How about creating an issue for the unresolved problem? |
) * Fix tests and examples under musl Signed-off-by: Xuanwo <[email protected]> * Cover build and test under musl Signed-off-by: Xuanwo <[email protected]> * Fix target not installed Signed-off-by: Xuanwo <[email protected]> * Install musl-tools Signed-off-by: Xuanwo <[email protected]> * Don't run test for aarch Signed-off-by: Xuanwo <[email protected]> * Cancel previous jobs Signed-off-by: Xuanwo <[email protected]> Signed-off-by: Christian Jordan <[email protected]>
This PR addresses the following problems:
x86_64-unknown-linux-musl