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

feat: Fix and cover tests for target x86_64-unknown-linux-musl #140

Merged
merged 6 commits into from Jun 24, 2022

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Jun 23, 2022

This PR addresses the following problems:

  • Tests and examples not compilable under x86_64-unknown-linux-musl
  • Add tests in GitHub actions so that all PRs are covered.

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
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"))]
Copy link
Member

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.

Copy link
Member Author

@Xuanwo Xuanwo Jun 23, 2022

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?

Copy link
Member Author

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.

Copy link
Member

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)

Copy link
Member

@YangKeao YangKeao left a 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"))]
Copy link
Member

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)

@YangKeao YangKeao merged commit 1b19478 into tikv:master Jun 24, 2022
@Xuanwo
Copy link
Member Author

Xuanwo commented Jun 24, 2022

How about creating an issue for the unresolved problem?

@Xuanwo Xuanwo deleted the cover-musl branch June 24, 2022 16:10
Jardynq pushed a commit to Jardynq/pprof-rs that referenced this pull request Jul 24, 2022
)

* Fix tests and examples under musl

Signed-off-by: Xuanwo <github@xuanwo.io>

* Cover build and test under musl

Signed-off-by: Xuanwo <github@xuanwo.io>

* Fix target not installed

Signed-off-by: Xuanwo <github@xuanwo.io>

* Install musl-tools

Signed-off-by: Xuanwo <github@xuanwo.io>

* Don't run test for aarch

Signed-off-by: Xuanwo <github@xuanwo.io>

* Cancel previous jobs

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Christian Jordan <svorre2304@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants