Murdock results

✔️ PASSED

d4c229e buildsystem: include Makefile.{dep,include} for application modules

Artifacts

• Documentation preview

Can you give a bit more context on what you want to achieve?

I don't think that the code you proposed actually does what it should:

-include $(sort $(USEMODULE:%=$(APPDIR)/%/Makefile.dep))

# with `USEMODULE = ztimer_msec periph_uart ...` this would be:
-include $(APPDIR)/ztimer_msec/Makefile.dep
-include $(APPDIR)/periph_uart/Makefile.dep
...

Since #20024 you might be having modules in a subfolder in you application folder.
If it has a Mekefile.include and Makefile.dep, they must be pulled by RIOT to handle dependency resolution.

That was actually not the intended use-case of #20024. If you want to have RIOT-style modules in subfolders, just add the top-directory to EXTERNAL_MODULE_DIRS. #20024 was about having subfolders without ending up with the boilerplate of additional RIOT modules.

But this:

# Add subfolders as modules
DIRS += module
USEMODULE += my_module # name as defined in module/Makefile

is done in examples/basic/subfolders.
And there, if there was a Makefile.dep or Makefile.include in /module, it would not be considered.

-include $(APPDIR)/ztimer_msec/Makefile.dep
-include $(APPDIR)/periph_uart/Makefile.dep

Your app directory will not have a module ztimer_msec or periph_uart. Because of the -include, make does not care that the files dont exist.

Because of the -include, make does not care that the files dont exist.

While that is generally true, IMO it does not make sense to try to include many dozens or hundreds of Makefiles that don't exist.

For example, tests/sys/shell on native64 has 67 entries in the USEMODULE variable, Make will try at least twice to include all the files that don't exist (once for .dep and once for .include). That are 134 bogus file requests.
I say at least because for example the Makefile.dep can be called multiple times, for the tests/sys/shell example, it will be called three times. That is a total of 268 file requests.

An approach similar to #21331 would be possible, but considering that you have to include all the source files of the subfolders manually anyway, wouldn't it make sense to just include the Makefile.dep and Makefile.include as well at that point?

wouldn't it make sense to just include the Makefile.dep and Makefile.include as well at that point?

i tried that before I created the PR, to hardcode inclusion of my app module Makefile.dep and Makefile.include.
But neither before nor after include $(RIOTBASE)/Makefile.include lead to proper dependency resolution.

When my module M requires module A from RIOT, module A wass pulled in.
But if module A requires RIOT module B, module B was not pulled in.

Can you create an example structure and attach it? It's a bit hard to tell if that's intended or unintended behavior and what's the best way to approach it.

My particular use case is this Makefile.dep in a module subfolder rt_config.

ifneq (,$(filter at86rf215,$(USEMODULE)))
  USEMODULE += rt_config_at86rf215
endif

ifneq (,$(filter sx126x,$(USEMODULE)))                               # any sx126x used?
  USEMODULE += rt_config_sx126x                                    # use application module
  USEMODULE += netdev_ieee802154_submac_legacy      # this is irrelevant for this PR
  USEMODULE += sx126x_ieee802154                                # want to use ieee802154 radio hal on sx126x
  USEMODULE += bhp                                                        # required for the radio hal 
endif

I run my app with USEMODULE+=sx1262. Which network driver is pulled in depends on what is present on the board.
Without this PR I do not get the dependencies for sx126x_ieee802154, which are netdev_ieee802154_submac and bhp_event.
So I guess when my application dependency is evaluated before the RIOT buildsystem has figured out that actually sx126x is used, then I do not get any of my dependencies. And when my application dependencies are pulled in after the include of RIOTs Makefile.include in the application Makefile, then I get my dependencies, but not the dependencies of my dependencies.

This is very specific I admit, and I could not come up with and example using upstream RIOT code.
But still, it makes a difference whether the application module dependencies are resolved in the RIOT build process or in the application Makefile.

