build system: Rework EXTERNAL_MODULE_DIRS#16104
Conversation
maribu
requested review from
MichelRottleuthner,
aabadie,
fjmolinas,
jia200x,
leandrolanzieri and
smlng
as code owners
maribu
added
Area: build system
|
Seems really good @maribu . :) Just a point, do you agree that makefiles/docker.inc.mk should be patched too to replace EXTERNAL_MODULE_DIRS by EXTERNAL_MODULE_PATHS ? gdo@gdo-desktop:~/Developpement/Informatique/Personnel/cogip/RIOT$ git diff ./makefiles/docker.inc.mk
diff --git a/makefiles/docker.inc.mk b/makefiles/docker.inc.mk
index 2e4e61f32f..2af7733743 100644
--- a/makefiles/docker.inc.mk
+++ b/makefiles/docker.inc.mk
@@ -253,7 +253,7 @@ DOCKER_VOLUMES_AND_ENV += $(if $(wildcard $(GIT_CACHE_DIR)),-e 'GIT_CACHE_DIR=$(
# Remap external module directories.
#
-# This remaps directories from EXTERNAL_MODULE_DIRS to subdirectories of
+# This remaps directories from EXTERNAL_MODULE_PATHS to subdirectories of
# $(DOCKER_BUILD_ROOT)/external
#
# Remapped directories must all have different basenames
@@ -261,10 +261,10 @@ DOCKER_VOLUMES_AND_ENV += $(if $(wildcard $(GIT_CACHE_DIR)),-e 'GIT_CACHE_DIR=$(
# Limitation: If a directory is inside RIOTPROJECT and not in RIOT it is
# remapped anyway instead of loading from inside RIOTPROJECT.
#
-# As EXTERNAL_MODULE_DIRS should ignore the 'Makefile' configuration, they must
+# As EXTERNAL_MODULE_PATHS should ignore the 'Makefile' configuration, they must
# be set using command line variable settings to not be modified within docker.
-DOCKER_VOLUMES_AND_ENV += $(call docker_volumes_mapping,$(EXTERNAL_MODULE_DIRS),$(DOCKER_BUILD_ROOT)/external,)
-DOCKER_OVERRIDE_CMDLINE += $(call docker_cmdline_mapping,EXTERNAL_MODULE_DIRS,$(DOCKER_BUILD_ROOT)/external,)
+DOCKER_VOLUMES_AND_ENV += $(call docker_volumes_mapping,$(EXTERNAL_MODULE_PATHS),$(DOCKER_BUILD_ROOT)/external,)
+DOCKER_OVERRIDE_CMDLINE += $(call docker_cmdline_mapping,EXTERNAL_MODULE_PATHS,$(DOCKER_BUILD_ROOT)/external,)
# Remap 'EXTERNAL_BOARD_DIRS' if they are external
DOCKER_VOLUMES_AND_ENV += $(call docker_volumes_mapping,$(EXTERNAL_BOARD_DIRS),$(DOCKER_BUILD_ROOT)/external,)
@@ -275,13 +275,13 @@ DOCKER_OVERRIDE_CMDLINE += $(call docker_cmdline_mapping,EXTERNAL_BOARD_DIRS,$(D
# External module directories sanity check:
#
# Detect if there are remapped directories with the same name as it is not handled.
-# Having EXTERNAL_MODULE_DIRS = /path/to/dir/name \
+# Having EXTERNAL_MODULE_PATHS = /path/to/dir/name \
# /another/directory/also/called/name
# would lead to both being mapped to '$(DOCKER_BUILD_ROOT)/external/name'
-_mounted_dirs = $(foreach d,$(EXTERNAL_MODULE_DIRS),$(if $(call dir_is_outside_riotbase,$(d)),$(d)))
+_mounted_dirs = $(foreach d,$(EXTERNAL_MODULE_PATHS),$(if $(call dir_is_outside_riotbase,$(d)),$(d)))
ifneq ($(words $(sort $(notdir $(_mounted_dirs)))),$(words $(sort $(_mounted_dirs))))
- $(warning Mounted EXTERNAL_MODULE_DIRS: $(_mounted_dirs))
- $(error Mapping EXTERNAL_MODULE_DIRS in docker is not supported for directories with the same name)
+ $(warning Mounted EXTERNAL_MODULE_PATHS: $(_mounted_dirs))
+ $(error Mapping EXTERNAL_MODULE_PATHS in docker is not supported for directories with the same name)
endif
# Handle worktree by mounting the git common dir in the same locationI do not know why this makefile is included as I do not use Docker ? I continue testing your PR, still having troubles but maybe on my side. |
|
a module cannot be renamed with "MODULE =" in Makefile, you confirm ? |
The current implementation locates them by name. It would be more involved to support mismatching folder and module names. I forgot that for internal modules this is supported, kind of. (For internal modules one has to manually extend This limitation should definitely be documented. Thanks for pointing this out. |
I haven't looked into docker integration yet. But your suggestion sounds good. But let's wait for some feedback on the interface change first. If that is agreed upon, I can fix the docker integration. |
The problem is I do neither use docker. ;) However, with patched docker.inc.mk that works very well for me with external modules outside of $(RIOTBASE). gdo@gdo-desktop:~/Developpement/Informatique/Personnel/cogip/mcu-firmware$ make -C examples/shell_menu/
make : on entre dans le répertoire « /home/gdo/Developpement/Informatique/Personnel/cogip/mcu-firmware/examples/shell_menu »
Building application "shell-menu" for "native" with MCU "native".
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/boards/native
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/boards/native/drivers
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/core
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/cpu/native
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/cpu/native/periph
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/cpu/native/stdio_native
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/drivers
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/drivers/periph_common
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/auto_init
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/ps
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/shell
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/shell/commands
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/mcu-firmware/sys/shell_menu
text data bss dec hex filename
38194 776 48092 87062 15416 /home/gdo/Developpement/Informatique/Personnel/cogip/mcu-firmware/examples/shell_menu/bin/native/shell-menu.elf
make : on quitte le répertoire « /home/gdo/Developpement/Informatique/Personnel/cogip/mcu-firmware/examples/shell_menu »
gdo@gdo-desktop:~/Developpement/Informatique/Personnel/cogip/mcu-firmware$ make -C examples/lds01/
make : on entre dans le répertoire « /home/gdo/Developpement/Informatique/Personnel/cogip/mcu-firmware/examples/lds01 »
Building application "lds01_example" for "nucleo-f446re" with MCU "stm32".
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/boards/nucleo-f446re
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/boards/common/nucleo
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/core
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/cpu/stm32
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/cpu/cortexm_common
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/cpu/cortexm_common/periph
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/cpu/stm32/periph
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/cpu/stm32/stmclk
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/cpu/stm32/vectors
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/drivers
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/drivers/periph_common
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/auto_init
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/div
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/event
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/isrpipe
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/malloc_thread_safe
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/newlib_syscalls_default
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/pm_layered
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/shell
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/stdio_uart
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/tsrb
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/xtimer
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/mcu-firmware/drivers/lds01
text data bss dec hex filename
17928 136 7048 25112 6218 /home/gdo/Developpement/Informatique/Personnel/cogip/mcu-firmware/examples/lds01/bin/nucleo-f446re/lds01_example.elf
[## ] 4% (18064 / 524288 Flash)
[ ] 1% (7184 / 134217728 RAM)
make : on quitte le répertoire « /home/gdo/Developpement/Informatique/Personnel/cogip/mcu-firmware/examples/lds01 » |
Waiting for EXTERNAL_MODULE_DIRS handling fix in RIOT upstream repository, use our own fork of RIOT with the cherry-picked fix[1]. This version is based on 2021.01 release. [1] RIOT-OS/RIOT#16104 Signed-off-by: Gilles DOFFE <g.doffe@gmail.com>
Waiting for EXTERNAL_MODULE_DIRS handling fix in RIOT upstream repository, use our own fork of RIOT with the cherry-picked fix[1]. This version is based on 2021.01 release. [1] RIOT-OS/RIOT#16104 Signed-off-by: Gilles DOFFE <g.doffe@gmail.com>
maribu
added
the
CI: ready for build
Interestingly, I couldn't reproduce the issue. I wonder if the issue was caused when using |
maribu
removed
the
CI: ready for build
maribu
added
the
CI: ready for build
Yes it is, even with DOCKER_BUILD=1. |
Waiting for EXTERNAL_MODULE_DIRS handling fix in RIOT upstream repository, use our own fork of RIOT with the cherry-picked fix[1]. This version is based on 2021.01 release. [1] RIOT-OS/RIOT#16104 Signed-off-by: Gilles DOFFE <g.doffe@gmail.com>
|
I like this PR a lot, it will need careful testing though, I'll take a closer look tomorrow. Regarding the semantics If I understand the same variable name is kept but it works differently? Will this work with setups using the previous workflow? |
Yes.
No, it will break existing setups. It would likely be possible to provide backward compatibility. I'm not sure if it is worth is. I don't expect many users of this features - it is more of a niche feature IMO. But all heavy users of this will likely like the new interface better and will be happy to migrate. Maybe it is best to ask in the forum. At the very least, it will help creating attention and prevent users being surprised by the change. |
| ***NOTE:*** The name of an external module must be unique (both in regard to other | ||
| external modules, as well to native RIOT modules). Additionally, the | ||
| directory containing the module must match the module name, e.g. | ||
| module `foo` must be located in `<PATH_IN_EXTERNAL_MODULE_DIRS>/foo`. |
There was a problem hiding this comment.
Having had a closer look at the PR I think this hard requirement is the main change that could potentially break external setups as well as make it harder to host bigger out-of-tree projects. I can think of any very good solutions though... @gdoffe used sed in his PR, but performing multiple shell calls is a bit slow.
At the same time EXTERNAL_MODULE_DIRS although less convenient actually maps what happens with Modules currently in RIOT regarding this, modules in RIOT are not automatically part of a search path either.
But this is only regarding compilation, Makefile inclusion inherently requires what is written in this note since it's based on rules such as: -include $(USEPKG:%=$(RIOTPKG)/%/Makefile.dep) or -include $(sort $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.dep)) or -include $(EXTERNAL_MODULE_DIRS:%=%/Makefile.dep), so if the module name does not match the directory name then no Makefile.% would get included. (Don't know if somewhere it says that a module with Makefile.% must have the same name as its directory for drivers).
So anyway we are already dealing with nondefault behavior to get some convenience, so trading one for another is fine if most agree, let's give you a forum post some time and then maybe add a Make warning for a while if there is EXTERNAL_MODULE_DIRS usage somewhere.
There was a problem hiding this comment.
Aren't unique module names already a requirement for RIOT modules?
Not mandating a flat hierarchy would be a plus - can we have multiple EXTERNAL_MODULE_DIRS though?
Then large projects could still have a hierarchy for cleanliness' sake.
There was a problem hiding this comment.
Aren't unique module names already a requirement for RIOT modules?
For the same build yes, but if there is no overlap, lets say between two different BOARD's, then both can have a module names periph. But here the important part is that directories - module would be 1:1, and although in the same build module names must be unique directories don't.
There was a problem hiding this comment.
can we have multiple EXTERNAL_MODULE_DIRS though?
Sure :-) the same already applies to EXTERNAL_BOARD_DIRS. I have one Repo with work boards, and one repo with home grown private boards in it :-)
jue89
left a comment
jue89
left a comment
There was a problem hiding this comment.
Sweet! Works perfectly with my application.
The reason was stupid me introducing a bug while cleaning up the code :-/ I fixed the issue and squashed right away. |
benpicco
added
Area: tools
|
Somehow the static-tests script is not coming to a conclusion |
It timed out . I can't see any reason for this. Maybe we just try to rerun them? |
This is weird, it should take around 3-4mins, and its been going on for 10, there must be an issue. |
|
Looks like cppcheck is running in and endless loop now :/ |
|
Locally, cppcheck terminates. Maybe I should check again using the riotbuild docker image. |
I ran it in docker and it still terminates for me... @maribu can you force push a change in the commit message or something? |
Actually, I think it's vera++ and not cppcheck. |
I think the files excluded need to be adapted with the renamins. |
|
@maribu can you try with this patch? diff --git a/dist/tools/vera++/check.sh b/dist/tools/vera++/check.sh
index ce74912d48..61a5a7158b 100755
--- a/dist/tools/vera++/check.sh
+++ b/dist/tools/vera++/check.sh
@@ -16,7 +16,7 @@ CURDIR=$(cd "$(dirname "$0")" && pwd)
# tests/pkg_utensor/models/deep_mlp_weight.hpp is an auto-generated file
# with lots of commas so T009 takes very long. Since it is auto-generated, just
# exclude it.
-EXCLUDE='^(.+/vendor/|dist/tools/coccinelle/include|dist/tools/fixdep/fixdep.c|dist/tools/lpc2k_pgm/src|tests/pkg_utensor/models)'
+EXCLUDE='^(.+/vendor/|dist/tools/coccinelle/include|dist/tools/fixdep/fixdep.c|dist/tools/lpc2k_pgm/src|tests/pkg_utensor/external_modules/models)'
FILES=$(changed_files)
if [ -z "${FILES}" ]; then |
Previously, external modules had to be individually added to both EXTERNAL_MODULE_DIRS and USEMODULE. If those where not in sync, this resulted in build errors. With this commit, search folders for external modules are added to EXTERNAL_MODULE_DIRS instead. So lets say the file system structure is like this ``` └── /path/to/external/modules ├── mod_a │ ├── Makefile │ ├── Makefile.dep │ ├── Makefile.include │ ├── foo.c │ └── include │ └── external_module.h └── mod_b ├── Makefile └── bar.c ``` One now adds `/path/to/external/modules` to EXTERNAL_MODULES and only with `USEMODULE += mod_a` the corresponding module, dependencies and include settings are actually used. Hence, it is possible to configure `EXTERNAL_MODULE_DIRS` from `~/.profile` or `~/.bashrc` once and never needs to worry about them again.
|
Gooo! |
|
Thanks everyone for the feedback and special thanks to @fjmolinas for figuring out the issue with vera++! |
Waiting for EXTERNAL_MODULE_DIRS handling fix in RIOT upstream repository, use our own fork of RIOT with the cherry-picked fix[1]. This version is based on 2021.01 release. [1] RIOT-OS/RIOT#16104 Signed-off-by: Gilles DOFFE <g.doffe@gmail.com>
|
A bit too late but I realize this broke dependency handling for EXTERNAL_MODULE when using Kconfig since |
I think I have the fix though. |
7 participants
Contribution description
Previously, external modules had to be individually added to both
EXTERNAL_MODULE_DIRS and USEMODULE. If those where not in sync, this
resulted in build errors.
With this commit, search folders for external modules are added to
EXTERNAL_MODULE_DIRS instead. So lets say the file system structure is
like this
One now adds
/path/to/external/modulesto EXTERNAL_MODULES and onlywith
USEMODULE += mod_athe corresponding module, dependencies andinclude settings are actually used. Hence, it is possible to configure
EXTERNAL_MODULE_DIRSfrom~/.profileor~/.bashrconce and neverneeds to worry about them again.
Testing procedure
The test application was updated to allow testing the new feature. But some packages also use this and have not been adapted. This is intended to be done if the general idea is approved.
Issues/PRs references
Alternative to #16103