-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
makefiles/docker.inc.mk: Use directories in RIOT when possible #9646
makefiles/docker.inc.mk: Use directories in RIOT when possible #9646
Conversation
Another test is building RIOT Tutorials, with
Where in master I had cpu and board using absolute paths too
I added a sub variable Inside
And from an external application, I get the external 'riotproject':
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good improvement, but I can't do a proper review until I am back from holidays.
makefiles/docker.inc.mk
Outdated
# Add volumes for possible outside directories | ||
|
||
DOCKER_RIOT_DIRS = riotcpu riotboard riotmake riotproject | ||
# Names with uppercase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a better comment. I think this is a mapping from directory name inside the docker container to environment variable name for the path outside docker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, a better comment would help. My understanding is:
It is RIOT directories that can be remapped outside of the RIOT repository.
When mounting volumes in docker, if they are not in the RIOT repository, they need to be mounted as well to be accessible and the environment variable set to the mounted volume.
However, I just thought that it does not correctly handle if someone puts a CPU inside his RIOT repository in another directory. Because the environment variable should be set anyway.
I will fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this as there are not that many variables and was more confusing than anything.
8141d1b
to
99e8f35
Compare
I found out my previous solution was wrong as I was not setting RIOTCPU for variables inside the repository and it could be in a different directory than the original one. So now everything is based on a Then another function that takes care of doing the defines. I do not really know how to test external CPU/BOARD even more that right now they are not working that much. So I think if building normal applications still work and building an external application with external modules work it will be ok. |
Handled empty variable and and relative path, docker daemon does not like mounting volumes with non absolute path. |
Testing the
|
Not WIP anymore for me. |
When #8987 will get merged, there will be a test with one external module dir (not multiple but it is already something). |
@gebart do you mind continuing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are probably a few variables in docker.inc.mk which do not need to be exported. They date back to when I wrote the initial Docker integration a few years ago.
Tested locally with some external board and it seems to be working, but I don't fully understand all the template magic
makefiles/docker.inc.mk
Outdated
# | ||
# $1 = directories (can be a list of relative files) | ||
# $2 = docker remap base directory (defaults to DOCKER_BUILD_ROOT) | ||
# $3 = mapname (defaults to $(notdir $d)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is $d?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default it is the basename
of the directory in $1. I will update the docs.
makefiles/docker.inc.mk
Outdated
# | ||
# For each directory: | ||
# * if inside, returns $(DOCKER_RIOTBASE)/<relative_path_in_riotbase> | ||
# * if outside, returns <docker remapbase>/<mapname> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inside/outside what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inside and outside RIOTBASE
I will update
makefiles/docker.inc.mk
Outdated
DOCKER_VOLUMES_AND_ENV += $(call docker_volume_and_env,RIOTMAKE,,riotmake) | ||
|
||
# Remap external module directories. Not handled if they have common dirnames | ||
ifneq ($(words $(sort $(notdir $(EXTERNAL_MODULE_DIRS)))),$(words $(sort $(EXTERNAL_MODULE_DIRS)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this check. What does it do, and why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we are mapping directories outside of docker to directories in docker all stored in a common base directory, without copying the whole tree hierarchy, they could have collision in names:
EXTERNAL_MODULE_DIRS = relative/to/dir/a another/directory/also/called/a
We would then try to map both to external/a
.
For the documentation, the error
line may be clearer. I could duplicate it to the comment.
This is a limitation with my implementation, maybe I could do another mapping than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without copying the whole tree hierarchy
Would it make sense to copy the whole tree hierarchy? (in another PR of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could introduce difference between build machine when building in docker which is something I would not want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, though it would be useful for the user (who knows what kind of weird setup they may have).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, I think nobody uses EXTERNAL_MODULE_DIRS
and it was not even exported to docker before.
-e 'RIOTCPU=$(DOCKER_BUILD_ROOT)/riotcpu' \ | ||
-e 'RIOTBOARD=$(DOCKER_BUILD_ROOT)/riotboard' \ | ||
-e 'RIOTMAKE=$(DOCKER_BUILD_ROOT)/riotmake' \ | ||
-e 'RIOTPROJECT=$(DOCKER_BUILD_ROOT)/riotproject' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
$(DOCKER_ENVIRONMENT_CMDLINE) \ | ||
-w '$(DOCKER_BUILD_ROOT)/riotproject/$(BUILDRELPATH)' \ | ||
-w '$(DOCKER_APPDIR)' \ | ||
'$(DOCKER_IMAGE)' make $(DOCKER_MAKECMDGOALS) $(DOCKER_OVERRIDE_CMDLINE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An idea to the future: Propagate the job count from the top level make to the docker make: make -j9
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried in the past to get the job count from the main makefile but could not :/
I think we only have access to it from a sub make instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we have it from within the target execution environment though as it is passed to sub make instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed MAKEFLAGS
has it when in the execution environment, and we could extract with a $(filter -j%,$(MAKEFLAGS))
or alike.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some local tested solution for this, configured from outside so re-using DOCKER_MAKECMDGOALS
.
# Extract parallel option
# The number of jobs is only available after make 4.2
# The value will not be available in make but in the targets body
MAKE_PARALLEL = $(filter -j -j%,$(MAKEFLAGS))
DOCKER_MAKECMDGOALS += $(MAKE_PARALLEL)
We would get -j
even when saying -j2
for make before 4.2
so would only pass -j
in ubuntu bionic. But it could still be good.
I can update the first description in a generic way of what I do and why, independently of the actual template implementation. |
I pushed some documentation updates and renames. I did not tested them as I do not have docker on this computer. I will do when I have access to the other one. |
What's the hold-up on this already ACK'd PR? |
It's missing a squash and not it has a conflict. After I acked it we took a long detour (caused in great part by #10344 ) |
e0c7e25
to
cc6f02b
Compare
It represents the path of RIOTBASE inside the docker container.
Prepare for when it can have a different value.
Update definition order for DOCKER_VOLUMES_AND_ENV. * Localtime * Mapping and env related to `RIOTBASE` * Build directories * Project * CPU/BOARD/make
Return to which directory in the container this directory should be mapped.
Add functions to get volume and env arguments for a given directory environment variable. It handles: * variables with multiple directories like EXTERNAL_MODULE_DIRS * relative path * if the 'directories' variable is empty, it will not be exported to docker
Use RIOTPROJECT from within the riot repository if it is inside. This means when it is the case to use: * Not mounting the directory to `riotproject` * Use `APPDIR` relative to inside RIOT If it is not inside, do the same as before: * Mount the RIOTPROJECT to `riotproject` * Use `APPDIR` relative to RIOTPROJECT
Use BUILD_DIR from within the riot repository if it is inside.
Use the directories from in RIOT if possible for RIOTCPU/RIOTBOARD/RIOTMAKE.
cc6f02b
to
e03ad33
Compare
I rebased on the current master and resolved conflicts. I will re-run all the tests again. |
I could successfully re-run all the tests from #9646 (comment) With the same results. When mounting directories for |
They are remapped to `$(DOCKER_BUILD_ROOT)/external` if they are not inside RIOT (usually the case but not for `tests/external_modul_dirs`). If they are inside 'riotproject' they are currently also remapped to 'external'. The value of `EXTERNAL_MODULE_DIRS` is then enforced by configuring it on the command line as the application should not try to set it anymore. The remapping is done in `external/directory_name` so cannot handle multiple external directories with the same name.
e03ad33
to
3a17ddc
Compare
Sooo this seems ready... or is there something left to do? |
So within RIOT:
external:
|
Thank you for the review. |
Contribution description
When building with docker, only mount the directories that are outside of RIOT.
This makes RIOT_FILE_RELATIVE be correctly relative to RIOTBASE when possible.
It also handles correctly
EXTERNAL_MODULE_DIRS
.It fixes:
To
This fixes build size issues when
RIOT_FILE_RELATIVE
was used.I also fixed
BUILDRELPATH
when building indocker
for an example outside of RIOT to allow testing. (I could split this outside if wanted).Testing
Build
hello-world
withdocker
it should work and you can look at the list of-v
and-e
options given to docker.Copy
hello-world
outside ofRIOT
, updateRIOTBASE
in the makefile to point to RIOT and build with docker. It should also work and you can notice that a new-v
and-e
options are given to docker.I did not test with external cpu/board/make.
Now more advanced tests are described in these two comments:
Reviewing
I finished this a bit in a friday evening rush so would definitely need a second read.
Issues/PRs references
Tracking issue #9645
#9507