Buildsystem: Automatically add Common Board Definitions when USEMODULE is set
#21299
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Murdock results❌ FAILED d60dcb4 fixup! fixup! fixup! boards/*: adapt boards to new common boards Build failures (21)
Artifacts |
8947c43 to
81f7aa2
Compare
mguetschow
reviewed
Mar 17, 2025
Makefile.include
Outdated
Comment on lines
611
to
612
| COMMON_BOARD_DIRS := $(subst _common_,/common/,$(COMMON_BOARD_MODULES)) | ||
| COMMON_BOARD_DIRS := $(addprefix $(RIOTBASE)/,$(COMMON_BOARD_DIRS)) |
There was a problem hiding this comment.
This introduces a 1-to-1 relationship between the module name and it's folder structure. While I do think that makes sense, I think this is new to RIOT.
Could we mimic the way sys/Makefile* are organized and instead add that to dirs in application.inc.mk?
81f7aa2 to
7fd7d2e
Compare
|
This is a much bigger task than I imagined... the fixup comment is a start for the new approach, but there is still a lot to do: I'm not sure how to proceed with the Lines 3 to 5 in 04a1698 |
31cdbcc to
250bef0
Compare
338944c to
2deeb69
Compare
3880883 to
ed8dcba
Compare
|
Okay the Microbit code is a bit weird, apparently the |
ed8dcba to
5738655
Compare
|
One could argue that this PR would be a good opportunity to add the That would really clean up the board definition Makefiles IMO and you would only have to add the |
5738655 to
d60dcb4
Compare
|
I think this PR has to be split up into separate PRs:
Otherwise this will be a cat-and-mouse game with CI with a lot of potential to break things 😅 Tomorrow I'll split this up and possibly create a tracking issue with groups that have to be adapted. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
Many boards in RIOT (such as the
feather-nrf52840,feather-m0, ...) use common board definitions for things like the bootloader. Back when this was implemented in #8058, the approach was to include the directory by adding the path to theDIRSvariable and setting theUSEMODULEin one of the common folders.This is not really elegant and the
Makefilein theboards/common/...subfolder didn't even get executed.You can find more information about this in #21298.
The PR is based on #21281 and since it is not currently merged, I added the changes in squashed-form. That is obviously to be deleted once #21281 is merged.This is meant as a Request for Comments.If this is valid solution, it would be possible to adapt it to the CPU common folders as well, but probably not in this PR.
Testing procedure
Make sure the boards still compile and the common folder is still included.
The only adapted boards are currently the two
feather-nrf52840*and theseeedstudio-xiao-nrf52840.`tests/sys/shell` on `feather-nrf52840` with #21281:
`tests/sys/shell` on `feather-nrf52840` with this PR:
Boards that are not yet adapted, such as the
feather-m0, will include the common files twice. I don't think that that would have any adverse effects, but it's obviously not desireable.`tests/sys/shell` on `feather-m0` with master
`tests/sys/shell` on `feather-m0` with this PR:
Issues/PRs references
Fixes #21298.