Skip to content

make: fix export features#6690

Merged
haukepetersen merged 3 commits intoRIOT-OS:masterfrom
lebrush:fix/export-features
Sep 8, 2017
Merged

make: fix export features#6690
haukepetersen merged 3 commits intoRIOT-OS:masterfrom
lebrush:fix/export-features

Conversation

@lebrush
Copy link
Copy Markdown
Member

@lebrush lebrush commented Mar 3, 2017

Fixes #2058

The current approach in the build system is to export all the variables required by the make -C ....
Another option would be to create a Makefile.build in the riotbuild folder and import it in Makefile.base.

@lebrush lebrush added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: build system Area: Build system labels Mar 3, 2017
@lebrush lebrush added this to the Release 2017.04 milestone Mar 3, 2017
@lebrush lebrush requested review from LudwigKnuepfer and miri64 March 3, 2017 09:19
@LudwigKnuepfer LudwigKnuepfer added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 13, 2017
@lebrush lebrush added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 22, 2017
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 8, 2017
miri64
miri64 previously approved these changes Apr 8, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK & go, when Murdock is happy.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Apr 8, 2017

(needs rebase though)

@lebrush lebrush force-pushed the fix/export-features branch from b9485c9 to f0ff5a1 Compare April 10, 2017 07:07
@lebrush
Copy link
Copy Markdown
Member Author

lebrush commented Apr 10, 2017

Rebased

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Apr 10, 2017

So the side effects in buildtest are resolved?

@lebrush lebrush added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 13, 2017
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 18, 2017
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Apr 21, 2017

needs further review, postponed

@miri64 miri64 dismissed their stale review May 30, 2017 18:47

Review dismissed because of @kaspar030's comment.

@miri64 miri64 requested a review from kaspar030 May 30, 2017 18:47
@miri64 miri64 removed their assignment May 30, 2017
@lebrush
Copy link
Copy Markdown
Member Author

lebrush commented Jul 19, 2017

Good catch @smlng, thanks!

smlng
smlng approved these changes Jul 19, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

ACK from my side, still I'd like to here @kaspar030 opinion on this before merge.

@smlng
Copy link
Copy Markdown
Member

smlng commented Jul 19, 2017

@lebrush I think you should squash (or drop) the merge commit - so the commit history in RIOT is clear from foreign merge commits

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 19, 2017
@lebrush lebrush force-pushed the fix/export-features branch from 412d9d2 to ff76b60 Compare July 24, 2017 10:14
@lebrush
Copy link
Copy Markdown
Member Author

lebrush commented Jul 24, 2017

There you go :-) sorry for the lack of responsiveness lately

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 24, 2017
smlng
smlng suggested changes Aug 24, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

@kaspar030 is right, there is still a problem when running make buildtest. With this PR make buildtest builds tests/periph_rtc for all boards, though most of them do not have RTC. Using master, it correctly compiles only for boards with feature RTC.

@smlng
Copy link
Copy Markdown
Member

smlng commented Aug 24, 2017

see #5128

@smlng smlng mentioned this pull request Aug 24, 2017
Merged
@smlng
Copy link
Copy Markdown
Member

smlng commented Aug 24, 2017

should wait for #7507

@smlng smlng added State: waiting for other PR State: The PR requires another PR to be merged first and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 25, 2017
@haukepetersen haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 8, 2017
@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Sep 8, 2017

#7507 was merged, giving this PR a last round of testing/verification now...

haukepetersen
haukepetersen approved these changes Sep 8, 2017
View reviewed changes
Copy link
Copy Markdown
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.

PR works as expected (at least for all buildtests/info-boards-supported calls that I tried). Also the optional inclusion of the RTC shell command works as expected on nucleo-f411 (included) and arduino-due (not present).

ACK

@haukepetersen haukepetersen mentioned this pull request Sep 8, 2017
Closed
kaspar030
kaspar030 approved these changes Sep 8, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Sep 8, 2017

@smlng ping

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: waiting for other PR State: The PR requires another PR to be merged first labels Sep 8, 2017
@kaspar030 kaspar030 changed the title Fix export features make: fix export features Sep 8, 2017
@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Sep 8, 2017

all green -> go

@haukepetersen haukepetersen merged commit 27361a5 into RIOT-OS:master Sep 8, 2017
@lebrush lebrush deleted the fix/export-features branch September 8, 2017 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haukepetersen haukepetersen haukepetersen approved these changes

@kaspar030 kaspar030 kaspar030 approved these changes

@miri64 miri64 miri64 left review comments

@LudwigKnuepfer LudwigKnuepfer Awaiting requested review from LudwigKnuepfer

+1 more reviewer

@smlng smlng smlng approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@LudwigKnuepfer LudwigKnuepfer

@kaspar030 kaspar030

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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2017.10

Development

Successfully merging this pull request may close these issues.

7 participants

@lebrush @miri64 @kaspar030 @smlng @haukepetersen @aabadie @LudwigKnuepfer