Skip to content

signal: uclibc SA_* use c_uint type#2503

Open
cppcoffee wants to merge 2 commits intonix-rust:masterfrom
cppcoffee:master
Open

signal: uclibc SA_* use c_uint type#2503
cppcoffee wants to merge 2 commits intonix-rust:masterfrom
cppcoffee:master

Conversation

@cppcoffee
Copy link
Copy Markdown
Contributor

@cppcoffee cppcoffee commented Sep 15, 2024

fixed uclibc SA_* type use c_uint type.

rust-lang/libc#3887 (comment)
rust-lang/libc#3211

Comment thread src/sys/signal.rs
if #[cfg(target_os = "redox")] {
type SaFlags_t = libc::c_ulong;
} else if #[cfg(target_env = "uclibc")] {
type SaFlags_t = libc::c_ulong;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From the comment you linked, this change should be done to #[cfg(all(target_env = "uclibc", target_arch = "mips*"))], why are we doing this for all uclibc triples?🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Though if we do this:

this change should be done to #[cfg(all(target_env = "uclibc", target_arch = "mips*"))]

We need that libc PR and change our libc dependency from the libc-0.2 branch to main, as they are breaking changes for the libc crate.

Signed-off-by: Xiaobo Liu <cppcoffee@gmail.com>
@cppcoffee
Copy link
Copy Markdown
Contributor Author

change to [cfg(all(target_env = "uclibc", target_arch = "mips"))]

Comment thread src/sys/signal.rs Outdated
cfg_if! {
if #[cfg(target_os = "redox")] {
type SaFlags_t = libc::c_ulong;
} else if #[cfg(all(target_env = "uclibc", target_arch = "mips"))] {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
} else if #[cfg(all(target_env = "uclibc", target_arch = "mips"))] {
} else if #[cfg(all(target_env = "uclibc", any(target_arch = "mips"), target_arch = "mips64"))] {

In your libc PR, you did it to both mips and mips64

@SteveLauC
Copy link
Copy Markdown
Member

Though if we do this:

...

We need that libc PR and change our libc dependency from the libc-0.2 branch to main

I cannot merge this PR until we switch the branch of the libc dependency to main, which won't happen soon as the main branch won't be released in the near future.

@tgross35
Copy link
Copy Markdown

We need that libc PR and change our libc dependency from the libc-0.2 branch to main

I cannot merge this PR until we switch the branch of the libc dependency to main, which won't happen soon as the main branch won't be released in the near future.

Since this is tier 3 I can backport rust-lang/libc#3211 once it is merge-ready. Would appreciate a sanity check of that change if you get the chance @SteveLauC.

@SteveLauC
Copy link
Copy Markdown
Member

Since this is tier 3 I can backport rust-lang/libc#3211 once it is merge-ready.

Get it, thanks, then we can finish this PR once the backport is done.

Would appreciate a sanity check of that change if you get the chance @SteveLauC.

Unfortunately, I don't have a uClibc/mips environment, and Nix does not have a CI for that, so I cannot test it:<. But from your comment, that libc PR looks exactly right.

Signed-off-by: Xiaobo Liu <cppcoffee@gmail.com>
@tgross35
Copy link
Copy Markdown

For reference, the libc made it into release 0.2.165.

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.

3 participants