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

boards: always include cpu features #10078

Merged
merged 2 commits into from
Oct 12, 2018
Merged

boards: always include cpu features #10078

merged 2 commits into from
Oct 12, 2018

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Sep 28, 2018 •
edited
Loading

Contribution description

This adds a sanity check that CPU/Makefile.features is always used and replace the optional '-include' from boards as CPU Makefile.features always exist (done with sed).

Testing procedure

Verify that murdock correctly builds 128 times (== all boards) for examples/hello-world.

Issues/PRs references

Waiting for: #10064 and #10063

Part of working on #9913

@cladmi cladmi added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 28, 2018
@cladmi cladmi requested a review from smlng September 28, 2018 16:54
@cladmi cladmi mentioned this pull request Sep 28, 2018
Merged
smlng
smlng reviewed Oct 1, 2018
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.

More of a question: if every CPU has a Makefile.features and is mandatory to be included, why not do so at a higher level rather than having the same line (i.e. include $(RIOTCPU)/<cpu_name>/Makefile.features) in each and every boards Makefile.features?

@cladmi
Copy link
Contributor Author

cladmi commented Oct 1, 2018

@smlng Glad that you asked :) and you are right it should be but it is also an upcoming PR.

The problem is that currently CPU is defined by $(BOARD)/Makefile.include and so is defined after parsing Makefile.features.
It is in my plans to move CPU to Makefile.features, and other variables, to achieve this. And all the small PRs I am doing right now are here to be ready for this, and prevent new boards to break the migration.

Why not including it after Makefile.include, or anything else?
That is what I describe in "Dependencies processing order issues #9913" and ask for feedback on.
So I would rather ask you to read it than trying to copy-paste things here :)

@cladmi cladmi mentioned this pull request Oct 1, 2018
Open
22 tasks
@cladmi cladmi force-pushed the pr/make/ensure_board_includes_cpu_features branch from 0518443 to b822586 Compare October 1, 2018 13:07
@cladmi cladmi removed the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 1, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Oct 1, 2018

But it's true I forgot to reference the main issue in this PR description. I fixed it.

@cladmi cladmi force-pushed the pr/make/ensure_board_includes_cpu_features branch from b822586 to 2c27a93 Compare October 1, 2018 15:10
@cladmi cladmi force-pushed the pr/make/ensure_board_includes_cpu_features branch from 5818129 to d68cc8d Compare October 9, 2018 15:39
smlng
smlng approved these changes Oct 11, 2018
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.

Tested ACK - features stay the same, also verified the sanity check.

@smlng smlng added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Oct 11, 2018
@smlng
Copy link
Member

smlng commented Oct 11, 2018

please squash

@cladmi
Makefile.include: sanity check that cpu Makefile.features is used
d809d82 
This prepares for being able to always include Makefile.features
globally. It prevents new board for missing this until it is done.
@cladmi cladmi force-pushed the pr/make/ensure_board_includes_cpu_features branch from d68cc8d to d0dce48 Compare October 11, 2018 15:13
@cladmi
boards: unconditionally include CPU/Makefile.features
b450141 
The file always exist so no need to do '-include'.

Replaced using:

    sed -i 's|-\(include $(RIOTCPU)/.*/Makefile.features\)|\1|' \
        $(git grep -l  '$(RIOTCPU)/.*/Makefile.features' boards)
@cladmi cladmi force-pushed the pr/make/ensure_board_includes_cpu_features branch from d0dce48 to b450141 Compare October 11, 2018 15:13
@cladmi
Copy link
Contributor Author

cladmi commented Oct 11, 2018

Thank you for reviewing.

I rebased and squashed. I also re-ran the sed command to check there were no new boards that were introduced.

@MrKevinWeiss MrKevinWeiss merged commit ee3ebe1 into RIOT-OS:master Oct 12, 2018
@cladmi cladmi deleted the pr/make/ensure_board_includes_cpu_features branch October 15, 2018 15:00
@cladmi cladmi added this to the Release 2018.10 milestone Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smlng smlng smlng approved these changes

Assignees
No one assigned
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 Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2018.10
Development

Successfully merging this pull request may close these issues.
3 participants
@cladmi @smlng @MrKevinWeiss