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

Makefile.dep: filter periph_init modules #13648

Merged
merged 1 commit into from
Mar 31, 2020
Merged

Makefile.dep: filter periph_init modules #13648

merged 1 commit into from
Mar 31, 2020

Conversation

fjmolinas
Copy link
Contributor

@fjmolinas fjmolinas commented Mar 17, 2020 •
edited
Loading

Contribution description

This is PR is based on some of the cleanups in #13349. It moves handling of MODULES that implement FEATURES to its own makefile. In doing so it also fixes the issue mentioned in #13639 (comment)_.

Testing procedure

Issues/PRs references

Based on #13349
Fixes issues introduced in #13639

@fjmolinas fjmolinas added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Mar 17, 2020
@fjmolinas fjmolinas requested a review from maribu March 17, 2020 09:03
maribu
maribu approved these changes Mar 17, 2020
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Looks good and seems to work. I have one comment inline. If you take this, please squash that right in.

makefiles/features.modules.mk Outdated Show resolved Hide resolved
@maribu
Copy link
Member

maribu commented Mar 17, 2020

Btw: I really like the idea to move the split out file to makefiles/ rather than polluting the root directory. I should do the same for the FEATURES_REQUIRED_ANY PR.

@fjmolinas fjmolinas force-pushed the pr_periph_init_cleanup branch from 4192724 to 1f12991 Compare March 17, 2020 14:14
@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before CI: run tests If set, CI server will run tests on hardware 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 17, 2020
@fjmolinas fjmolinas force-pushed the pr_periph_init_cleanup branch from 1f12991 to d803143 Compare March 17, 2020 15:59
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 17, 2020
@fjmolinas fjmolinas mentioned this pull request Mar 18, 2020
Closed
@maribu maribu mentioned this pull request Mar 19, 2020
Merged
@fjmolinas
Copy link
Contributor Author

Found dome more differences when running save_all_dependencies_resolution_variables.sh.

@fjmolinas fjmolinas force-pushed the pr_periph_init_cleanup branch from d803143 to 0ea0eb8 Compare March 26, 2020 15:38
@fjmolinas fjmolinas changed the title Makefile.dep: add features.modules.mk Makefile.dep: filter periph_init modules Mar 26, 2020
@fjmolinas @maribu
Makefile.dep: filter periph_int modules
9279282 
Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de>
@fjmolinas fjmolinas force-pushed the pr_periph_init_cleanup branch from 0ea0eb8 to 9279282 Compare March 26, 2020 15:44
@fjmolinas
Copy link
Contributor Author

@maribu because of the comments above I simply added the filter and fix the performance issue, I'll move on to review #13349 for more general fixups and cleanups.

  • PR
time make -C tests/xtimer_usleep info-boards-supported 
...
...
real	0m0.658s
user	0m0.511s
sys	0m0.147s
  • master
time make -C tests/xtimer_usleep info-boards-supported 
...
...
real	0m15.744s
user	0m15.551s
sys	0m0.162s

@maribu maribu 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: 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 Mar 26, 2020
@maribu
Copy link
Member

maribu commented Mar 26, 2020

Murdock took super long for this one. Maybe there was another performance issue introduced?

@fjmolinas
Copy link
Contributor Author

Murdock took super long for this one. Maybe there was another performance issue introduced?

I think its just because all tests where run, but there where some test failures, all on nrf52dk. I'll try to reproduce.

@maribu maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Mar 26, 2020
maribu
maribu approved these changes Mar 26, 2020
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. Works as expected. I see no way this could cause the test failures; so they should not delay this PR. However, it would still be good to investigate them.

@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 31, 2020
@miri64 miri64 merged commit 210543d into RIOT-OS:master Mar 31, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Apr 3, 2020
@fjmolinas fjmolinas deleted the pr_periph_init_cleanup branch July 31, 2020 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu 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 2020.04
Development

Successfully merging this pull request may close these issues.
4 participants
@fjmolinas @maribu @miri64 @leandrolanzieri