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.

Sign up for GitHub

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

Jump to bottom

make: fix export features #6690

Merged
merged 3 commits into from
Sep 8, 2017
Merged

make: fix export features #6690

merged 3 commits into from
Sep 8, 2017

Conversation

lebrush
Copy link
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
@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
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
Member

miri64 commented Apr 8, 2017

(needs rebase though)

@lebrush
Copy link
Member Author

lebrush commented Apr 10, 2017

Rebased

@kaspar030
Copy link
Contributor

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
Contributor

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
Member Author

lebrush commented Jul 19, 2017

Good catch @smlng, thanks!

smlng
smlng approved these changes Jul 19, 2017
View reviewed changes
Copy link
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
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
Copy link
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
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
Member

smlng commented Aug 24, 2017

see #5128

@smlng smlng mentioned this pull request Aug 24, 2017
Merged
@smlng
Copy link
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
Contributor

#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
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
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
Contributor

@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
Contributor

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

@smlng smlng smlng approved these changes

@miri64 miri64 miri64 left review comments

@LudwigKnuepfer LudwigKnuepfer Awaiting requested review from LudwigKnuepfer

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