Makefile.features: prerequisites for moving CPU/CPU_MODEL to boards/Makefile.features by cladmi · Pull Request #11478 · RIOT-OS/RIOT

cladmi · 2019-05-02T16:22:38Z

Contribution description

This pull request introduce a new Makefile.features file and prepares for handling CPU/CPU_MODEL being defined in boards/Makefile.features instead of Makefile.include.

It is somehow the official start of the migration.

This is an API change and may need to be advertised on the mailing list.

Testing procedure

Currently this should have no difference as nothing is defining CPU/CPU_MODEL in the board.

Testing may come more from testing #11477 directly (TODO too)

Reviewing procedure

Please look at the review commit by commit too and ask any question on each steps.

The undefine in makefiles/info-global.inc.mk ensure that no definition will leak between steps
The inclusion of CPU/Makefile.features currently break some boards (boards removing hwrng after kinetis/Makefile.features, so defining CPU for these ones require a fix.
- -> kinetis: move filtering-out periph_hwrng in cpu/kinetis #11479

Issues/PRs references

Tracking: move CPU/CPU_MODEL to Makefile.features #11477
Tracking issue: Build dependencies - processing order issues #9913

Depends on ~~#11480~~

jcarrano · 2019-05-03T13:02:32Z

#11480 is in, rebase.

cladmi · 2019-05-03T13:10:18Z

I will rewrite the too long commit message at the same time.

cladmi · 2019-05-03T13:29:39Z

Github is showing the diff in the creation time order instead of commit order…

jcarrano · 2019-05-06T08:38:19Z

GitHub emphasizes Pull Requests as a space for discussion. All aspects of it--comments, references, and commits--are represented in a chronological order. Rewriting your Git commit history while performing rebases alters the space-time continuum, which means that commits may not be represented the way you expect them to in the GitHub interface.

Or in other words,

GitHub emphasizes it's own vision of how you should work and you are expected to follow it because they know better (I mean they are GitHub).

aabadie · 2019-05-06T08:46:32Z

Better give the reference on Github: https://help.github.com/en/articles/why-are-my-commits-in-the-wrong-order

Or in other words,

This is a personal interpretation. From the link given above, they just recommend not using git rebase

If you always want to see commits in order, we recommend not using git rebase. However, rest assured that nothing is broken when you see things outside of a chronological order!

jcarrano

I think this is a good change. I'm waiting for another review.

fjmolinas

Good stepping stone towards #11477, I have a minor comment to improve variable documentation.

fjmolinas · 2019-05-06T11:50:11Z

makefiles/vars.inc.mk

 export APPLICATION_MODULE    # The application module name.
 export BOARD                 # The board to compile the application for.
-export CPU                   # The CPU, set by the board's Makefile.include.
+export CPU                   # The CPU, set by the board's Makefile.features.


I like the comment specifying where CPU should be defined, it documents a little bit the build system variables. Could we do the same for CPU_MODEL?

I do it now.

kaspar030

Not sure moving CPU etc to Makefile.features. Seems like hack, noone would expect it there.

cladmi · 2019-05-06T11:57:38Z

Not sure moving CPU etc to Makefile.features. Seems like hack, noone would expect it there.

How to solve the issue from #9913 without it then ? Setting a new file seems like an overhead right now.

cladmi · 2019-05-06T11:58:19Z

Not sure moving CPU etc to Makefile.features. Seems like hack, noone would expect it there.

How to solve the issue from #9913 without it then ? Setting a new file seems like an overhead right now.

Also, it would then need to be defined before Makefile.features.

fjmolinas · 2019-05-06T12:17:37Z

Not sure moving CPU etc to Makefile.features. Seems like hack, no one would expect it there.

I think it does make sense to have them defined in Makefile.features if you think of CPU as another feature provided by a board. How I understand this, a board is not only an MCU but an CPU (MPU), with different peripherals and connections to different drivers. IMO it would be more intuitive to think of the CPU as another feature which is then included when needed.

How to solve the issue from #9913 without it then ? Setting a new file seems like an overhead right now.

In #9913 @cladmi hinted it could make event more sense if they were defined in a completely different file. Although I agree with that, this seems like a sensible stepping stone towards dependencies resolution while still making sense semantically.

It also makes sense in a bigger picture where you may have 1 board with multiple CPU units (e.g. usb-kw41z witch also has a k22).

@kaspar030 what do you think?

cladmi · 2019-05-06T12:25:55Z

Another direction for the change, would be to change the definition by defining first the hierarchy that defines a board, in separate files: Board/board_common_1/board_common_2/board_common_3/cpu/cpu_common/cpu_common2. Then from this automatically know which Makefile.features/Makefile.dep/Makefile.include to use and they would not need to include each other anymore as they are automatically included from the hierarchy directories.

I just know it is a really complicated change, with the currently many inconsistencies, and requires to basically have everything fixed before it can be changed. And am not sure I want to change it if we do not have Makefile.dep parsed before Makefile.include as the current behavior is problematic and could really introduce bugs.

It could be an upcoming step but I do not really trust me on doing the change in the current state.

MrKevinWeiss · 2019-06-14T10:10:11Z

Though I am not sure about the ramifications of this (it seems like it will touch a lot of files) it does seem to make sense.

The Makefile.features are what the board has, periphs and whatnot, I could see how a cpu is like a periph...
Currently it is in the Makefile.include, which looks like it is responsible for how to flash, debug and term, all things that are not related to building but to connecting. I could see why it may not belong there.

MrKevinWeiss · 2019-06-26T08:57:20Z

@kaspar030 have your comments been addressed?

smlng

I like this, also (as @MrKevinWeiss pointed out) putting CPU and CPU_MODEL into Makefile.features together with Peripheral stuff makes more sense (to me) than to put it in (or leave in) Makefile.include� (which to me is more a file referencing other Makefiles).

smlng · 2019-06-28T21:02:13Z

I'd say: please squash

MrKevinWeiss · 2019-07-01T07:58:36Z

it seems like we have two acks now. @kaspar030 should we proceed?

kaspar030 · 2019-07-01T09:13:51Z

@kaspar030 should we proceed?

yup!

MrKevinWeiss · 2019-07-01T09:15:48Z

@cladmi please squash!

…deps Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.

Describe the ongoing migration. Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.

Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.

Prepare for when boards define `CPU` in `BOARD/Makefile.features`. Include '$(RIOTCPU)/$(CPU)/Makefile.features' directly when it is defined. This will allow removing the file inclusion from the `BOARD/Makefile.features`. The board must then not include it directly. Transitional change to allow migrating part by parts. Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.

Prepare for when boards define `CPU` in `BOARD/Makefile.features`. Include '$(RIOTCPU)/$(CPU)/Makefile.features' directly when it is defined. This will allow removing the file inclusion from the `BOARD/Makefile.dep`. The board must then not include it directly. Transitional change to allow migrating part by parts. Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include. This can currently trigger including two times the cpu/CPU/Makefile.dep but allows a by board complete migration.

cladmi · 2019-07-01T15:25:11Z

Squashed and rebased.

cladmi · 2019-07-01T16:19:56Z

Thank you for the review and decision to move in this direction.

As this is a major change, I will notify it on the mailing list.

However as soft-feature-freeze is already there, I would not start any big migration before the release is done. @MrKevinWeiss

MrKevinWeiss · 2019-07-02T06:47:48Z

Thanks

cladmi added this to the Release 2019.07 milestone May 2, 2019

cladmi mentioned this pull request May 2, 2019

Tracking: move CPU/CPU_MODEL to Makefile.features #11477

Closed

18 tasks

cladmi requested review from aabadie, fjmolinas, jcarrano and kaspar030 May 2, 2019 16:26

This was referenced May 2, 2019

kinetis: move filtering-out periph_hwrng in cpu/kinetis #11479

Merged

Makefile.features: add a common file for the features parsing #11480

Merged

cladmi added the State: waiting for other PR State: The PR requires another PR to be merged first label May 2, 2019

cladmi requested a review from gschorcht May 2, 2019 17:39

cladmi removed the State: waiting for other PR State: The PR requires another PR to be merged first label May 3, 2019

cladmi force-pushed the pr/make/makefile_features branch from 1620272 to 883b50c Compare May 3, 2019 13:26

cladmi added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label May 3, 2019

jcarrano approved these changes May 6, 2019

View reviewed changes

fjmolinas reviewed May 6, 2019

View reviewed changes

kaspar030 requested changes May 6, 2019

View reviewed changes

cladmi mentioned this pull request May 6, 2019

Build dependencies - processing order issues #9913

Open

22 tasks

cladmi force-pushed the pr/make/makefile_features branch from 7e119d1 to 1c87ce2 Compare June 14, 2019 17:02

cladmi mentioned this pull request Jun 25, 2019

cpu/sam0_common: add hwrng driver #11647

Merged

cladmi modified the milestones: Release 2019.07, Release 2019.10 Jun 27, 2019

smlng approved these changes Jun 28, 2019

View reviewed changes

kaspar030 approved these changes Jul 1, 2019

View reviewed changes

cladmi force-pushed the pr/make/makefile_features branch from 1c87ce2 to 82b3b2c Compare July 1, 2019 15:23

cladmi added 5 commits July 1, 2019 17:23

makefiles/info-global.inc.mk: Reset CPU/CPU_MODEL value when reading …

168cb62

…deps Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.

Makefile.features: add doc on CPU/CPU_MODEL definition in board

3125d6f

Describe the ongoing migration. Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.

makefiles/vars.inc.mk: update doc on CPU/CPU_MODEL definition

5c0ba89

Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.

cladmi force-pushed the pr/make/makefile_features branch from 82b3b2c to fecc9c1 Compare July 1, 2019 15:23

MrKevinWeiss merged commit 9b4c01c into RIOT-OS:master Jul 1, 2019

cladmi deleted the pr/make/makefile_features branch July 1, 2019 16:09

cladmi modified the milestones: Release 2019.10, Release 2019.07 Jul 1, 2019

cladmi mentioned this pull request Jul 1, 2019

Makefile.include: remove RIOT_VERSION_OVERRIDE #11771

Merged

cladmi mentioned this pull request Jul 4, 2019

ble/softdevice: add ble_nordic_softdevice feature #11769

Merged

cladmi mentioned this pull request Aug 15, 2019

boards: move CPU/CPU_MODEL definition to Makefile.features #12014

Merged

cladmi mentioned this pull request Sep 25, 2019

make: add targets to debug dependencies variables #12004

Merged

Conversation

cladmi commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Reviewing procedure

Issues/PRs references

Uh oh!

jcarrano commented May 3, 2019

Uh oh!

cladmi commented May 3, 2019

Uh oh!

cladmi commented May 3, 2019

Uh oh!

jcarrano commented May 6, 2019

Uh oh!

aabadie commented May 6, 2019

Uh oh!

jcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas May 6, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi May 6, 2019

Choose a reason for hiding this comment

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented May 6, 2019

Uh oh!

cladmi commented May 6, 2019

Uh oh!

fjmolinas commented May 6, 2019

Uh oh!

cladmi commented May 6, 2019

Uh oh!

MrKevinWeiss commented Jun 14, 2019

Uh oh!

MrKevinWeiss commented Jun 26, 2019

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

smlng commented Jun 28, 2019

Uh oh!

MrKevinWeiss commented Jul 1, 2019

Uh oh!

kaspar030 commented Jul 1, 2019

Uh oh!

MrKevinWeiss commented Jul 1, 2019

Uh oh!

cladmi commented Jul 1, 2019

Uh oh!

cladmi commented Jul 1, 2019

Uh oh!

MrKevinWeiss commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cladmi commented May 2, 2019 •

edited

Loading