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

REVERT native: remove non required NATIVEINCLUDES #8940

Merged
merged 3 commits into from
Apr 12, 2018

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Apr 12, 2018

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:

  • RIOT: 4
  • Linux: 10
  • MAC: 30

So using our posix headers in interaction with Linux is problematic.

Further steps

  • Add a documentation about it
  • Re-order includes to match RIOT include order

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

@cladmi cladmi added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: native Platform: This PR/issue effects the native platform Area: POSIX Area: POSIX API wrapper labels Apr 12, 2018
@cladmi cladmi added this to the Release 2018.04 milestone Apr 12, 2018
@cladmi cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 12, 2018
@kYc0o
Copy link
Contributor

kYc0o commented Apr 12, 2018

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.

Copy link
Member

@vincent-d vincent-d left a 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

@jnohlgard
Copy link
Member

@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.

@kYc0o
Copy link
Contributor

kYc0o commented Apr 12, 2018

@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.

@kYc0o kYc0o merged commit d85c294 into RIOT-OS:master Apr 12, 2018
@miri64
Copy link
Member

miri64 commented Apr 12, 2018

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 ^^.

@kYc0o
Copy link
Contributor

kYc0o commented Apr 12, 2018

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.

@kaspar030
Copy link
Contributor

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)?

@kYc0o
Copy link
Contributor

kYc0o commented Apr 12, 2018

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 !

@cladmi cladmi deleted the pr/revert_remove_nativeincludes branch April 16, 2018 12:43
maxvankessel pushed a commit to maxvankessel/RIOT that referenced this pull request Apr 20, 2018
…cludes

REVERT native: remove non required NATIVEINCLUDES
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: POSIX Area: POSIX API wrapper CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: native Platform: This PR/issue effects the native platform Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native CAN device does not build when VFS is used
6 participants