Makefile.include: introduce 'BOARDSDIR' for boards directory #12183
Conversation
|
@bergzand Finally something for your issue :) @vincent-d @javierfileiv @toonst I think you were all interested by this. Please complain if it does not work for your need, I want an implementation that can be used. |
miri64
added
Area: build system
benpicco
added
the
CI: ready for build
|
Travis is not happy: |
Indeed, I also added a comment to describe that there is not need for a WHITELIST. It was the low level reason that made me do this |
|
I will review this soon because I think this is very useful. At first sight the PR seems in a good shape (with tests and documentation). |
|
You can squash the squash commit since no code review was already started AFAICS. |
I squashed the fixup for |
|
Hi everyone... I was checking this MR on Mac OS and there is a problem with the build that I've solve changing this lines I commented the line using sed for Mac as it doesn't work and I didn't understand why are we generating riotbuild.h.in, I guess it's not to create it several times if CFLAGS had not change. |
|
@javierfileiv does this patch work for you #12435 ? |
|
@benpicco Thanks! Actually I solved it installing gnu sed with homebrew and changing the path as its shown below as @vincent-d told me to do! :) First line on rc file: Beside that little problem, everything was working fine for me. |
|
Can you also give #12435 a try? |
|
@cladmi I like the PR! diff --git a/Makefile.include b/Makefile.include
index 20901af1d..93f1314a7 100644
--- a/Makefile.include
+++ b/Makefile.include
@@ -248,6 +248,11 @@ ifeq (,$(UNZIP_HERE))
endif
endif
+# allow fall-back to upstream boards if BOARDSDIR is set to a custom value
+ifneq (0,$(shell test -d $(BOARDSDIR)/$(BOARD); echo $$?))
+ BOARDSDIR := $(RIOTBOARD)
+endif
+
# Tool saving stdin to a file only on content update.
# It keeps the file timestamp if it would end up the same.
LAZYSPONGE ?= $(RIOTTOOLS)/lazysponge/lazysponge.pyThis way you can still use the upstream boards even when This way I can set |
I was testing it wrong... it didn't work on my MAC... what I can do it's just pull the PR and try it but I guess I did the mods he has done |
|
@javierfileiv You can just apply the patch directly: |
|
Thank @benpicco ... it didn't work... actually riotbuild.h.in gets generated but riotbuild.h it's empty. What I have done before knowing @vincent-d patch was to only create riotbuild.h, without using the .in file. But as I said before, with the steps that Vincent told me was working flawlessly |
The The issue here must be with the sed command with multiple |
This half handling of a board PATH would break the Also this implementation relies on calling the |
There was a problem hiding this comment.
Looks good in general and ACK for BOARDSDIR variable name.
The testing procedure is well described and is working. Also thanks for providing a test. I found a couple of typos, that's why I'm requesting changes. You can squash the fixes right away and we'll be good to go.
I agree that @benpicco's improvement suggestion can be done in a follow-up PR (and will need careful review, e.g without use of shell and := ;) ).
It would then be the same as implementing as a normal board that does not use any common (I had no idea what was required for this tbh so never wrote it), except an appendix to say to overwrite So in here, adding comments like "Include the common Makefile.dep, this board is implemented as if Feel free to push the changes you would like directly. |
|
@dylad @aabadie I agree with the fact that the documentation here is not very clear, or can be confusing when looking at the test example. It would indead be good to document how to port a I propose we open a follow-up PR extending the documentation and limit this one to the technical feature. This to ease up future changes since the original contributor isn't active anymore (@cladmi commented but isn't developping anymore) Other than that there is still something I would like to see in this PR and is to generally fallback to setting |
|
This is a very useful feature so I'm not opposed to merge it as it now and improve the documentation later.
I have no strong opinion on this point as long as it doesn't duplicate or complicate stuff in the build system. |
Ok, lets handle this in another PR then, and discuss it there. |
|
I opened an issue #12963 for the documentation and auto-assigned it to myself, I'll deal with it at soon as possible and before the end of this release cycle (unless someone does it before). |
|
@aabadie Do you have any more changes request pending ? |
|
There's a problem with the static tests. I'll have a look. |
Allow defining new directory variables that will not be overridden when set from command line. Command line is supposed to override from the value in make. Promoting another behavior is against `make`.
Introduce a new variable 'BOARDSDIR' to use when referencing the base boards directory. This is a transition to allow defining external boards while still using the `RIOT/boards` directory for like `boards/common` for example through 'RIOTBOARD'.
Replace uses of 'RIOTBOARD' by 'BOARDSDIR' during the compilation.
Replace uses of 'RIOTBOARD' by 'BOARDSDIR' for tools.
Handle setting 'BOARDSDIR' when building in docker.
Replace using 'RIOTBOARD' by 'BOARDSDIR' to define external boards.
Add an example that implements an external board based on native. It relies on 'BOARDSDIR' and uses common files from 'RIOT/boards' through 'RIOTBOARDS'. This application also works with the docker integration.
Replace uses of 'RIOTBOARD' by 'BOARDSDIR' in examples.
Replace using 'RIOTBOARD' by 'BOARDSDIR' to define external boards.
This test application must act as a regular application to showcase how to provide a board support from an external directory
aabadie
force-pushed
the
pr/boardsdir
branch
from
faf7463 to
211cec8
Compare
|
I pushed I fix: exclude the new test application from one of the build system static test. Straight forward. If you are fine with this @dylad, then I'll approve this PR. |
|
@aabadie Let's move forward. I'm happy with the current state. |
|
Here we go. |
Contribution description
Introduce a new variable 'BOARDSDIR' to use when referencing the base
boards directory.
The goal is to replace using 'RIOTBOARD' by 'BOARDSDIR' to define external boards. With this, external boards could still use 'RIOTBOARD' to use, for example, 'RIOT/boards/common' files.
Help needed
Naming
Does the name fit? I tried something that was not prepended by
RIOTdirectly to say it is not a RIOT/something directory, but ended up not being namespaced anymore.I did not use 'external' either, as it should be the one used when refering to boards inside riot too.
I can replace it and update all the commit messages accordingly, it was easier to go with something and test than wait until I find the right name.
Documentation
Was there a documentation to say to use
RIOTBOARDfor external boards? a git grep finds nothing.I can add a documentation in http://doc.riot-os.org/advanced-build-system-tricks.html
Does it fit your needs?
Does the wrapping works for you?
It did the choice to not overwrite the variable anymore even if set from the command line (make BOARDSDIR=lala) as it is not a normal make behavior.
The implementation also still does `BOARDSDIR := $(
Special for @bergzand
No currently I do not pass this variable to
riotbootwhen building the bootloader asRIOTBOARDwas not passed either.Testing procedure
I added an example in
tests/external_board_native/that declares an externalnativeboard. Usingnativename will allow murdock to run the test.make --no-print-directory -C tests/external_board_native/ flash testListing boards only lists the external boards. It is not a boards path search implementation.
Building in docker is supported. I implemented it such as the
BOARDSDIRis always set from the command line (when notRIOTBOARD), to allow overwriting any value that could be set in the makefile to an absolute path not depending on build path.As I am building on an
ubuntu:bionic, as our docker image, I can even run the native executable locally.BUILD_IN_DOCKER=1 DOCKER="sudo docker" make --no-print-directory -C tests/external_board_native/ flash testWhen the directory is not in
RIOT, the mounting is handled correctly:Build in docker with out of tree `BOARDSDIR`
Building a normal example does not show any BOARDSDIR handling in docker as it should
Issues/PRs references
Was referenced as an issue in #11155
Also talked about it with different persons during the RIOT summit.