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

travis: Add include_check job #9286

Merged
merged 1 commit into from
Jan 22, 2019

Conversation

alekla01
Copy link
Contributor

@alekla01 alekla01 commented Jan 8, 2019

Description

MBEDOSTEST-421
check for #include "mbed.h".
#include "mbed.h" should not be used in header and source files.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[x] Test update
[ ] Breaking change

Reviewers

@AnttiKauppila

@alekla01 alekla01 changed the title Update .travis.yml travis include_check Jan 8, 2019
.travis.yml Outdated
- NAME=include_check
script:
- echo 'Checking that there are no '#include "mbed.h"' in code where it should not be'
- ! git grep '^#include "mbed.h"$' -- '*.c' '*.h' '*.cpp' '*.hpp' ':!*platform_mbed.h' ':!*TESTS/*' ':!TEST_APPS/' ':!UNITTESTS/' ':!*tests/*' ':!*targets/*' ':!*TARGET_*' ':!*unsupported/*'
Copy link
Contributor

Choose a reason for hiding this comment

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

why is targets folder also excluded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to @AnttiKauppila: targets folder is excluded for now because it has not been fixed as of yet.

Choose a reason for hiding this comment

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

I checked and there are 5 #mbed.h" includes under targets/. Maybe we should raise minor defects out of those. I want that corresponding teams fixes those because testing changes can be difficult.

Copy link
Contributor

Choose a reason for hiding this comment

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

That explains it, lets create issues for these and will be fixed

@ciarmcom ciarmcom requested review from AnttiKauppila and a team January 8, 2019 10:00
@ciarmcom
Copy link
Member

ciarmcom commented Jan 8, 2019

@alekla01, thank you for your changes.
@AnttiKauppila @ARMmbed/mbed-os-maintainers please review.

.travis.yml Outdated
- NAME=include_check
script:
- echo 'Checking that there are no '#include "mbed.h"' in code where it should not be'
- ! git grep '^#include "mbed.h"$' -- '*.c' '*.h' '*.cpp' '*.hpp' ':!*platform_mbed.h' ':!*TESTS/*' ':!TEST_APPS/' ':!UNITTESTS/' ':!*tests/*' ':!*targets/*' ':!*TARGET_*' ':!*unsupported/*'
Copy link
Contributor

Choose a reason for hiding this comment

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

@0xc0170 Could you confirm that astyle settings will guarantee that ^#include "mbed.h"$ is sufficient for matching?

Otherwise, @alekla01 might need to make this search a bit more generic, maybe something like the following:

Suggested change
- ! git grep '^#include "mbed.h"$' -- '*.c' '*.h' '*.cpp' '*.hpp' ':!*platform_mbed.h' ':!*TESTS/*' ':!TEST_APPS/' ':!UNITTESTS/' ':!*tests/*' ':!*targets/*' ':!*TARGET_*' ':!*unsupported/*'
- ! git grep '^#include.*mbed.h["']$' -- '*.c' '*.h' '*.cpp' '*.hpp' ':!*platform_mbed.h' ':!*TESTS/*' ':!TEST_APPS/' ':!UNITTESTS/' ':!*tests/*' ':!*targets/*' ':!*TARGET_*' ':!*unsupported/*'

Copy link
Contributor

Choose a reason for hiding this comment

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

@0xc0170 Could you confirm that astyle settings will guarantee that ^#include "mbed.h"$ is sufficient for matching?

I don't think it formats this. Thus between include and mbed should be "any space"

.travis.yml Outdated Show resolved Hide resolved
@cmonr
Copy link
Contributor

cmonr commented Jan 9, 2019

CI started

@cmonr
Copy link
Contributor

cmonr commented Jan 10, 2019

This PR needs a rebase before we can retest it.
A Travis CI fix has landed in master in the past 24 hours.

.travis.yml Outdated Show resolved Hide resolved
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 16, 2019

@alekla01 Before this goes to CI, can you squash these 5 commits into one: Add Travis include mbed.h check

@0xc0170 0xc0170 changed the title travis include_check travis: Add include_check job Jan 16, 2019
@alekla01 alekla01 force-pushed the alekla01-include_check-patch-1 branch 2 times, most recently from c9fcb4d to 8c437c7 Compare January 17, 2019 13:15
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 21, 2019

CI started

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants