BUILD_IN_DOCKER ignores USEMODULE

miri64 commented 4 years ago

Description

When building with docker, it is not possible to externally provide modules to an application. Where modifying the Makefile (e.g. in automated build environments) is not an option this is a big hindrance.

Steps to reproduce the issue

BUILD_IN_DOCKER=1 \
    DOCKER_ENV_VARS=USEMODULE \
    USEMODULE=gnrc_pktbuf_cmd \
        make -C examples/gnrc_networking

Expected results

The environment variable USEMODULE is included in the docker run command

docker run --rm --tty --user $(id -u) -v '/usr/share/zoneinfo/Europe/Berlin:/etc/localtime:ro' -v '~/Repositories/RIOT-OS/RIOT:/data/riotbuild/riotbase:delegated' -e 'RIOTBASE=/data/riotbuild/riotbase' -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' -e 'BUILD_DIR=/data/riotbuild/riotbase/build' -e 'RIOTPROJECT=/data/riotbuild/riotbase' -e 'RIOTCPU=/data/riotbuild/riotbase/cpu' -e 'RIOTBOARD=/data/riotbuild/riotbase/boards' -e 'RIOTMAKE=/data/riotbuild/riotbase/makefiles'      -e 'USEMODULE=gnrc_pktbuf_cmd'  -w '/data/riotbuild/riotbase/examples/gnrc_networking/' 'riot/riotbuild:latest' make    -j8

Actual results

The enviroment variable USEMODULE is not included in the docker run command

docker run --rm --tty --user $(id -u) -v '/usr/share/zoneinfo/Europe/Berlin:/etc/localtime:ro' -v '~/Repositories/RIOT-OS/RIOT:/data/riotbuild/riotbase:delegated' -e 'RIOTBASE=/data/riotbuild/riotbase' -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' -e 'BUILD_DIR=/data/riotbuild/riotbase/build' -e 'RIOTPROJECT=/data/riotbuild/riotbase' -e 'RIOTCPU=/data/riotbuild/riotbase/cpu' -e 'RIOTBOARD=/data/riotbuild/riotbase/boards' -e 'RIOTMAKE=/data/riotbuild/riotbase/makefiles'        -w '/data/riotbuild/riotbase/examples/gnrc_networking/' 'riot/riotbuild:latest' make    -j8

and the module is not compiled in (can be tested by using the pktbuf command)

> pktbuf
pktbuf
shell: command not found: pktbuf

Versions

Seems to be the case since docker was introduced, up until 2020.10-devel.

$ docker -v
Docker version 19.03.12-ce, build 48a66213fe

But also whatever docker is used with the ubuntu-latest platform in Github Actions.

miri64 commented 4 years ago

I can't just use DOCKER_ENVIROMENT_CMDLINE="-e USEMODULE=gnrc_pktbuf_cmd" unless I want to monkey patch around in the test framework for this special case :-/ (USEMODULE is set as an environment variable for the RIOTCtrl in the framework I'm using)

miri64 commented 4 years ago

