-
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, build tests: omit recursive make #7507
Conversation
CI doesn't use buildtest, so reviewers need to test locally. |
yeah, sure I know - just for completeness 😉 IIRC we trigger Murdock even for typos in documentation of C files - which isn' checked either). |
btw. please go on and do so 😄 |
@@ -38,7 +38,7 @@ buildtest: | |||
@ \ | |||
BUILDTESTOK=true; \ | |||
APP_RETRY=0; \ | |||
for BOARD in $$($(MAKE) -s info-boards-supported); do \ | |||
for BOARD in ${BOARDS}; do \ |
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.
Mhhhh, but this also includes blacklisted/non-whitelisted boards...
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.
did you actually try? Because I don't see that ...
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.e., I test with tests/periph_rtc
because of #6690 and just added BOARD_BLACKLIST := arduino-zero
to its Makefile, which successfully excludes this board though it has the feature periph_rtc
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.
Mhh... I'm not sure where the BOARDS
comes from. But make -s info-boards-supported
calculates this... if this is done in another way, we might be able to remove or simplify the info-boards-supported
target (but not in this PR)
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 trust your tests on this for now, I just noticed that by looking at the code ;-))
@@ -255,7 +255,7 @@ info-boards-features-missing: | |||
|
|||
FEATURES_REQUIRED += $(FEATURES_OPTIONAL) | |||
|
|||
ifneq (, $(filter info-boards-supported info-boards-features-missing info-build, $(MAKECMDGOALS))) | |||
ifneq (, $(filter buildtest info-boards-supported info-boards-features-missing info-build info-buildsizes info-buildsizes-diff, $(MAKECMDGOALS))) |
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.
@miri64 the code below generate ${BOARDS}
but this ifneq
guards the code to be only exec for specific make targets, e.g. for info-boards-supported
but w/o this PR not for target buildtest
which IMHO make[s] no sense. So I added all targets that require the boards list here and that way the recursive call of make info-boards-supported
is obsolete which thereby also fixes described issues with the exported features variables, see e.g. #6690
so? anybody willing to test this one, IMHO it resolves some major issues and would therefore be nice to have soonish, or not? |
I'll have a look today :-) |
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.
@kaspar030 what's your opinion/assessment on this one? You know our build system quiet well. |
@haukepetersen can you have a look at this one? You reported #5128, and this should fix that issue. |
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, this fixes the issue I had in #5065, at least for all the examples I just tried. So ACK from my side.
and it fixes also the #5128, of course... |
Confirmed that make-info-boards-supported still works as expected. Nice one! |
Nice! |
This PR fixes problems with exported variables by omitting recursive make, and also fixes an issues with parameter usage for
size
command on macOS.Fixes issues discussed in #6690, and likely #5128, too.