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.

Sign up for GitHub

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

Jump to bottom

static_tests: add build system checks #10060

Merged
merged 3 commits into from
Nov 27, 2018
Merged

static_tests: add build system checks #10060

merged 3 commits into from
Nov 27, 2018

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Sep 27, 2018 •
edited
Loading

Contribution description

Add a script to execute sanity checks on build system files.
It should prevent bad patterns to re-appear after being cleaned.

Currently adds a check for using the content of FEATURES instead of
USEMODULE.

There is one case currently ignored as fixing it requires handing
CPU dependencies and the script could go in without it. Fixed by #10153

Fixes

This PR also include fixes for the test that can be split in separate PRs.

I think mainly the hifive1/fe310 changes should have a separate review as they are a bit specific.

Testing procedure

The script can be run directly with:

./dist/tools/buildsystem_sanity_check/check.sh

To test on master, it requires to cherry-picking just the commit adding the script and running it.

Issues/PRs references

It is part of issues found while working on #9913
The fatfs issue was also found in #9307

Depending on commits regarding hifive1 #10062 and the script split in #10179

@cladmi cladmi added Area: tests Area: tests and testing framework Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 27, 2018
@cladmi cladmi requested a review from jnohlgard September 27, 2018 15:24
@cladmi cladmi added the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 27, 2018
This was referenced Sep 27, 2018
Merged
Open
@cladmi cladmi requested a review from smlng October 4, 2018 15:17
@jcarrano jcarrano self-assigned this Oct 15, 2018
jcarrano
jcarrano previously approved these changes Oct 15, 2018
View reviewed changes
Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

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

It works, and I agree that the patterns listed are bad patterns.

@jcarrano
Copy link
Contributor

Why is murdock complaining that There is nothing to exclude from by :(exclude) patterns.?

@cladmi
Copy link
Contributor Author

cladmi commented Oct 15, 2018 •
edited
Loading

Hmm I should check on ubuntu as I developed it on arch.

@cladmi cladmi force-pushed the pr/tests/build_system_checks branch from 19c3855 to df43c07 Compare October 17, 2018 14:14
@cladmi
Copy link
Contributor Author

cladmi commented Oct 17, 2018

In git 2.7.4 from ubuntu 16.04, the pathspec exclude functionnality needs to have a inclusive pattern before https://stackoverflow.com/questions/10423143/how-to-exclude-certain-directories-files-from-git-grep-search/30084612#30084612

I updated the script accordingly.

@cladmi
Copy link
Contributor Author

cladmi commented Oct 17, 2018

Murdock does not complain about it anymore. Just the waiting for the other PR dependencies to be squashed.

I can the PR and re-order the commits to have parts integrated earlier if wanted.

Like adding the script alone and only enabling it here when everything is fixed.
And also split the other modules fixes in different PRs.

What do you think ?

@jcarrano
Copy link
Contributor

I think it is a good idea to add the tool and enable it on separate commits.

@cladmi cladmi mentioned this pull request Oct 17, 2018
Merged
@cladmi cladmi force-pushed the pr/tests/build_system_checks branch from b95621f to c78f241 Compare October 17, 2018 15:25
@cladmi
Copy link
Contributor Author

cladmi commented Oct 17, 2018

I rebased on top of #10179

@cladmi cladmi 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 Oct 17, 2018
@cladmi cladmi added this to the Release 2018.10 milestone Oct 21, 2018
@jcarrano jcarrano dismissed their stale review October 26, 2018 10:23

Dismissing my review now that the PR got split.

@cladmi cladmi force-pushed the pr/tests/build_system_checks branch from c78f241 to ed28cc9 Compare November 16, 2018 15:42
@cladmi
pkg/fatfs: fix declaration of periph_rtc optional dependency
c7de150 
* Declare optional dependency to periph_rtc
* Move CFLAGS definition to Makefile.include
@cladmi
sys/shell: select sc_rtc on USEMODULE
ebd438c 
sc_rtc.c should only be compiled if periph_rtc module is actually used.

In practice there was not linking error when PERIPH_OPTIONAL|_REQUIRED
was not set as shell_commands hides calling the functions with
'#ifdef MODULE_PERIPH_RTC'.
@cladmi cladmi force-pushed the pr/tests/build_system_checks branch from ed28cc9 to bcf0342 Compare November 19, 2018 15:01
@cladmi
Copy link
Contributor Author

cladmi commented Nov 19, 2018

Rebased since #10179 was merged.

I can split the fixes out if you want.

@jcarrano jcarrano removed the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 19, 2018
jcarrano
jcarrano approved these changes Nov 19, 2018
View reviewed changes
Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

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

All OK, build is passing, tool works.

@jcarrano jcarrano added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 19, 2018
@danpetry danpetry 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 Nov 20, 2018
@jcarrano jcarrano merged commit d3e9eec into RIOT-OS:master Nov 27, 2018
@cladmi cladmi deleted the pr/tests/build_system_checks branch November 27, 2018 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcarrano jcarrano jcarrano approved these changes

@jnohlgard jnohlgard Awaiting requested review from jnohlgard

@smlng smlng Awaiting requested review from smlng

Assignees

@jcarrano jcarrano

Labels
Area: build system Area: Build system Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2019.01
Development

Successfully merging this pull request may close these issues.
5 participants
@cladmi @jcarrano @miri64 @jia200x @danpetry