make: refactor info targets & cleanup buildtest by kaspar030 · Pull Request #7589 · RIOT-OS/RIOT

kaspar030 · 2017-09-08T22:54:02Z

Previously, our build system mixed local targets (corresponding to one board, e.g., make all) and global targets (corresponding to all boards, e.g., make info-boards-supported).

Half of the build defines was set for $BOARD, and the dependencies were only parsed for one board.
This had several side effects, most notable:

make info-boards-supported could not take dependencies into account
make buildtest needed to reset most variables to sane values, which looked ugly

This PR essentially cuts the build parsing in two modes: a "build for one board" mode, which is the default, and a "global targets" mode, which is selected automatically if one of the "global" targets is in $(MAKECOMMANDGOALS).

The latter case now re-parses the dependencies for every board. That means if e.g., a driver has FEATURES_REQUIRED += xxx, that dependency gets properly handled for e.g., info-boards-supported. As a nice side effect, this is now almost twice as fast.

I took the liberty to remove a lot of now unnecessary cruft from buildtest.

Unfortunately all board Makefile.features now must include the CPU's Makefile.features, but the CPU is not known, thus it's name must be hard-coded in the include path. I consider this a minor uglyness compared to the vast cleanup this PR offers.

haukepetersen

Awesome! On a first quick look the changes look valid to me, though I have not run any tests yet, so a further review is to be done...

haukepetersen · 2017-09-09T21:00:57Z

makefiles/features.inc.mk

@@ -1,6 +1,5 @@
 # import list of provided features
 -include $(RIOTBOARD)/$(BOARD)/Makefile.features


wondering: with the boards Makefile.features now including the CPUs Makefile.features, shouldn't we force the existence of a Makefile.features for every board (-> removing the minus sign in this line)? As there might be features provided by a CPU that are not looked at in case a board does not provide a Makefile.features...

aabadie

Had a brief look and saw some indentation issues.

aabadie · 2017-09-11T14:40:03Z

Makefile.include

+endif
+
+ifeq (none,$(BOARD))
+include $(RIOTMAKE)/info-global.inc.mk


indentation missing here

aabadie · 2017-09-11T14:41:22Z

Makefile.include


 # include multislot support
 include $(RIOTMAKE)/multislot.mk
+endif # BOARD=none


The indentation above seems off (not related to this PR though)

Actually, it is. But I didn't want to indent the whole file (or most of it), so I intentionally didn't indent the "BOARD!=none" case.

aabadie · 2017-09-11T14:42:41Z

makefiles/info-global.inc.mk

+FEATURES_OPTIONAL_GLOBAL := $(FEATURES_OPTIONAL)
+DISABLE_MODULE_GLOBAL := $(DISABLE_MODULE_GLOBAL)
+
+define board_missing_features


indentation is off in this function

Looks fine here, all tabs. anything specific?

Before it was not indented this way. That's why I pointed this out. I don't know what's the best practice here

fixed it to two spaces

miri64 · 2017-09-13T09:57:07Z

Fixes #7033

miri64

Tested with tests/driver_at86rf2xx and test/unittests. info-buildsizes does not put out the build sizes, just the boards, info-files misses all the non-Make files, info-features-missing has no output for tests/unittests.

miri64 · 2017-09-13T11:10:37Z

(otherwise I'm fine with the changes and would approve, if pointed out errors are fixed).

kaspar030 · 2017-09-13T12:36:41Z

info-buildsizes does not put out the build sizes, just the boards

fixed.

info-files misses all the non-Make files, info-features-missing has no output for tests/unittests.

Both is also true for master...

miri64

Alright then you get my ACK. @aabadie what about you?

aabadie

Well, there are still indentation issues but that's fine.

ACK

miri64 · 2017-09-13T13:31:39Z

Well, there are still indentation issues but that's fine.

As far as I can see it, all your comments are addressed.

@kaspar030 please squash.

aabadie · 2017-09-13T13:47:35Z

As far as I can see it, all your comments are addressed.

Yes, but I saw it in other places...

kaspar030 · 2017-09-13T17:48:41Z

@kYc0o found something not working on OS X (the color makefile parsing was moved before assignment of the OS variable, which broke OS X's special handling).

The fix is simple, but I think it needs a re-ack.

miri64 · 2017-09-13T19:14:36Z

The fix is simple, but I think it needs a re-ack.

In theory I would ACK, but can't test that. @kYc0o?

kYc0o · 2017-09-14T11:14:46Z

@kaspar030 can you squash?

kaspar030 · 2017-09-14T11:16:28Z

squashed.

kYc0o

ACK. But I hope a PR fixing the feature check for all targets comes soon :)

kYc0o · 2017-09-14T11:40:43Z

Go!

kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system labels Sep 8, 2017

kaspar030 added this to the Release 2017.10 milestone Sep 8, 2017

kaspar030 requested a review from haukepetersen September 8, 2017 22:54

kaspar030 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

kaspar030 mentioned this pull request Sep 9, 2017

make: use submodules for periph #7241

Merged

haukepetersen reviewed Sep 9, 2017

View reviewed changes

kaspar030 requested review from jnohlgard and miri64 September 11, 2017 14:38

aabadie requested changes Sep 11, 2017

View reviewed changes

miri64 requested changes Sep 13, 2017

View reviewed changes

miri64 approved these changes Sep 13, 2017

View reviewed changes

aabadie approved these changes Sep 13, 2017

View reviewed changes

kaspar030 added 3 commits September 14, 2017 13:15

make: include cpu Makefile.features from board Makefile.features

02e055f

make: refactor info- and buildtest targets

daa277b

make: fix gnrc_slip dependency

0a47815

kaspar030 force-pushed the fix_deps branch from 283dfab to 0a47815 Compare September 14, 2017 11:16

kYc0o approved these changes Sep 14, 2017

View reviewed changes

kYc0o merged commit 46c132d into RIOT-OS:master Sep 14, 2017

kaspar030 deleted the fix_deps branch September 14, 2017 11:41

cladmi mentioned this pull request Sep 27, 2017

make: buildest fails with board dependent application Makefiles #5207

Closed

miri64 mentioned this pull request Oct 18, 2017

Release 2017.10 - RC1 RIOT-OS/Release-Specs#47

Closed

43 tasks

This was referenced Oct 18, 2017

make: add capability to ignore provided environment variables #7747

Closed

make: fix info-boards-supported/-features-missing #6063

Closed

		@@ -1,6 +1,5 @@
		# import list of provided features
		-include $(RIOTBOARD)/$(BOARD)/Makefile.features

Conversation

kaspar030 commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Sep 13, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Sep 13, 2017

Uh oh!

kaspar030 commented Sep 13, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Sep 13, 2017

Uh oh!

aabadie commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Sep 13, 2017

Uh oh!

miri64 commented Sep 13, 2017

Uh oh!

kYc0o commented Sep 14, 2017

Uh oh!

kaspar030 commented Sep 14, 2017

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Sep 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kaspar030 commented Sep 8, 2017 •

edited

Loading

aabadie commented Sep 13, 2017 •

edited

Loading