Skip to content
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

Merged
merged 2 commits into from
Sep 8, 2017

Conversation

smlng
Copy link
Member

@smlng smlng commented Aug 24, 2017

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.

@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system labels Aug 24, 2017
@smlng smlng mentioned this pull request Aug 24, 2017
@smlng smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 24, 2017
@kaspar030
Copy link
Contributor

smlng added the Ready for CI build label 9 minutes ago

CI doesn't use buildtest, so reviewers need to test locally.

@smlng
Copy link
Member Author

smlng commented Aug 24, 2017

yeah, sure I know - just for completeness 😉 IIRC we trigger Murdock even for typos in documentation of C files - which isn' checked either).

@smlng
Copy link
Member Author

smlng commented Aug 24, 2017

reviewers need to test locally

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 \
Copy link
Member

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...

Copy link
Member Author

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 ...

Copy link
Member Author

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

Copy link
Member

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)

Copy link
Member

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)))
Copy link
Member Author

@smlng smlng Aug 24, 2017

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

@smlng smlng added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Aug 28, 2017
@smlng
Copy link
Member Author

smlng commented Aug 30, 2017

so? anybody willing to test this one, IMHO it resolves some major issues and would therefore be nice to have soonish, or not?

@lebrush
Copy link
Member

lebrush commented Aug 31, 2017

I'll have a look today :-)

@lebrush lebrush self-assigned this Aug 31, 2017
Copy link
Member

@lebrush lebrush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested alone and in combination with #6690. Works as expected 👍 ACK. Also BOARD_BLACKLIST works fine. Thanks for tackling this @smlng!

@smlng
Copy link
Member Author

smlng commented Aug 31, 2017

@kaspar030 what's your opinion/assessment on this one? You know our build system quiet well.

@smlng
Copy link
Member Author

smlng commented Sep 4, 2017

@haukepetersen can you have a look at this one? You reported #5128, and this should fix that issue.

Copy link
Contributor

@haukepetersen haukepetersen left a 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.

@haukepetersen
Copy link
Contributor

and it fixes also the #5128, of course...

@kaspar030
Copy link
Contributor

Confirmed that make-info-boards-supported still works as expected.

Nice one!

@jnohlgard
Copy link
Member

Nice!

@smlng smlng deleted the enh/makefiles branch February 6, 2018 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants