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

dist/tools: add build system sanity check script #10179

Merged

Conversation

Projects
None yet
4 participants
@cladmi
Copy link
Contributor

commented Oct 17, 2018

Contribution description

Split from #10060

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.

Modules should not check the content of FEATURES_PROVIDED/_REQUIRED/OPTIONAL
Handling specific behaviors/dependencies should by checking the content of:

  • USEMODULE
  • maybe FEATURES_USED if it is not a module (== not a periph_)

The script cannot currently be enabled by default as there are some fixes that are not included:
#10060

Testing procedure

The script correctly passes shellcheck without errors.

You can list the current errors by running:

./dist/tools/buildsystem_sanity_check/check.sh

It finds the current violations on using FEATURES_PROVIDED, FEATURES_OPTIONAL and FEATURES_REQUIRED to configure specific behavior.

./dist/tools/buildsystem_sanity_check/check.sh 
Invalid build system patterns found by dist/tools/buildsystem_sanity_check/check.sh:
boards/hifive1/Makefile.features:ifneq (,$(filter periph_rtc,$(FEATURES_REQUIRED)))
pkg/fatfs/Makefile.dep:ifneq (, $(filter periph_rtc, $(FEATURES_PROVIDED)))
sys/shell/commands/Makefile:ifneq (,$(filter periph_rtc,$(FEATURES_PROVIDED)))

With the fixes to these, it runs silently without errors as done in #10060

Issues/PRs references

Split from #10060 and is part of working on #9913

@jcarrano
Copy link
Contributor

left a comment

Fine. I left some suggestions to simplify it a bit.

@cladmi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

The question I still have, is should I change the way of handling errors. I took this one from another script but fear it will not scale correctly when adding other tests.

It would currently make all errors have only one header with Invalid build system patterns found by .

I was thinking about adding a sed that prepends some lines describing the type of errors after the git grep if it returns some text. What do you think ?

@jcarrano

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

It is not critical how we present it, as long as it is easy for the user to find and correct them.

This issue and the one with RIOTBASE, SCRIPTPATH, excluding the script file from the grep, etc are shared amongst all check scripts, so it may make more sense at some point to put all of that functionality in a shared file and source that from all the scripts.

@cladmi

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2018

I am not sure SCRIPTPATH is required by other tools as I am using it to not self match myself only.
But the RIOTBASE might indeed be.

Should I still change something with this PR for the moment ?

dist/tools: add build system sanity check script
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`.

Modules should not check the content of FEATURES_PROVIDED/_REQUIRED/OPTIONAL
Handling specific behaviors/dependencies should by checking the content of:
 * `USEMODULE`
 * maybe `FEATURES_USED` if it is not a module (== not a periph_)

@cladmi cladmi force-pushed the cladmi:pr/dist/tools/build_system/check branch from ae0bdb3 to 143e393 Nov 16, 2018

@MrKevinWeiss

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

I tested this PR and got:

./dist/tools/buildsystem_sanity_check/check.sh

Invalid build system patterns found by ./dist/tools/buildsystem_sanity_check/check.sh:
pkg/fatfs/Makefile.dep:ifneq (, $(filter periph_rtc, $(FEATURES_PROVIDED)))
sys/shell/commands/Makefile:ifneq (,$(filter periph_rtc,$(FEATURES_PROVIDED)))

When testing with #10060 it has no errors.

It does make sense to enforce structure in the build system but I cannot say much more than that.

@cladmi

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

Thank you for testing, and indeed, since I created the PR, the first error line was fixed!

@MrKevinWeiss What you can answer, is would you like more output if you get this error on a PR.
Or is it enough for you to go in the check.sh script to find the reason.

@jcarrano
Copy link
Contributor

left a comment

Tested:

$ ./dist/tools/buildsystem_sanity_check/check.sh 
Invalid build system patterns found by ./dist/tools/buildsystem_sanity_check/check.sh:
pkg/fatfs/Makefile.dep:ifneq (, $(filter periph_rtc, $(FEATURES_PROVIDED)))
sys/shell/commands/Makefile:ifneq (,$(filter periph_rtc,$(FEATURES_PROVIDED)))
@cladmi

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

@jcarrano thanks for ACKing, please just wait for @MrKevinWeiss feedback if he would like some changes now before merging.

@MrKevinWeiss

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

I think the output is sufficient. If I am editing a makefile and for some reason add a FEATURES_PROVIDED in the wrong section it specifies where it shouldn't be and what exactly it is. I was more stating that I don't understand the details of how this effects the build system and if there are conditions where the violation is warranted. Either way I think it is good and trust your ability so I too will ACK and go!

@MrKevinWeiss MrKevinWeiss merged commit 58500f9 into RIOT-OS:master Nov 19, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Murdock The build succeeded. runtime: 18m:39s
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cladmi

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

Thank you for reviewing ! 👍

@cladmi cladmi deleted the cladmi:pr/dist/tools/build_system/check branch Nov 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.