Skip to content

Fix EXTERNAL_MODULE_DIRS handling#16103

Closed
gdoffe wants to merge 1 commit intoRIOT-OS:masterfrom
cogip:gdo_fix_externals
Closed

Fix EXTERNAL_MODULE_DIRS handling#16103
gdoffe wants to merge 1 commit intoRIOT-OS:masterfrom
cogip:gdo_fix_externals

Conversation

@gdoffe
Copy link
Contributor

@gdoffe gdoffe commented Feb 26, 2021

Contribution description

All paths in EXTERNAL_MODULE_DIRS are added to DIRS, making them built even if they are not activated with USEMODULE.
Worst, their dependencies are parsed and activated too, leading to build non related modules to the original application.
To reproduce, comment "USEMODULE += external_module" in $(RIOTBASE)/tests/external_module_dirs/Makefile: the application build and it should not.
To fix this, filter EXTERNAL_MODULE_DIRS with USEMODULE variable.
To handle cases where module name is different from its folder name, use sed to extract module name from Makefile and try to find the module in USEMODULE.

I set this PR to draft in case someone could have a better solution.

Testing procedure

gdo@gdo-desktop:~/Developpement/Informatique/Personnel/cogip/RIOT$ make -C tests/external_module_dirs/
make : on entre dans le répertoire « /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/tests/external_module_dirs »
Building application "tests_external_module_dirs" 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/luid
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/sys/random
"make" -C /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/tests/external_module_dirs/external_module
   text	   data	    bss	    dec	    hex	filename
  29353	    628	  47820	  77801	  12fe9	/home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/tests/external_module_dirs/bin/native/tests_external_module_dirs.elf
make : on quitte le répertoire « /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/tests/external_module_dirs »

gdo@gdo-desktop:~/Developpement/Informatique/Personnel/cogip/RIOT$ sed -i "s/^USEMODULE/#USEMODULE/" tests/external_module_dirs/Makefile 

gdo@gdo-desktop:~/Developpement/Informatique/Personnel/cogip/RIOT$ make -C tests/external_module_dirs/
make : on entre dans le répertoire « /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/tests/external_module_dirs »
Building application "tests_external_module_dirs" for "native" with MCU "native".

/home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/tests/external_module_dirs/main.c:30:2: error: #error "Dependency not included"
   30 | #error "Dependency not included"
      |  ^~~~~
make[1]: *** [/home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/Makefile.base:107 : /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/tests/external_module_dirs/bin/native/application_tests_external_module_dirs/main.o] Erreur 1
make: *** [/home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/tests/external_module_dirs/../../Makefile.include:636 : application_tests_external_module_dirs.module] Erreur 2
make : on quitte le répertoire « /home/gdo/Developpement/Informatique/Personnel/cogip/RIOT/tests/external_module_dirs »

Issues/PRs references

I did not create an issue for that as I already have a fix, but I could.

All paths in EXTERNAL_MODULE_DIRS are added to DIRS, making them
built even if they are not activated with USEMODULE.
Worst, their dependencies are parsed and activated too, leading to
build non related modules to the original application.
To reproduce, comment "USEMODULE += external_module" in
$(RIOTBASE)/tests/external_module_dirs/Makefile: the application
build and it should not.
To fix this, filter EXTERNAL_MODULE_DIRS with USEMODULE variable.
To handle cases where module name is different from its
folder name, use sed to extract module name from Makefile and
try to find the module in USEMODULE.

Signed-off-by: Gilles DOFFE <g.doffe@gmail.com>
@gdoffe
Copy link
Contributor Author

gdoffe commented Feb 26, 2021

Arf, I forgot to set it as a draft -_-'.
Not sure I can change it myself.

@benpicco benpicco added Area: build system Area: Build system Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Feb 26, 2021
@maribu
Copy link
Member

maribu commented Feb 26, 2021

If I recall correctly the reported behavior is exactly as it is intended: If you want to use an external module, you have to add its path to EXTERNAL_MODULE_DIRS. If you don't want to use it, it shouldn't be added to EXTERNAL_MODULE_DIRS.

@benpicco
Copy link
Contributor

Ah but that's inconvenient.
To just being able to use USEMODULE += module like with in-tree modules, we have

MODULEDIR ?= $(abspath $(dir $(lastword $(MAKEFILE_LIST))))

KNOWN_EXTERNAL_MODULES = $(foreach md, $(dir $(wildcard $(MODULEDIR)/*/.)), $(shell basename $(md)))
EXTERNAL_MODULE_DIRS += $(addprefix $(MODULEDIR)/, $(filter $(KNOWN_EXTERNAL_MODULES), $(USEMODULE)))

in the folder with our external modules.

@maribu
Copy link
Member

maribu commented Feb 26, 2021

I'm toying around with changing the interface so that EXTERNAL_MODULE_DIRS are more like search paths for external modules, and modules will be located as subfolders of the dirs in EXTERNAL_MODULE_DIRS. This behavior would be consistent with how external boards work and would allow to specify EXTERNAL_MODULE_DIRS via ~./profile (or ~/.bashrc) once, and use USEMODULE to actually use external modules.

@gdoffe
Copy link
Contributor Author

gdoffe commented Feb 26, 2021

If I recall correctly the reported behavior is exactly as it is intended: If you want to use an external module, you have to add its path to EXTERNAL_MODULE_DIRS. If you don't want to use it, it shouldn't be added to EXTERNAL_MODULE_DIRS.

It is a bit more than that. Here is the original pull request:
#8987

Allow using EXTERNAL_MODULE_DIRS for complete modules with configuration and dependencies as if they were in RIOT.

For now it is not as they were in RIOT. If it is modules they have to be submitted to same rules than RIOT modules.
That means that these modules can be added or removed using USEMODULE.

Thats a real problem when you build in an external source tree as we do (https://github.com/cogip/mcu-firmware/tree/gdo_test_makefiles) with several possible configurations.

Moreover, using the fix, I can develop modules in the RIOT way making them easier to integrate (nearly a copy/paste).
Example of a 2D Lidar 360° LDS-01, while keeping my RIOT/ up to date without often rebasing:
https://github.com/cogip/mcu-firmware/tree/gdo_test_makefiles/drivers/lds01

Maybe EXTERNAL_MODULE_DIRS is something else, but in that case the example and naming does not follow the intent.

@maribu
Copy link
Member

maribu commented Feb 26, 2021

Allow using EXTERNAL_MODULE_DIRS for complete modules with configuration and dependencies as if they were in RIOT.

Well, that is certainly not the case with the implementation provided in that PR.

Could you take a look at #16104 ? That PR changes the interface to work similar to EXTERNAL_MODULE_DIRS in which you don't provide path to your module, but rather the path(s) to (a) folder(s) containing one or more external modules. Only modules actually used will be considered for building and dependency resolution.

@gdoffe
Copy link
Contributor Author

gdoffe commented Feb 26, 2021

Your PR seems really better than mine.
I close this one. ;)

@gdoffe gdoffe closed this Feb 26, 2021
@gdoffe gdoffe deleted the gdo_fix_externals branch February 26, 2021 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: build system Area: Build system Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants