-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Change signal constants to c_int on espidf #3895
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
cc @Tevz-Beskovnik because you wrote #3658, just in case you want to tell me that this PR is a bad idea for some target-specific reason 🙈 |
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 looks reasonable to me since the sigaction
functions seem to still use the default int
https://github.com/espressif/newlib-esp32/blob/7cdcb0b0848a5d909e906834fe81903ba9466c90/newlib/libc/include/sys/signal.h#L184 (our version
Line 1533 in cdf6896
pub fn sigaction( |
Also I would rather like to have this in the rustc repo so that the relevant targets can build again. I think that means this needs to go to the 0.2 branch as well? Do you just take care of that? |
Yeah, if you just put the stable-nominated label then I'll get it in the next round of cherry picks before my release this week (I already added it here, and it works with rustbot too) |
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.
I want to do a release today so I'm just going to merge this. @Tevz-Beskovnik feel free to leave any feedback if there is a reason this shouldn't be done. We can always change it back if needed since this is tier 3.
(backport <rust-lang#3895>) (cherry picked from commit a6f4694)
(backport <rust-lang#3895>) (cherry picked from commit a6f4694)
The espidf targets cannot build libtest because the types of these signal constants disagree with all the other cfg(unix) targets. Since they're of course macros on the C side https://github.com/espressif/newlib-esp32/blob/7cdcb0b0848a5d909e906834fe81903ba9466c90/newlib/libc/include/sys/signal.h#L259-L269 I think
c_int
is a more reasonable type to use. Without this change, attempting to build libtest will produce this error:It's not entirely clear to me why the conversation on #3615 and the issues from there petered out. I'm a bit concerned that the users just configured their editors to suppress the error and moved on. That's certainly what it looks like.
I've checked that this fixes the problem by pointing my rust-lang/rust checkout to my libc branch and running
or