Makefile.include: add BUILD_FILES variable that holds all files to be built by fjmolinas · Pull Request #12302 · RIOT-OS/RIOT

fjmolinas · 2019-09-25T09:04:39Z

Contribution Description

This PR adds a BUILD_FILES variable to hold all files that need to be linked. This is an attempt to fix #12003.

By default BUILD_FILES will hold $(ELFFILE), $(FLASHFILE) but more dependencies can be added as for example SLOT_RIOT_ELFS. This ensures the added targets are also built in docker when required.

There are other possible solutions, e.g:

ifneq (1, $(RIOTNOLINK))
  ifneq (1, $(BUILD_IN_DOCKER))
  link: $(SLOT_RIOT_ELFS)
  endif
endif

The first approach is better IMO as it doesn't mix in docker logic into riotboot.mk and the built files are correctly defined.

PS: I'm unsure about the name of the variable.

Testing procedure

Building in docker now succeeds and doesn't use the local toolchain.

BUILD_IN_DOCKER=1 DOCKER="sudo docker" BOARD=frdm-k64f make --no-print-directory -C bootloaders/riotboot/

Unknown device, --name=, --path=, or absolute path in /dev/ or /sys expected.
Launching build container using image "riot/riotbuild:latest".
sudo docker run --rm -t -u "$(id -u)" \
    -v '/usr/share/zoneinfo/Europe/Paris:/etc/localtime:ro' -v '/home/francisco/workspace/RIOT:/data/riotbuild/riotbase' -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 'BOARD=frdm-k64f' \
    -w '/data/riotbuild/riotbase/bootloaders/riotboot/' \
    'riot/riotbuild:latest' make   
[sudo] password for francisco: 
Building application "riotboot" for "frdm-k64f" with MCU "kinetis".

"make" -C /data/riotbuild/riotbase/boards/frdm-k64f
"make" -C /data/riotbuild/riotbase/core
"make" -C /data/riotbuild/riotbase/cpu/kinetis
"make" -C /data/riotbuild/riotbase/cpu/cortexm_common
"make" -C /data/riotbuild/riotbase/cpu/cortexm_common/periph
"make" -C /data/riotbuild/riotbase/cpu/kinetis/periph
"make" -C /data/riotbuild/riotbase/drivers
"make" -C /data/riotbuild/riotbase/drivers/periph_common
"make" -C /data/riotbuild/riotbase/sys
"make" -C /data/riotbuild/riotbase/sys/checksum
"make" -C /data/riotbuild/riotbase/sys/newlib_syscalls_default
"make" -C /data/riotbuild/riotbase/sys/riotboot
"make" -C /data/riotbuild/riotbase/sys/stdio_uart
compiling /data/riotbuild/riotbase/dist/tools/riotboot_gen_hdr/bin/genhdr...
make: Nothing to be done for 'all'.
creating /data/riotbuild/riotbase/bootloaders/riotboot/bin/frdm-k64f/riotboot-slot0.1569401349.riot.bin...
   text	   data	    bss	    dec	    hex	filename
   3444	      0	    604	   4048	    fd0	/data/riotbuild/riotbase/bootloaders/riotboot/bin/frdm-k64f/riotboot.elf

Also tested on machine without local toolchain that uses docker by default.

make -C bootloaders/riotboot/

Launching build container using image "riot/riotbuild:latest".
docker run --rm -t -u "$(id -u)" \
    -v '/usr/share/zoneinfo/Europe/Paris:/etc/localtime:ro' -v '/builds/tmp/RIOT:/data/riotbuild/riotbase' -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' -v '/home/ci/.gitcache:/data/riotbuild/gitcache' -e 'GIT_CACHE_DIR=/data/riotbuild/gitcache'   \
     \
    -w '/data/riotbuild/riotbase/bootloaders/riotboot/' \
    'riot/riotbuild:latest' make    
Building application "riotboot" for "samr21-xpro" with MCU "samd21".

"make" -C /data/riotbuild/riotbase/boards/samr21-xpro
"make" -C /data/riotbuild/riotbase/core
"make" -C /data/riotbuild/riotbase/cpu/samd21
"make" -C /data/riotbuild/riotbase/cpu/cortexm_common
"make" -C /data/riotbuild/riotbase/cpu/cortexm_common/periph
"make" -C /data/riotbuild/riotbase/cpu/sam0_common
"make" -C /data/riotbuild/riotbase/cpu/sam0_common/periph
"make" -C /data/riotbuild/riotbase/cpu/samd21/periph
"make" -C /data/riotbuild/riotbase/drivers
"make" -C /data/riotbuild/riotbase/drivers/periph_common
"make" -C /data/riotbuild/riotbase/sys
"make" -C /data/riotbuild/riotbase/sys/checksum
"make" -C /data/riotbuild/riotbase/sys/newlib_syscalls_default
"make" -C /data/riotbuild/riotbase/sys/pm_layered
"make" -C /data/riotbuild/riotbase/sys/riotboot
"make" -C /data/riotbuild/riotbase/sys/stdio_uart
compiling /data/riotbuild/riotbase/dist/tools/riotboot_gen_hdr/bin/genhdr...
make: Nothing to be done for 'all'.
creating /data/riotbuild/riotbase/bootloaders/riotboot/bin/samr21-xpro/riotboot-slot0.1569401425.riot.bin...
   text	   data	    bss	    dec	    hex	filename
   2564	      0	    608	   3172	    c64	/data/riotbuild/riotbase/bootloaders/riotboot/bin/samr21-xpro/riotboot.elf

