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

dist/buildsystem_sanity_check: add check for CPU/CPU_MODEL migration #12337

Merged
merged 3 commits into from
Oct 1, 2019
Merged

dist/buildsystem_sanity_check: add check for CPU/CPU_MODEL migration #12337

merged 3 commits into from
Oct 1, 2019

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Sep 30, 2019 •
edited
Loading

Contribution description

Static tests to ensure.

  • CPU Makefile.include/Makefile.features/Makefile.dep must not be
    included by the board directly anymore.

  • CPU and CPU_MODEL variables must now be defined by
    boards/**/Makefile.features files instead of `**/Makefile.include.

  • CPU is defined by board Makefile.features with a warning

This currently blacklist the slwstk6000b and cortexm.inc.mk that still define
'CPU_MODEL' in a file that is not a 'Makefile.features.
This allows getting this sanity check script in master

Depends on

This is currently only warning during build that CPU is not defined. I have commits in #11477 to enforce it, not sure if we want it already or not.

Testing procedure

The static tests do not fail, and no warning for any board regarding defining CPU.

When reverting both depending commits and removing the blacklist lines, the forbidden outputs are matched:

./dist/tools/buildsystem_sanity_check/check.sh
Invalid build system patterns found by ./dist/tools/buildsystem_sanity_check/check.sh:
Makefiles files from cpu must not be included by the board anymore
        boards/common/blxxxpill/Makefile.features:include $(RIOTCPU)/stm32f1/Makefile.features
        boards/p-l496g-cell02/Makefile.features:include $(RIOTCPU)/stm32l4/Makefile.features
        boards/samr34-xpro/Makefile.features:include $(RIOTCPU)/saml21/Makefile.features
        boards/slwstk6000b/Makefile.dep:include $(RIOTCPU)/efm32/Makefile.dep
        boards/slwstk6000b/Makefile.features:include $(RIOTCPU)/efm32/Makefile.features
        boards/stm32f723e-disco/Makefile.features:include $(RIOTCPU)/stm32f7/Makefile.features
CPU and CPU_MODEL definition must be done by board/BOARD/Makefile.features or board/common/**/Makefile.features
        boards/slwstk6000b/Makefile.include:export CPU = efm32
        boards/slwstk6000b/Makefile.include:export CPU_MODEL = $(MODULE_CPU)
        makefiles/arch/cortexm.inc.mk:export CPU_MODEL ?= $(CPU)

And the warning is triggered:

for board in $(make info-boards); do ESP32_SDK_DIR=/tmp ESP8266_SDK_DIR=/tmp ESP8266_NEWLIB_DIR=/tmp PORT=/dev/null BOARD=${board} make --n
o-print-directory -C examples/hello-world/ clean; done
/home/harter/work/git/RIOT/Makefile.features:18: CPU must be defined by board / board_common Makefile.features

Issues/PRs references

Depends on

Part of Tracking: move CPU/CPU_MODEL to Makefile.features #11477

This was referenced Sep 30, 2019
Closed
Merged
@cladmi
Copy link
Contributor Author

cladmi commented Sep 30, 2019

This one has overlap with #12338 as it fixes the need for a blacklist of cortexm.inc.mk. The one that goes second should remove the blacklist line.

@benpicco benpicco added Area: build system Area: Build system State: waiting for other PR State: The PR requires another PR to be merged first Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Sep 30, 2019
@cladmi cladmi force-pushed the pr/make/cpu/makefile_features/static_test branch from 6eb31e0 to 7aa9dd4 Compare October 1, 2019 09:36
cladmi added 3 commits October 1, 2019 15:57
@cladmi
dist/buildsystem_sanity_check: add check for included CPU Makefiles
7064f7e 
CPU Makefile.include/Makefile.features/Makefile.dep must not be
included by the board directly anymore.
@cladmi
dist/buildsystem_sanity_check: add check for CPU/CPU_MODEL definition
cbfab67 
CPU and CPU_MODEL variables must now be defined by
`boards/**/Makefile.features` or 'cpu/CPU/Makefile.features' files
instead of `**/Makefile.include.

Currently blacklist the slwstk6000b that still define
'CPU_MODEL' in a file that is not a 'Makefile.features.
This allows getting this sanity check script in master

This allow using the variables when parsing dependencies.
@cladmi
Makefile.features: warning if CPU not defined by BOARD/Makefile.features
4277624 
CPU must be defined by `$(RIOTBOARD)/$(BOARD)/Makefile.features` or
one of its common included Makefile.features file.

This currently only adds a warning but not prevents building for the
moment.
@cladmi cladmi force-pushed the pr/make/cpu/makefile_features/static_test branch from 7aa9dd4 to 4277624 Compare October 1, 2019 14:03
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 1, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Oct 1, 2019

I rebased and adapted now that #12338 is merged.

I update the check as now CPU_MODEL is defined by cpu/CPU/Makefile.features for the cpu/lpc1768/Makefile.features.
It made sense since the beginning but was not required at first so was stricter.

The documentation and the test do not precise that CPU must be defined by the board and only the CPU_MODEL might be set by the cpu, but it would not make sense otherwise anyway. I could adapt if it is not clear enough.

@benpicco benpicco 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 1, 2019
@cladmi cladmi mentioned this pull request Oct 1, 2019
Merged
benpicco
benpicco approved these changes Oct 1, 2019
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

This adds a compile-time check to ensure all board Makefiles are following the standard.
Murdock just confirmed all boards pass that check.

This prevents new board additions from violating those standards.

@benpicco benpicco merged commit 29de470 into RIOT-OS:master Oct 1, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Oct 2, 2019

Thank you for the review.

@cladmi cladmi deleted the pr/make/cpu/makefile_features/static_test branch October 2, 2019 09:04
@kb2ma kb2ma added this to the Release 2019.10 milestone Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

Assignees
No one assigned
Labels
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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.
3 participants
@cladmi @benpicco @kb2ma