boards: use explicit paths for file inclusion by fjmolinas · Pull Request #12999 · RIOT-OS/RIOT

fjmolinas · 2019-12-20T17:41:00Z

Contribution description

While testing #12972 I realized that when having an external native board name e.g.: BOARD=native-external it will try to include headers in RIOTBASE with the external BOARD name, thus failing.

EDIT: I extended this to all of the cases of:

git grep \/\$$BOARD$ boards/

boards/arduino-mkrwan1300/Makefile.include:INCLUDES += -I$(RIOTBOARD)/$(BOARD)/include
boards/cc2538dk/Makefile.include:DEBUGGER = $(RIOTBOARD)/$(BOARD)/dist/debug.sh
boards/cc2538dk/Makefile.include:RESET ?= $(RIOTBOARD)/$(BOARD)/dist/reset.sh
boards/cc2538dk/Makefile.include:  FLASHER = $(RIOTBOARD)/$(BOARD)/dist/flash.sh
boards/chronos/Makefile.include:INCLUDES += -I$(RIOTBOARD)/$(BOARD)/drivers/include
boards/common/arduino-mkr/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/common/nrf52/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/f4vi1/Makefile.include:DEBUGGER = $(RIOTBOARD)/$(BOARD)/dist/debug.sh
boards/f4vi1/Makefile.include:DEBUGGER_FLAGS = $(RIOTBOARD)/$(BOARD)/dist/gdb.conf $(ELFFILE)
boards/feather-m0/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/mbed_lpc1768/Makefile.include:FLASHER = $(RIOTBOARD)/$(BOARD)/dist/flash.sh
boards/native/Makefile.include:export NATIVEINCLUDES += -I$(RIOTBOARD)/$(BOARD)/include/
boards/native/Makefile.include:RESET ?= $(RIOTBOARD)/$(BOARD)/dist/reset.sh
boards/opencm904/Makefile.include:FLASHER = $(RIOTBOARD)/$(BOARD)/dist/robotis-loader.py
boards/openmote-b/Makefile.include:  export JLINK_RESET_FILE = $(RIOTBOARD)/$(BOARD)/dist/hw_reset.seg
boards/sensebox_samd21/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep

Except for boards/common/nrf52/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep since it is a hack for dependency resolution.

Testing procedure

cp external-native to whatever directory out of riotbase

cp -r tests/external_board_native/external_boards/ <dir/external-boards>

change native to externalnative mv native/ external-native
compile on external BOARD, fails without this change.

BOARDSDIR=<dir/external-boards> BOARD=native make -C examples/hello-world/

No change in any of the touched variables.

for board in arduino-mkrwan1300 cc2538dk chronos f4vi1 feather-m0 mbed_lpc1768 native opencm904 openmote-b sensebox_samd21; do BOARD=${board} make --no-print-directory -C examples/hello-world/ info-debug-variable-BOARD info-debug-variable-USEMODULE info-debug-variable-JLINK_RESET_FILE info-debug-variable-FLASHER info-debug-variable-RESET info-debug-variable-DEBUGGER_FLAGS info-debug-variable-INCLUDES info-debug-variable-DEBUGGER; done > pr.txt; git checkout upstream/master; for board in arduino-mkrwan1300 cc2538dk chronos f4vi1 feather-m0 mbed_lpc1768 native opencm904 openmote-b sensebox_samd21; do BOARD=${board} make --no-print-directory -C examples/hello-world/ info-debug-variable-BOARD info-debug-variable-USEMODULE info-debug-variable-JLINK_RESET_FILE info-debug-variable-FLASHER info-debug-variable-RESET info-debug-variable-DEBUGGER_FLAGS info-debug-variable-INCLUDES info-debug-variable-DEBUGGER; done > master.txt; diff pr.txt master.txt; git checkout pr-12999
# no diff

miri64 · 2019-12-20T18:16:02Z

I'm not sure I understand the use case.

fjmolinas · 2019-12-30T09:15:02Z

I'm not sure I understand the use case.

Sorry, I'll give more details. With the addition of BOARDSDIR one can reuse common BOARDS code present in $(RITOBOARD) by changing $(BOARDSDIR) to an external directory where your BOARD code is stored and replicate some of the included to pull in the common BOARD code present in RIOTBOARD. For example I could extend a samr21-xpro to a samr21-xpro-lora which has an additional lora radio.