There should be no consequences as the base behavior is kept.

Issues/PRs references

Fixes partially #12003

fjmolinas · 2019-09-25T09:05:20Z

@cladmi would you mind taking a look at this one, are there other usages for this? Is there a better way of defining this?

fjmolinas · 2019-09-25T13:44:58Z

This actually still has some issue when compiling riotboot target

fjmolinas · 2019-09-25T14:14:40Z

This actually still has some issue when compiling riotboot target

This would need something like...

ifeq ($(BUILD_IN_DOCKER),1)
# It will trigger executing 'scan-build' or 'scan-build-analyze' with docker
riotboot: ..in-docker-container
riotboot/slot0: ..in-docker-container
riotboot/slot1: ..in-docker-container
riotboot/combined-slot0: ..in-docker-container
endif # BUILD_IN_DOCKER

This isn't in the scope of the original issue @cladmi? right?

cladmi · 2019-09-25T14:23:14Z

This actually still has some issue when compiling riotboot target

This would need something like...
ifeq ($(BUILD_IN_DOCKER),1)
# It will trigger executing 'scan-build' or 'scan-build-analyze' with docker
riotboot: ..in-docker-container
riotboot/slot0: ..in-docker-container
riotboot/slot1: ..in-docker-container
riotboot/combined-slot0: ..in-docker-container
endif # BUILD_IN_DOCKER
This isn't in the scope of the original issue @cladmi? right?

Only few targets are mapped to be build in docker in RIOT, somehow only 'all' or similar. To add support they must be added to

RIOT/makefiles/docker.inc.mk

Lines 6 to 12 in 0170d1c

    
           export DOCKER_MAKECMDGOALS_POSSIBLE = \ 
        
             all \ 
        
             buildtest-indocker \ 
        
             scan-build \ 
        
             scan-build-analyze \ 
        
             tests-% \ 
        
             archive-check \

So this is indeed out of the scope of my original issue.

Doing mcuboot-flash would also not currently work.

cladmi · 2019-09-26T11:37:46Z

I tested and this indeed fixes the compilation with docker and no local toolchain.

Testing can still not be done even when having local toolchain but it is outside of this PR.

cladmi · 2019-09-26T11:48:51Z

Makefile.include

 PKGDIRBASE     ?= $(BINDIRBASE)/pkg/$(BOARD)
 DLCACHE        ?= $(RIOTTOOLS)/dlcache/dlcache.sh
 DLCACHE_DIR    ?= $(RIOTBASE)/.dlcache
+BUILD_FILES    ?= $(ELFFILE) $(FLASHFILE)


I am not sure about defining it as ?= for that case.

If an application Makefile defines it it would overwrite this value and is somehow changing the behavior. As the value must be defined before defining link, it cannot be modified after including Makefile.include so an application would need to re-add everything. (ELFFILE would be generated by print-size anyway, but it would be because of black magic)

If you really want to have the ?= functionality, I would split in two variables, BUILD_FILES_DEFAULT ?= $(ELFFILE) $(FLASHFILE) and BUILD_FILES += $(BUILD_FILES_DEFAULT). Not sure if it is currently required though.

For its definition location, the variables at the top are more "directories" related ones. It think it could go at the same place as the definition of FLASHFILE (currently 440).

cladmi · 2019-09-26T11:49:34Z

makefiles/boot/riotboot.mk

 # being present without having to trigger re-compilation.
 ifneq (1, $(RIOTNOLINK))
-link: $(SLOT_RIOT_ELFS)
+BUILD_FILES += $(SLOT_RIOT_ELFS)


The RIOTNOLINK is not required anymore, as the files are only used when there is a compilation.

cladmi

I like the name BUILD_FILES, as it does not give any information on the types of files, it is generic. Somehow the PR description should not say "files to be linked" though. The fact that it is created by the link target is just historical naming. (RIOTNOLINK does still calls the target link…)

The variables could also be documented in makefiles/vars.inc.mk.

fjmolinas · 2019-09-26T13:05:40Z

I like the name BUILD_FILES, as it does not give any information on the types of files, it is generic. Somehow the PR description should not say "files to be linked" though. The fact that it is created by the link target is just historical naming. (RIOTNOLINK does still calls the target link…)

changed the PR title

The variables could also be documented in makefiles/vars.inc.mk.

addressed

cladmi · 2019-09-26T13:44:38Z