I know there are workarounds, like I could also specify all the dependencies of may dependencies by hand or specify USEMODULE+=sx126x additionally to sx1262, but I am not looking for workarounds.
I would not see how the PR harms the buildsystem. You can as well compare it with EXTERNAL_MODULE_PATHS for which `Makefile.dep is pulled in.

# include external modules dependencies
# processed before RIOT ones to be evaluated before the 'default' rules.
-include $(EXTERNAL_MODULE_PATHS:%=%Makefile.dep)

I think in your case, createing a Makefile.board.dep file in the application directory would be the best option.

Adding USEMODULE += sx1262 to the examples/basic/subfolders/Makefile and creating an examples/basic/subfolders/Makefile.board.dep with $(warning $(USEMODULE)) as content leads to the following output:

chris@W11nMate:~/RIOTstuff/riot-various/RIOT/examples/basic/subfolders$ make
using BOARD="native64" as "native" on a 64-bit system
/home/chris/RIOTstuff/riot-various/RIOT/examples/basic/subfolders/Makefile.board.dep:1: board board_common_init core core_init core_lib core_msg core_panic core_thread cpu libc my_module sx1262 sys
/home/chris/RIOTstuff/riot-various/RIOT/examples/basic/subfolders/Makefile.board.dep:1: board board_common_init boards_common_native core core_idle_thread core_init core_lib core_msg core_panic core_thread cpu fdcan iolist libc lora my_module native_drivers netdev_legacy_api periph periph_common periph_gpio periph_gpio_irq periph_pm periph_uart stdio stdio_default stdio_native sx1262 sx126x sys
/home/chris/RIOTstuff/riot-various/RIOT/examples/basic/subfolders/Makefile.board.dep:1: board board_common_init boards_common_native core core_idle_thread core_init core_lib core_msg core_panic core_thread cpu driver_sx126x_hal fdcan iolist libc lora my_module native_drivers netdev_legacy_api periph periph_common periph_gpio periph_gpio_irq periph_gpio_linux periph_pm periph_spi periph_uart stdio stdio_default stdio_native sx1262 sx126x sys ztimer ztimer_usec
/home/chris/RIOTstuff/riot-various/RIOT/examples/basic/subfolders/Makefile.board.dep:1: board board_common_init boards_common_native core core_idle_thread core_init core_lib core_msg core_panic core_thread cpu driver_sx126x_hal fdcan frac iolist libc lora my_module native_drivers netdev_legacy_api periph periph_common periph_gpio periph_gpio_irq periph_gpio_linux periph_pm periph_spi periph_spidev_linux periph_timer periph_timer_query_freqs periph_uart stdio stdio_default stdio_native sx1262 sx126x sys ztimer ztimer_convert ztimer_convert_frac ztimer_convert_shift ztimer_core ztimer_extend ztimer_periph_timer ztimer_usec
Building application "subfolders" for "native64" with CPU "native".

"make" -C /home/chris/RIOTstuff/riot-various/RIOT/pkg/driver_sx126x/
...

As you can see, the Makefile.board.dep is called (multiple times) during the dependency resolution and also includes the sx126x module.

Somewhat unrelated to this PR, but the option to create a Makefile.board.dep for an application should be documented in the Creating an Application documentation.

I would not see how the PR harms the buildsystem

Harm is a strong word, but the build system is kind of a mess already and I would like to avoid adding things that might make the performance even worse.

For example, this is what this PR adds to the Debug trail of the Make process:

cbuec@W11nMate:~/RIOTstuff/riot-vanilla/RIOT$ git checkout pr/subfolder_module_include_dep
Switched to branch 'pr/subfolder_module_include_dep'
cbuec@W11nMate:~/RIOTstuff/riot-vanilla/RIOT$ make -C examples/basic/subfolders -d > pr.txt
using BOARD="native64" as "native" on a 64-bit system
cbuec@W11nMate:~/RIOTstuff/riot-vanilla/RIOT$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
cbuec@W11nMate:~/RIOTstuff/riot-vanilla/RIOT$ make -C examples/basic/subfolders -d > master.txt
using BOARD="native64" as "native" on a 64-bit system

That was actually not the intended use-case of #20024. If you want to have RIOT-style modules in subfolders, just add the top-directory to EXTERNAL_MODULE_DIRS

You were right. I only added EXTERNAL_MODULE_DIRS += $(CURDIR) to my application Makefile and this also solves my issue.

I like this more than Makefile.board.dep, but it could have been a solution as well.

But this:

# Add subfolders as modules
DIRS += module
USEMODULE += my_module # name as defined in module/Makefile

is done in examples/basic/subfolders. And there, if there was a Makefile.dep or Makefile.include in /module, it would not be considered.

Maybe we should reconsider this example then if it leads people to wrong conclusions 🤔

Maybe we should reconsider this example then if it leads people to wrong conclusions 🤔

There we go: #21476