On a completely different occasion I noticed: BINDIRBASE is also ignored. :(

miri64 commented 4 years ago

(I use it to generate different binaries for local size comparisons)

dylad commented 3 years ago

I took some look at this and I think there is several ways to fix this.

First of all, the reason why BINDIRBASE and USEMODULE aren't added to DOCKER_ENVIRONMENT_CMDLINE_AUTO is because of their 'origin'. the Makefile origin function tells us that BINDIRBASE is "override" and USEMODULE is "file". Thus, the filter command discards them because it expects them to come from either environment or command.

A quick fix toBINDIRBASE would be this:

From db618cd9ace847b2ef3b5b8b928a5fc79ed746b7 Mon Sep 17 00:00:00 2001
From: Dylan Laduranty <dylan.laduranty@mesotic.com>
Date: Fri, 17 Sep 2021 13:10:55 +0200
Subject: [PATCH] tmp

---
 Makefile.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.include b/Makefile.include
index 3ca75ca268..b14cc047cc 100644
--- a/Makefile.include
+++ b/Makefile.include
@@ -139,7 +139,7 @@ override RIOTTOOLS              := $(abspath $(RIOTTOOLS))
 override RIOTPROJECT            := $(abspath $(RIOTPROJECT))
 override APPDIR                 := $(abspath $(APPDIR))
 override BUILD_DIR              := $(abspath $(BUILD_DIR))
-override BINDIRBASE             := $(abspath $(BINDIRBASE))
+override BINDIRBASE             ?= $(abspath $(BINDIRBASE))
 override BINDIR                 := $(abspath $(BINDIR))
 override PKGDIRBASE             := $(abspath $(PKGDIRBASE))
 override DLCACHE_DIR            := $(abspath $(DLCACHE_DIR))
-- 
2.17.1

But I am really unsure about all the side effects...

Regarding USEMODULE, a quick fix is to call make with -e flag which will enforce the environnement override so the filter function will now see USEMODULE origin as "environnement override" again this might have side effects I am unaware

dylad commented 3 years ago

To give more insights on this, the issue with USEMODULE is that USEMODULE make variable is (most of the time) set in the Makefile application. In @miri64 's example, examples/gnrc_networking/Makefile defines a bunch of USEMODULE += foo and later include Makefile.include. Docker stuff will be parse early in Makefile.include but it's already too late as USEMODULE were already written. As soon as we hit a USEMODULE += foo in the Makefile application, the origin of USEMODULE will change to file even if we pass aUSEMODULE as make argument. Due to its origin, USEMODULE will never be include to docker build so we need to find a workaround.

Easiest way is to use the -e argument when calling make, this will force USEMODULE origin to environment override and thus it will be parse by docker makefile (if USEMODULE is DOCKER_ENV_VARS) but this should be documented somewhere as this is not obvious.
Second option would be to intercept the USEMODULE variable at the beginning of the Makefile application before the USEMODULE += foo stuff, store any USEMODULE given by command line, set a new variable which will be parse by docker makefile, then add it to DOCKER_ENVIRONMENT_CMDLINE for instance. This way, USEMODULE given from command line will be pass to docker build and there is no need to add USEMODULE to DOCKER_ENV_VARS. Unfortunately, we need to touch every Makefile application... For application within tests/ , this is pretty easy. We just have to do some magic in tests/Makefile.tests_common but we would have to duplicate this behavior for the examples/ folder.
Third option is to force USEMODULE to be added to DOCKER_ENVIRONMENT_CMDLINE every build unconditionally. This way docker makefile will set all USEMODULE for a given application then pass it to docker build which will in turn filter out the duplicate modules.
Any ideas ?

I can provide a PR but we need to choose an option before.

fjmolinas commented 3 years ago

@dylad I'll try yo take a look at this soon, dealing with some deadlines, in the meantime maybe @cladmi has some background on why this was left as is when first encountered?

There is this issue https://github.com/RIOT-OS/RIOT/issues/12027 where some context was give, I would need to re-check if issues mentioned there are still present.

cladmi commented 3 years ago

From memory only and without re-checking the current state.

Indeed your first solution is the simplest one and leaves it to the user.

For the second option, the "clean" way of doing it, is moving the USEMODULE that are in the application module in dedicated Makefile.deps and parse them at the dependency time. (same for a Makefile.include IIRC if there should be configuration depending on the dependencies). I did not do it as refactoring the features and moving dependency parsing before parsing Makefile.include took ages to get reviewed. I may not even have finished it myself, I am not sure.

For the third option, not sure if there are still any side-effects possible for this. It was at the beginning not possible because of the dependencies/cpu/board/Makefile.include inverse parsing order. For USEMODULE it could be ok now that dependencies are parsed first if the application makefiles do not do weird things. But not be for all variables.

For BINDIRBASE and all these variables, I did some poc removing the "override" and ":= abspath" replaced by a check if the variables were absolute but I think never went to proposing the command line API change https://github.com/cladmi/RIOT/commits/wip/pr/make/remove_var_override

fjmolinas commented 2 years ago

@dylad for USEMODULE option 3 seems like the easiest option I think it should work now... Makefile.include is not parsed after all dependencies.

In any case, we should combine this with option 1 since that solution works for call cases where we are passing environment variables, CFLAGS and others right?

dylad commented 2 years ago

Personally I'm fine with the third option.

In any case, we should combine this with option 1 since that solution works for call cases where we are passing environment variables, CFLAGS and others right?

I am wondering how we can document it properly as this is not something obvious.

fjmolinas commented 2 years ago

Personally I'm fine with the third option.

In any case, we should combine this with option 1 since that solution works for call cases where we are passing environment variables, CFLAGS and others right?

I am wondering how we can document it properly as this is not something obvious.

BUILD_IN_DOCKER is already quite verbose, how about just adding a $(info message) when it is set, there is also an entry in getting started on docker, it should probably got here I would say

dylad commented 2 years ago

BUILD_IN_DOCKER is already quite verbose, how about just adding a $(info message) when it is set, there is also an entry in getting started on docker, it should probably got here I would say

Then we can go ahead with this solution !

maribu commented 1 year ago

Cannot reproduce the issue anymore, apparently this was fixed and just forgotten to be closed.

RIOT-OS / RIOT