Makefile.include

  $(error FLASHFILE is not defined for this board: $(FLASHFILE))
 endif

+# By default allways build ELFFILE and FLASHFILE


Typo always.

cladmi · 2019-09-26T13:59:54Z

The base change with BUILD_FILES looks good like this for me.
It was one thing I also had planned after adding FLASHFILE.

bootloaders/riotboot tests/riotboot tests/riotboot_flashwrite now compile correctly with BUILD_IN_DOCKER even without local toolchain.

tests/riotboot test does not work with BUILD_IN_DOCKER but is unrelated to this PR.

aabadie

Trusting @cladmi's review in #12302 (comment)

Let's merge this. ACK

aabadie · 2019-09-30T10:55:56Z

Please squash!

- Add BULD_FILES that holds all files that need to be built and linked.

- Fixes compiling in docker by using BUILD_FILES to define extra files to be built

fjmolinas · 2019-10-02T09:28:20Z

Only few targets are mapped to be build in docker in RIOT, somehow only 'all' or similar. To add support they must be added to

Should we do this?

cladmi · 2019-10-02T10:16:56Z

Only few targets are mapped to be build in docker in RIOT, somehow only 'all' or similar. To add support they must be added to

Should we do this?

It could go in after a cleanup of the current handling. There are I think no reason to currently define the list of possible docker goals there.
But when changing this, there are also other wrong thing like the useless export of this variable and the export DOCKER_MAKECMDGOALS ?= all that does nothing at all as it is defined a few lines before.

fjmolinas · 2019-10-02T11:31:32Z

It could go in after a cleanup of the current handling. There are I think no reason to currently define the list of possible docker goals there.
But when changing this, there are also other wrong thing like the useless export of this variable and the export DOCKER_MAKECMDGOALS ?= all that does nothing at all as it is defined a few lines before.

Something that would work as well is BUILD_FILES += $(SLOT_RIOT_ELFS) $(SLOT_RIOT_BINS)

fjmolinas · 2019-10-02T15:08:10Z

Something that would work as well is BUILD_FILES += $(SLOT_RIOT_ELFS) $(SLOT_RIOT_BINS)

This is a bad fix, would only work if the code is not updated. So yeah a general cleanup is needed

fjmolinas added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: build system Area: Build system labels Sep 25, 2019

fjmolinas requested a review from kaspar030 September 25, 2019 09:04

fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 25, 2019

cladmi reviewed Sep 26, 2019

View reviewed changes

fjmolinas mentioned this pull request Sep 26, 2019

riotboot: add RIOTBOOT_BUILD make var #12307

Merged

fjmolinas changed the title ~~Makefile.include: add BUILD_FILES variable that holds all files to be linked~~ Makefile.include: add BUILD_FILES variable that holds all files to be built Sep 26, 2019

cladmi reviewed Sep 26, 2019

View reviewed changes

fjmolinas requested a review from aabadie September 26, 2019 15:24

aabadie approved these changes Sep 30, 2019

View reviewed changes

Makefile.include: add BUILD_FILES

ded2ebc

- Add BULD_FILES that holds all files that need to be built and linked.

fjmolinas force-pushed the pr_build_files branch from abdc023 to c7e0ea4 Compare September 30, 2019 11:09

makefiles/boot/riotboot.mk: use BUILD_FILES

5790e06

- Fixes compiling in docker by using BUILD_FILES to define extra files to be built

fjmolinas force-pushed the pr_build_files branch from c7e0ea4 to 5790e06 Compare September 30, 2019 11:10

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 30, 2019

aabadie merged commit fe6d892 into RIOT-OS:master Sep 30, 2019

cladmi mentioned this pull request Sep 30, 2019

bootloaders|tests/riotboot: broken with BUILD_IN_DOCKER and wrong flashfile #12003

Closed

fjmolinas deleted the pr_build_files branch October 1, 2019 08:11

cladmi mentioned this pull request Oct 1, 2019

[WIP] Fixes to compile using DOCKER and flash boards without local toolchain #10870

Closed

19 tasks

kb2ma added this to the Release 2019.10 milestone Oct 28, 2019

Conversation

fjmolinas commented Sep 25, 2019

Contribution Description

Testing procedure

Issues/PRs references

Uh oh!

fjmolinas commented Sep 25, 2019

Uh oh!

fjmolinas commented Sep 25, 2019

Uh oh!

fjmolinas commented Sep 25, 2019

Uh oh!

cladmi commented Sep 25, 2019

Uh oh!

cladmi commented Sep 26, 2019

Uh oh!

cladmi Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

fjmolinas Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

fjmolinas Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Sep 26, 2019

Uh oh!

cladmi Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

fjmolinas Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi commented Sep 26, 2019

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented Sep 30, 2019

Uh oh!

fjmolinas commented Oct 2, 2019

Uh oh!

cladmi commented Oct 2, 2019

Uh oh!

fjmolinas commented Oct 2, 2019

Uh oh!

fjmolinas commented Oct 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants