Skip to content

cpu: moved all vendor headers to CPUNAME/include/vendor/#6700

Merged
jnohlgard merged 7 commits intoRIOT-OS:masterfrom
haukepetersen:opt_vendorheaders
Mar 7, 2017
Merged

cpu: moved all vendor headers to CPUNAME/include/vendor/#6700
jnohlgard merged 7 commits intoRIOT-OS:masterfrom
haukepetersen:opt_vendorheaders

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Mar 6, 2017

The reason for this is to simplify handling of vendor headers in the future, e.g. by easier excluding them from doxygen builds etc...

@haukepetersen haukepetersen added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 6, 2017
@haukepetersen haukepetersen added this to the Release 2017.04 milestone Mar 6, 2017
kaspar030
kaspar030 reviewed Mar 6, 2017
View reviewed changes
doc/doxygen/riot.doxyfile Outdated
*/cpu/lpc11u34/include/LPC11Uxx.h \
*/cpu/lpc1768/include/LPC17xx.h \
*/cpu/lpc*/include/core_cm*.h \
*/cpu/*/include/vendor/*
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 Mar 6, 2017

Choose a reason for hiding this comment

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

Why not */vendor/*?

Copy link
Copy Markdown
Contributor Author

@haukepetersen haukepetersen Mar 6, 2017

Choose a reason for hiding this comment

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

good point. will adapt

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Mar 6, 2017

Could you also adapt the cppcheck filters in dist/tools/cppcheck/check.sh?

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Mar 6, 2017

done

@haukepetersen haukepetersen mentioned this pull request Mar 6, 2017
Merged
6 tasks
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Mar 6, 2017

fixed excludes for externc and some empty line at EOF issues.

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Mar 6, 2017

I'm globally +1 with the goal of this PR but as you have noticed, I have some opened PRs that will be impacted : #6651, #6025, #6553, #6594, #6615, #6625.
Some of them are opened for a long time now (#6025) or closed to being merged (just missing a positive review) (#6651) .
Would it be possible to merge them first ?

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Mar 6, 2017

Good idea, but what about having two separate trees, instead of polluting the include tree.
Like this:
Cpu/stm32f4/vendor/include

Also, while I can see that having to prefix vendor/ on all vendor includes has its merits with regards to readability, it may also cause a hassle when vendor headers include other vendor headers (e.g. Atmel CPUs).
I'm writing this from the phone so I haven't really managed to get a good overview of this PR, so please excuse me if I have missed something here.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Mar 6, 2017

@aabadie: I would prefer to merge this PR first (as there are probably another 20 PRs or so, that would need to be merged first...), as right now there is the effort for the 'code quality task force'.

@gebart I think this PR is not polluting the include tree, IMHO it's quite the contrary by cleanly separating the vendor headers. Regarding the vendor libraries (as e.g. the Atmel files), this PR has no negative impact, as they use relative include paths and find their 'peer' files locally without even looking into the include tree...

@jnohlgard jnohlgard added the Impact: major The PR changes a significant part of the code base. It should be reviewed carefully label Mar 6, 2017
jnohlgard
jnohlgard reviewed Mar 6, 2017
View reviewed changes
doc/doxygen/riot.doxyfile Outdated
*/cpu/lpc11u34/include/LPC11Uxx.h \
*/cpu/lpc1768/include/LPC17xx.h \
*/cpu/lpc*/include/core_cm*.h \
*/vendor/* \
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Mar 6, 2017

Choose a reason for hiding this comment

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

nice cleanup here! 👍
Could you move it to the correct place to keep the list sorted?

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Mar 6, 2017

@haukepetersen After looking at this properly on my PC I am starting to agree that this is a good solution.
Added the major tag because this touches a lot of files across the tree.

@aabadie the rebase of other PRs touching these vendor headers should be quite uncomplicated. I agree with @haukepetersen that merging this is preferable compared to merging everything else first. There are new PRs opened all the time so it would be an infinite catching up to try to get this merged.

kaspar030
kaspar030 approved these changes Mar 6, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK. (Needs more ACKs, though.)

OlegHahm
OlegHahm approved these changes Mar 6, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@OlegHahm OlegHahm left a comment

Choose a reason for hiding this comment

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

I don't think that splitting it up into so many commits is necessary, but I leave it to Hauke if he wants to squash or not. ACK for the change.

@OlegHahm OlegHahm added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 6, 2017
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Mar 6, 2017

@gebart, indeed, just tested the rebase, things works fine (even if I have to modify files afterwards but that was expected anyways).

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Mar 7, 2017

Addressed comment by @gebart above and squashed. Now waiting for the CI once more to make sure I didn't break anything and then we should be ready to merge this.

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Mar 7, 2017

Murdock2 currently chokes as cppcheck takes longer than 5 minutes, which is Murdock2's default job timeout. The other CI's agree, so I'd say merge!

jnohlgard
jnohlgard approved these changes Mar 7, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

ACK

@jnohlgard jnohlgard merged commit 302d5d3 into RIOT-OS:master Mar 7, 2017
@haukepetersen haukepetersen deleted the opt_vendorheaders branch March 7, 2017 10:17
@kYc0o kYc0o mentioned this pull request Mar 17, 2017
Merged
@jnohlgard jnohlgard mentioned this pull request Nov 22, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnohlgard jnohlgard jnohlgard approved these changes

@OlegHahm OlegHahm OlegHahm approved these changes

@kaspar030 kaspar030 kaspar030 approved these changes

Assignees

@miri64 miri64

@OlegHahm OlegHahm

@kaspar030 kaspar030

Labels

Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2017.04

Development

Successfully merging this pull request may close these issues.

6 participants

@haukepetersen @kaspar030 @aabadie @jnohlgard @OlegHahm @miri64