tests/external_board_native/Makefile is supposed to show this with a native BOARD only it does a couple of hacks, namely keeping the original native name so the ci compiles and tests this. If you change the fake directory name for native to something else, lets say native-2 (tests/external_board_native/external_boards/native-2 instead of tests/external_board_native/external_boards/native) then you get:

cc1: error: /home/francisco/workspace/RIOT/boards/native-2/include/: No such file or directory [-Werror=missing-include-dirs]

Because $(BOARD) doesn't have the expected name. I don't know how much of a use case there is for this. But this goes in the direction of saying all our BOARD code is actually re-usable common code, it is similar to what we do when making explicit a include $(RIOTBOARD)/common/arduino-mkr/Makefile.include include.

And actually I would change it for all boards and add a static check:

git grep \/\$$BOARD$ boards/

boards/arduino-mkrwan1300/Makefile.include:INCLUDES += -I$(RIOTBOARD)/$(BOARD)/include
boards/cc2538dk/Makefile.include:DEBUGGER = $(RIOTBOARD)/$(BOARD)/dist/debug.sh
boards/cc2538dk/Makefile.include:RESET ?= $(RIOTBOARD)/$(BOARD)/dist/reset.sh
boards/cc2538dk/Makefile.include:  FLASHER = $(RIOTBOARD)/$(BOARD)/dist/flash.sh
boards/chronos/Makefile.include:INCLUDES += -I$(RIOTBOARD)/$(BOARD)/drivers/include
boards/common/arduino-mkr/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/common/nrf52/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/f4vi1/Makefile.include:DEBUGGER = $(RIOTBOARD)/$(BOARD)/dist/debug.sh
boards/f4vi1/Makefile.include:DEBUGGER_FLAGS = $(RIOTBOARD)/$(BOARD)/dist/gdb.conf $(ELFFILE)
boards/feather-m0/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep
boards/mbed_lpc1768/Makefile.include:FLASHER = $(RIOTBOARD)/$(BOARD)/dist/flash.sh
boards/native/Makefile.include:export NATIVEINCLUDES += -I$(RIOTBOARD)/$(BOARD)/include/
boards/native/Makefile.include:RESET ?= $(RIOTBOARD)/$(BOARD)/dist/reset.sh
boards/opencm904/Makefile.include:FLASHER = $(RIOTBOARD)/$(BOARD)/dist/robotis-loader.py
boards/openmote-b/Makefile.include:  export JLINK_RESET_FILE = $(RIOTBOARD)/$(BOARD)/dist/hw_reset.seg
boards/sensebox_samd21/Makefile.include:include $(RIOTBOARD)/$(BOARD)/Makefile.dep

fjmolinas · 2020-01-08T08:13:33Z

@miri64 I updated the PR description and testing procedure with what I mentioned.

fjmolinas · 2020-01-08T08:37:09Z

Rebased and squashed since review doesn't seem to have started yet. Also added a build system sanity check.

miri64 · 2020-01-09T12:28:14Z

Thanks for the explanation. This makes sense.

miri64 · 2020-01-09T12:28:39Z

Oops, sorry /o. Hit the wrong button by accident.

fjmolinas · 2020-01-12T15:11:11Z

ping @miri64!

miri64 · 2020-01-12T15:28:53Z

Sorry, I should have explained: I first and foremost wanted to understand if the concept makes sense. IMHO it does, so I gave the label for that. However, I also think the final decision should be made by someone else, as this is not my usual work area ;-).

fjmolinas · 2020-01-12T15:35:50Z

Maybe @leandrolanzieri @smlng can take a look?

leandrolanzieri

This makes sense (besides the presented use case), we don't need to use an interface here as we already know which board we are referring to. Too bad we have to add the exception for nrf52 boards for now.

I performed the proposed test procedure and using the external native board works now. Also the static check is working correctly and variables maintain their values. Only some nitpicks:

In the message of 109ebf7:
- eternal -> external
- presente -> present

dist/tools/buildsystem_sanity_check/check.sh

