Skip to content

ci/build_system_check: improve check for features only provided in Makefile.features#14511

Merged
fjmolinas merged 1 commit intoRIOT-OS:masterfrom
aabadie:pr/ci/build_system_check_makefile_features
Jul 20, 2020
Merged

ci/build_system_check: improve check for features only provided in Makefile.features#14511
fjmolinas merged 1 commit intoRIOT-OS:masterfrom
aabadie:pr/ci/build_system_check_makefile_features

Conversation

@aabadie
Copy link
Contributor

@aabadie aabadie commented Jul 14, 2020

Contribution description

This PR improves the build system sanity checks to be able to detect the use of FEATURES_PROVIDED in files other than Makefile.features: it nows checks for path other than only cpu/boards and it take into account potential spaces at the beginning of lines where it is used.

This allows for catching things like:

FEATURES_PROVIDED += periph_rtc

Testing procedure

$ ./dist/tools/buildsystem_sanity_check/check.sh 
Invalid build system patterns found by ./dist/tools/buildsystem_sanity_check/check.sh:
Features in cpu and boards should only be provided in Makefile.features files
	drivers/Makefile.dep:  FEATURES_PROVIDED += periph_rtc
  • One can also apply the following patches to verify there's no regression in the check:
Details
diff --git a/boards/6lowpan-clicker/Makefile.dep b/boards/6lowpan-clicker/Makefile.dep
index 2fa80dd76e..74b92b3e78 100644
--- a/boards/6lowpan-clicker/Makefile.dep
+++ b/boards/6lowpan-clicker/Makefile.dep
@@ -3,3 +3,5 @@ ifneq (,$(filter saul_default,$(USEMODULE)))
 endif
 
 USEMODULE += newlib_syscalls_mips_uhi
+
+FEATURES_PROVIDED += periph_uart
diff --git a/boards/6lowpan-clicker/Makefile.include b/boards/6lowpan-clicker/Makefile.include
index 9ac719b33b..e9e449090c 100644
--- a/boards/6lowpan-clicker/Makefile.include
+++ b/boards/6lowpan-clicker/Makefile.include
@@ -14,3 +14,5 @@ ifeq ($(PROGRAMMER),pic32prog)
   FLASHFILE ?= $(HEXFILE)
   include $(RIOTMAKE)/tools/pic32prog.inc.mk
 endif
+
+FEATURES_PROVIDED += periph_uart

results in:

./dist/tools/buildsystem_sanity_check/check.sh 
Invalid build system patterns found by ./dist/tools/buildsystem_sanity_check/check.sh:
Features should only be provided in Makefile.features files
	boards/6lowpan-clicker/Makefile.dep:FEATURES_PROVIDED += periph_uart
	boards/6lowpan-clicker/Makefile.include:FEATURES_PROVIDED += periph_uart
	drivers/Makefile.dep:  FEATURES_PROVIDED += periph_rtc
git diff
diff --git a/cpu/stm32/Makefile.dep b/cpu/stm32/Makefile.dep
index 56bbbc945b..96a3abc50b 100644
--- a/cpu/stm32/Makefile.dep
+++ b/cpu/stm32/Makefile.dep
@@ -17,3 +17,5 @@ ifneq (,$(filter periph_uart_nonblocking,$(USEMODULE)))
 endif
 
 include $(RIOTCPU)/cortexm_common/Makefile.dep
+
+FEATURES_PROVIDED += periph_wdt
diff --git a/cpu/stm32/Makefile.include b/cpu/stm32/Makefile.include
index b0f9a4a681..61057737fd 100644
--- a/cpu/stm32/Makefile.include
+++ b/cpu/stm32/Makefile.include
@@ -46,3 +46,5 @@ endif
 VECTORS_O ?= $(BINDIR)/stm32_vectors/vectors_$(CPU_FAM).o
 
 include $(RIOTMAKE)/arch/cortexm.inc.mk
+
+FEATURES_PROVIDED += periph_wdt

results in:

./dist/tools/buildsystem_sanity_check/check.sh 
Invalid build system patterns found by ./dist/tools/buildsystem_sanity_check/check.sh:
Features should only be provided in Makefile.features files
	cpu/stm32/Makefile.dep:FEATURES_PROVIDED += periph_wdt
	cpu/stm32/Makefile.include:FEATURES_PROVIDED += periph_wdt
	drivers/Makefile.dep:  FEATURES_PROVIDED += periph_rtc

Issues/PRs references

Will work when #14497 is merged

@aabadie aabadie added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: CI Area: Continuous Integration of RIOT components CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Jul 14, 2020
@aabadie aabadie changed the title ci/build_system_check: improve check for features provided in Makefile.features ci/build_system_check: improve check for features only provided in Makefile.features Jul 14, 2020
@aabadie aabadie requested a review from fjmolinas July 14, 2020 14:34
@fjmolinas
Copy link
Contributor

Is there any reason for features to be provided by something else than BOARD'S or CPU'S?

@aabadie
Copy link
Contributor Author

aabadie commented Jul 15, 2020

Is there any reason for features to be provided by something else than BOARD'S or CPU'S?

Apparently, there's still the question about modules that could provide features. I noticed something of this kind was merged into master for rtt_rtc:

FEATURES_PROVIDED += periph_rtc

This PR adds a check to prevent that.

@fjmolinas
Copy link
Contributor

Discussed offline with @aabadie, I had an issue with the change since currently FEATURES should only be provided by BOARDS and CPUS. Since currently Makefile.feature only exist for those this change is OK.

…e.features

- Take into account potential spaces at the beginning of a line
- Check all Makefile.<something> files, not only the ones in boards and cpus
@aabadie aabadie force-pushed the pr/ci/build_system_check_makefile_features branch from 7c0b19d to b7f2ad8 Compare July 20, 2020 11:06
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 20, 2020
@aabadie
Copy link
Contributor Author

aabadie commented Jul 20, 2020

rebased now that #14497 is merged. It should pass the CI now.

@aabadie
Copy link
Contributor Author

aabadie commented Jul 20, 2020

ping @fjmolinas

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK

@fjmolinas fjmolinas merged commit e8a8d12 into RIOT-OS:master Jul 20, 2020
@aabadie aabadie deleted the pr/ci/build_system_check_makefile_features branch July 20, 2020 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: CI Area: Continuous Integration of RIOT components CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants