Makefile.include: initial features check before dependency resolution

[fjmolinas]

Contribution description

In #13349 the initial inclusion of features was inadvertently removed, it was kept in info-boards-supported but removed in Makefile.include. This PR re-introduces it.

Testing procedure

Found in #15011, with this PR you can check against FEATURES_USED:

Rebase on #15011 and apply this patch:

diff --git a/cpu/arm7_common/Makefile.dep b/cpu/arm7_common/Makefile.dep
index 004481beba..fabfff3190 100644
 # include common periph code
 USEMODULE += cortexm_common_periph
 
-ifneq (,$(filter picolibc,$(FEATURES_OPTIONAL) $(FEATURES_REQUIRED)))
+ifneq (,$(filter picolibc,$(FEATURES_USED)))
   # Use Picolibc when explicitly selected
   USEMODULE += picolibc
 else
diff --git a/cpu/fe310/Makefile.dep b/cpu/fe310/Makefile.dep
index 196cb33cca..99efb3df64 100644
--- a/cpu/fe310/Makefile.dep
+++ b/cpu/fe310/Makefile.dep
@@ -1,4 +1,4 @@
-ifneq (,$(filter picolibc,$(FEATURES_OPTIONAL) $(FEATURES_REQUIRED)))
+ifneq (,$(filter picolibc,$(FEATURES_USED)))
   USEMODULE += picolibc
 else
   USEMODULE += newlib_nano

BOARD=samr21-xpro FEATURES_OPTIONAL=picolibc make -C examples/gnrc_networking info-modules | grep picolibc
picolibc
picolibc_syscalls_default

For good measure ./dist/tools/buildsystem_sanity_check/save_all_dependencies_resolution_variables.sh could be run, but IMO
the fix is necessary and obvious.

Issues/PRs references

Introduced in #13349
Found in #15011

[benpicco]

Thank you for the fix!
Works as expected. Murdock will hopefully confirm that this has no bad side effects :)

[fjmolinas]

@benpicco I ended up running the script, please wait before merging.

[maribu]

How about this instead?

diff --git a/makefiles/dependency_resolution.inc.mk b/makefiles/dependency_resolution.inc.mk
index 827a1e17a6..52de81bf9e 100644
--- a/makefiles/dependency_resolution.inc.mk
+++ b/makefiles/dependency_resolution.inc.mk
@@ -5,12 +5,12 @@
 # Back up current state to detect changes
 OLD_STATE := $(USEMODULE) $(USEPKG) $(FEATURES_USED)
 
-# pull in dependencies of the currently used modules and pkgs
-include $(RIOTBASE)/Makefile.dep
-
 # check if required features are provided and update $(FEATURES_USED)
 include $(RIOTMAKE)/features_check.inc.mk
 
+# pull in dependencies of the currently used modules and pkgs
+include $(RIOTBASE)/Makefile.dep
+
 # translate used features into used module, where needed
 include $(RIOTMAKE)/features_modules.inc.mk

[fjmolinas]

How about this instead?

If this is done then:

 # Back up current state to detect changes
 OLD_STATE := $(USEMODULE) $(USEPKG) $(FEATURES_USED)

Would need to be changed to include FEATURES_%, so I think as it s is better.

[maribu]

Would need to be changed to include FEATURES_%, so I think as it s is better.

OK, that would be a pitty. However, I think that the additional feature check should be moved into the else part of ifeq (1,$(TEST_KCONFIG)).

[fjmolinas]

Would need to be changed to include FEATURES_%, so I think as it s is better.

OK, that would be a pitty. However, I think that the additional feature check should be moved into the else part of ifeq (1,$(TEST_KCONFIG)).

Also that cange makes Makefile.dep being included before Makefile.features, which is a bigger change than this one whitch is restoring what was already the case before.

And I agree with the Kconfig part, I forgot about that, think I'move the if higher up, @leandrolanzieri any reason why this could affect kconfig?:

# Include Kconfig functionalities
include $(RIOTMAKE)/kconfig.mk

# For testing, use TEST_KCONFIG as a switch between Makefile.dep and Kconfig
ifeq (1,$(TEST_KCONFIG))
  $(info === [ATTENTION] Testing Kconfig dependency modelling ===)
  KCONFIG_MODULES := $(call lowercase,$(patsubst CONFIG_MODULE_%,%,$(filter CONFIG_MODULE_%,$(.VARIABLES))))
  USEMODULE := $(KCONFIG_MODULES)
else
  # process provided features
  include $(RIOTBASE)/Makefile.features

  # check if required features are provided and update $(FEATURES_USED)
  include $(RIOTMAKE)/features_check.inc.mk

  # mandatory includes!
  include $(RIOTMAKE)/pseudomodules.inc.mk
  include $(RIOTMAKE)/defaultmodules.inc.mk

  # handle removal of default modules
  USEMODULE += $(filter-out $(DISABLE_MODULE), $(DEFAULT_MODULE))

  # process dependencies
  include $(RIOTMAKE)/dependency_resolution.inc.mk
endif

[leandrolanzieri]

And I agree with the Kconfig part, I forgot about that, think I'move the if higher up, @leandrolanzieri any reason why this could affect kconfig?:

Yes. kconfig.mk makes use of the make variable KCONFIG_ADD_CONFIG to list the default configuration files to be loaded. This variable is being set in Makefile.features files, so that inclusion should happen before.

[fjmolinas]

Yes. kconfig.mk makes use of the make variable KCONFIG_ADD_CONFIG to list the default configuration files to be loaded. This variable is being set in Makefile.features files, so that inclusion should happen before.

Huh? why is it done in Makefile.features and not in a kconfig file, anyway since this is out of scope I will just move the features.check into the ifelse.

[leandrolanzieri]

Huh? why is it done in Makefile.features and not in a kconfig file

The build system needs to know which .config files to merge and load into Kconfig. => it should happen before running Kconfig. This is basically to share configuration files among CPUs, archs, etc.

[fjmolinas]

The build system needs to know which .config files to merge and load into Kconfig. => it should happen before running Kconfig. This is basically to share configuration files among CPUs, archs, etc.

Ah right, sorry for the noise.

[fjmolinas]

Here are the results of running the script: diff.txt there are only some redundancy changes (expected with this change), but the sorted output has not changed. IMO this ready to be merged, @maribu I think your comment is addressed as well.

[fjmolinas]

Anyone willing to hit the green button? :)

[aabadie]

Me ?

[fjmolinas]

Thanks!