With the introduction of BOARDSDIR external boards can re-use common code of BOARDS present in RIOTBASE. To be able to do this file references may not use $(BOARD) since BOARD will be set by the external BOARD.

Makefile.dep is already included for the board no need to do it here.

fjmolinas · 2020-01-13T10:54:34Z

@leandrolanzieri thanks for testing and reviewing, addressed comments.

leandrolanzieri

It looks like dist/tools/flatc/flatc is not part of this?

leandrolanzieri

Everything looks good. ACK!

fjmolinas · 2020-01-13T15:48:09Z

@leandrolanzieri thanks for the review!

fjmolinas added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: native Platform: This PR/issue effects the native platform Area: build system Area: Build system labels Dec 20, 2019

fjmolinas mentioned this pull request Dec 30, 2019

Makefile.include: default to RIOTBOARD when BOARD not in BOARDSDIR #12972

Merged

fjmolinas force-pushed the pr_native_makefile_include_explicit_path branch from e9df414 to dfde913 Compare January 8, 2020 08:04

fjmolinas changed the title ~~boards/native/Makefile.include: use explicit paths~~ boards: use explicit paths for file inclusion Jan 8, 2020

fjmolinas requested a review from aabadie January 8, 2020 08:13

fjmolinas force-pushed the pr_native_makefile_include_explicit_path branch from df97bd4 to 1baf3a7 Compare January 8, 2020 08:35

fjmolinas force-pushed the pr_native_makefile_include_explicit_path branch from 1baf3a7 to 8016227 Compare January 8, 2020 08:37

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 8, 2020

fjmolinas requested a review from dylad January 8, 2020 08:47

miri64 closed this Jan 9, 2020

miri64 reopened this Jan 9, 2020

miri64 added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Jan 9, 2020

fjmolinas requested a review from leandrolanzieri January 10, 2020 08:49

leandrolanzieri reviewed Jan 13, 2020

View reviewed changes

dist/tools/buildsystem_sanity_check/check.sh Outdated Show resolved Hide resolved

fjmolinas force-pushed the pr_native_makefile_include_explicit_path branch from 8016227 to 3baa142 Compare January 13, 2020 10:53

fjmolinas added 2 commits January 13, 2020 11:53

boards: replace use of $(BOARD) by explicit name

5a8f996

With the introduction of BOARDSDIR external boards can re-use common code of BOARDS present in RIOTBASE. To be able to do this file references may not use $(BOARD) since BOARD will be set by the external BOARD.

boards/common/arduino-mkr: remove /Makefile.dep

74cecf3

Makefile.dep is already included for the board no need to do it here.

fjmolinas force-pushed the pr_native_makefile_include_explicit_path branch from 3baa142 to c93bbbe Compare January 13, 2020 10:53

leandrolanzieri reviewed Jan 13, 2020

View reviewed changes

fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 13, 2020

dist/tools/buildsystem_sanity_check: check \ not in boards/

6a0eaec

fjmolinas force-pushed the pr_native_makefile_include_explicit_path branch from c93bbbe to 6a0eaec Compare January 13, 2020 11:44

fjmolinas added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 13, 2020

leandrolanzieri approved these changes Jan 13, 2020

View reviewed changes

leandrolanzieri merged commit 50f1b14 into RIOT-OS:master Jan 13, 2020

fjmolinas deleted the pr_native_makefile_include_explicit_path branch January 13, 2020 15:48

fjmolinas added this to the Release 2020.01 milestone Jan 15, 2020

miri64 mentioned this pull request Jan 17, 2020

boards: add support for nrf52840-dongle #13146

Merged

8 tasks

Conversation

fjmolinas commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Uh oh!

miri64 commented Dec 20, 2019

Uh oh!

fjmolinas commented Dec 30, 2019

Uh oh!

fjmolinas commented Jan 8, 2020

Uh oh!

fjmolinas commented Jan 8, 2020

Uh oh!

miri64 commented Jan 9, 2020

Uh oh!

miri64 commented Jan 9, 2020

Uh oh!

fjmolinas commented Jan 12, 2020

Uh oh!

miri64 commented Jan 12, 2020

Uh oh!

fjmolinas commented Jan 12, 2020

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjmolinas commented Jan 13, 2020

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Jan 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fjmolinas commented Dec 20, 2019 •

edited

Loading