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

buildsystem: split default modules include early and late #17662

Merged
merged 1 commit into from
Jun 2, 2022
Merged

buildsystem: split default modules include early and late #17662

merged 1 commit into from
Jun 2, 2022

Conversation

fjmolinas
Copy link
Contributor

Contribution description

This PR is merely a cleanup from #17632, with #17584, more modules would be added as DEFAULT_MODULEs only at the end. So this PR adds a file to cumulate those as well as renaming the file introduced in #17632 so that naming aligns.

Testing procedure

Green murdock should be enough here.

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 16, 2022
@fjmolinas fjmolinas requested review from benpicco and maribu February 16, 2022 08:23
@github-actions github-actions bot added the Area: build system Area: Build system label Feb 16, 2022
@fjmolinas fjmolinas requested a review from kaspar030 February 16, 2022 09:50
@fjmolinas
Copy link
Contributor Author

I ran ./dist/tools/buildsystem_sanity_check/save_all_dependencies_resolution_variables.sh against the head of this PR and the commit just before, then diffed the result, there is no diff so nothing changed regarding dependencies.

# on 726c461cb5f1b8bfaef80317f54e0d2e102dbfbc
./dist/tools/buildsystem_sanity_check/save_all_dependencies_resolution_variables.sh build/deps/master
# on d62f82708682a6017873d604d8a88af84bdd642a
./dist/tools/buildsystem_sanity_check/save_all_dependencies_resolution_variables.sh build/deps/master
# diff both
diff build/deps/pr/deps_info ../RIOT2/build/deps/master/deps_info
# empty

@maribu Is there anything to look at besides naming? I'm personally in favor of your descriptive long names, but would rather get all the nitpicks at once since the build is already passing, I want to do only one more build.

@fjmolinas fjmolinas force-pushed the pr_default_modules_early_late branch from 542f69b to b981061 Compare February 21, 2022 08:51
@fjmolinas
Copy link
Contributor Author

Ping @maribu!

@fjmolinas fjmolinas force-pushed the pr_default_modules_early_late branch from b981061 to f8e5f6e Compare June 1, 2022 07:38
@fjmolinas
Copy link
Contributor Author

IMO this is really only a cleanup, but better to have a specific file for this as well

benpicco
benpicco approved these changes Jun 1, 2022
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This essentially just moves some default modules from dependency_resolution.inc.mk to it's own file and renames some files for consistency.

Please squash.

@maribu maribu enabled auto-merge June 1, 2022 09:42
@maribu
Copy link
Member

maribu commented Jun 1, 2022

CI passes except for two bogus hash mismatches due to unreproducible builds with ESP. Let's run the CI again without compile tests.

@maribu maribu added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs 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 Jun 1, 2022
@maribu maribu disabled auto-merge June 2, 2022 07:18
@maribu
Copy link
Member

maribu commented Jun 2, 2022

@fjmolinas: Please squash ;)

@fjmolinas fjmolinas force-pushed the pr_default_modules_early_late branch from f8e5f6e to fbe9ad0 Compare June 2, 2022 08:19
@fjmolinas
Copy link
Contributor Author

Squashed!

@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: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 2, 2022
@fjmolinas
buildsystem: split default modules include early and late
b5197db 
DEFAULT_MODULEs declared in defaultmodules_regular.inc.mk MAY only be
disabled at APPLICATION level or in BOARD/CPU Makefile.default. These
modules MAY have complex dependencies themselfs.

DEFAULT_MODULEs declared in defaultmodules_no_recursive_deps.inc.mk MAY be disabled
during dependency resolution. The MUST only have dependencies against
modules with no dependencies themselfs, and these dependencies must
be defined in makefiles/defaultmodules._deps.inc.mk
@fjmolinas
Copy link
Contributor Author

Rebased to get the pthon fix in.

@fjmolinas fjmolinas force-pushed the pr_default_modules_early_late branch from fbe9ad0 to b5197db Compare June 2, 2022 10:57
@maribu
Copy link
Member

maribu commented Jun 2, 2022

Last full CI run is from yesterday --> recent enough.

@maribu maribu merged commit 055a26d into RIOT-OS:master Jun 2, 2022
@fjmolinas fjmolinas deleted the pr_default_modules_early_late branch June 3, 2022 06:45
@fjmolinas
Copy link
Contributor Author

Thanks for the review!

@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu left review comments

@benpicco benpicco benpicco approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

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 CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.
4 participants
@fjmolinas @maribu @benpicco @chrysn