-
Notifications
You must be signed in to change notification settings - Fork 2k
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
REVERT native: remove non required NATIVEINCLUDES #8940
Conversation
IIRC we don't revert commits (on my first PRs I did things like that, and @miri64 told me that), but rather make "normal" commits which revert what have you done. |
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 confirm it fixes #8922.
ACK from my side
@kYc0o I think you must have misinterpreted the situation where you got that advice. There is no difference between a revert and a commit undoing the same change. On the other hand, if you need to revert only part of a commit, you will need to manually create a commit to perform that change. |
I certainly missed the point, you're right. I can ACK this too, since I was the one who ACKed the original change. |
Just wanted to confirm, that @kYc0o might have misunderstood me, because I don't remember saying something like that and would really be surprised about it ^^. |
I think the situation was different and a revert would make the things worse at that time, so I was told to not do revert but better a new commit undoing the changes. I think it was even not all of the changes but just one. Anyways, that's back on the time where I was doing my first serious C code ^^'. Sorry for the noise. |
Maybe it was that we don't click the revert button of a PR after it has been merged, but create a new PR (which might contain revert comits)? |
Bingo! I think that was the advice I was given. Thanks for the reminder @kaspar030 ! |
…cludes REVERT native: remove non required NATIVEINCLUDES
Contribution description
This revert #8652 PR because in fact, values of posix
AF_*
symbols, and maybe others are not portable. It will fix #8922.When writing #8652 PR, I was missing that what is defined in the posix standard is only symbols not values.
So even right now, if we look at the values of AF_INET6, they are not compatible:
So using our posix headers in interaction with Linux is problematic.
Further steps
Also, I would like to check if it would make sense and would be possible to only use system posix headers in native.
My reason is that if a module using RIOT posix headers, communicates posix values with a module using NATIVEINCLUDES, then they would understand it differently.
Issues/PRs references
Reverts #8652
Fixes #8922
Linked to #8713