Enable libc-test and fix definitions/declarations for AIX #4450
Conversation
xingxue-ibm
force-pushed
the
enable-aix-libc-test
branch
5 times, most recently
from
0a7ad40 to
6c81afa
Compare
libc-test/test/cmsg.rs
Outdated
| #[cfg(target_os = "aix")] | ||
| if cmsg_len % std::mem::size_of::<cmsghdr>() != 0 { | ||
| continue; | ||
| } | ||
| for next_cmsg_len in 0..32 { | ||
| // The size of socklen_t is 4-bytes on AIX. | ||
| #[cfg(target_os = "aix")] | ||
| let cmsg_len2: u32 = cmsg_len as u32; | ||
| #[cfg(not(target_os = "aix"))] |
There was a problem hiding this comment.
Could if cfg!(...) be used instead of #[cfg(...)] so the test still gets type checked on other platforms?
There was a problem hiding this comment.
Thanks for the comment! I tried the following:
(*pcmsghdr).cmsg_len = if cfg!(target_os = "aix") {
cmsg_len as u32
} else {
cmsg_len
};
but got this error:
--> libc-test/test/cmsg.rs:83:29
|
83 | ... cmsg_len
| ^^^^^^^^ expected `u32`, found `usize`
|
help: you can convert a `usize` to a `u32` and panic if the converted value doesn't fit
|
83 | cmsg_len.try_into().unwrap()
| ++++++++++++++++++++
Changing as the compiler suggests does work:
(*pcmsghdr).cmsg_len = if cfg!(target_os = "aix") {
cmsg_len as u32
} else {
cmsg_len.try_into().unwrap()
};
But this does not seem to check the type on other platforms either. Any suggestions?
There was a problem hiding this comment.
Ah, I didn't realize they are different types. Could you just do (*pcmsghdr).cmsg_len = cmsg_len as _?
There was a problem hiding this comment.
That's a great idea! It works perfectly, thanks so much, @tgross35!
|
One minor nit from me. I don't see anything concerning, but also don't know the platform so I'll ping the target maintainers. @daltentry or @gilamn5tr would one of you mind double checking this? |
|
|
LGTM from the AIX side. |
xingxue-ibm
force-pushed
the
enable-aix-libc-test
branch
from
6c81afa to
accc52e
Compare
libc-test/test/cmsg.rs
Outdated
| // Address must be a multiple of 0x4 for testing on AIX. | ||
| #[cfg(target_os = "aix")] | ||
| if cmsg_len % std::mem::size_of::<cmsghdr>() != 0 { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Sorry, I missed this one. Could you also replace #[cfg(...)] with cfg!(...) && in the if statement?
Avoiding #[cfg(...)] when possible is ideal because it keeps things from accidentally breaking even though we don't test aix.
There was a problem hiding this comment.
Yeah, that makes sense. Thanks again!
xingxue-ibm
force-pushed
the
enable-aix-libc-test
branch
from
accc52e to
8c97246
Compare
Head branch was pushed to by a user without write access
xingxue-ibm
force-pushed
the
enable-aix-libc-test
branch
from
8c97246 to
c192a5c
Compare
|
Hi @tgross35, Since this patch fixes functional definitions and declarations that affect AIX users, we’re wondering if it would be possible to cherry-pick it into the stable branch. I’ve created a draft PR libc v0.2 cherry-pick: Enable libc-test and fix definitions/declarations for AIX #4455 which applies the cherry-pick with minor merge conflict resolutions in Additionally, I had to modify The CI for the draft PR failed in the |
|
No need to open a PR (but thank you for putting one up); just add the label and it will get into the next release 👍. I do them in batches to avoid conflicts from out of order picks. @rustbot label +stable-nominated |
rustbot
added
the
stable-nominated
Got it, thanks! |
Description
This PR enables
libc-teston AIX. With the fixes to definitions and declarations,libc-testnow runs successfully on AIX.Sources
Checklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see rust-lang/libc#3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI