Skip to content

tools/insufficient_memory: add create_makefile.ci.sh and Makefile.ci make target #16109

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
Merged
merged 1 commit into from
Nov 15, 2021
Merged

tools/insufficient_memory: add create_makefile.ci.sh and Makefile.ci make target #16109

merged 1 commit into from
Nov 15, 2021

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Feb 28, 2021 •
edited
Loading

Contribution description

For adding a new board we have add_insufficient_memory_board.sh to automatically update all Makefile.ci lists for tests that do not fit the memory of the CPU.

This adds a similar script for doing the same when adding / modifying a test and generates a Makefile.ci by compiling the application for all supported boards.

Testing procedure

For any test / application run

rm Makefile.ci
make Makefile.ci

In the end, a new Makefile.ci should be created that is indistinguishable from the original one, if that one was correct.

Issues/PRs references

@benpicco benpicco added Area: tools Area: Supplementary tools CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Feb 28, 2021
@benpicco benpicco mentioned this pull request Feb 28, 2021
Merged
@miri64
Copy link
Member

miri64 commented Mar 1, 2021

But adding

create_Makefile.ci:
        $(RIOTTOOLS)/insufficient_memory/create_makefile.ci.sh --no-docker

You need to make it a global command ;-) (so add it to makefiles/info-global.inc.mk e.g., but maybe there is a better venue as that file is mostly for informational commands). Since you are setting the board in every applications Makefile (usually native) it will just use that board. Maybe @fjmolinas can help.

miri64
miri64 reviewed Mar 1, 2021
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Some of the shellcheck suggestions seem reasonable. Please apply.

@aabadie
Copy link
Contributor

aabadie commented Mar 1, 2021

Th logic is almost the same as the one in add_insufficient_memory_board.sh, is there a way to adapt the previous script for this use case and share some code ?

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@github-actions github-actions bot added the Area: build system Area: Build system label Aug 6, 2021
@benpicco benpicco requested a review from maribu August 17, 2021 16:04
@benpicco
Copy link
Contributor Author

benpicco commented Aug 22, 2021 •
edited
Loading

Th logic is almost the same as the one in add_insufficient_memory_board.sh, is there a way to adapt the previous script for this use case and share some code ?

The boilerplate in the beginning is the same, but the actual logic is slightly different enough that generalizing it would lead to a more messy result. (If you know of an elegant way to do it, patches welcome of course)

@benpicco benpicco changed the title tools/insufficient_memory: add create_makefile.ci.sh tools/insufficient_memory: add create_makefile.ci.sh and Makefile.ci make target Aug 26, 2021
@benpicco benpicco requested a review from kfessel September 2, 2021 16:56
@benpicco benpicco mentioned this pull request Sep 3, 2021
Open
@benpicco benpicco force-pushed the dist/tools/create_makefile.ci.sh branch from 41cd632 to c96f97b Compare September 15, 2021 13:00
@benpicco benpicco requested a review from kaspar030 September 15, 2021 13:27
@benpicco benpicco mentioned this pull request Nov 4, 2021
Merged
maribu
maribu reviewed Nov 15, 2021
View reviewed changes
makefiles/info-global.inc.mk
@@ -143,6 +143,9 @@ info-boards-features-blacklisted:
info-boards-features-conflicting:
@for f in $(BOARDS_FEATURES_CONFLICTING); do echo $${f}; done | column -t

Makefile.ci:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Makefile.ci:
.PHONY: Makefile.ci
Makefile.ci:

This is needed if you want to be able to update an existing Makefile.ci. Otherwise it will skip with make: 'Makefile.ci' is up to date.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think (but do not know and remember the exact rules) that a file that is actually be produced should be marked PHONY; shouldn't this (on an explicit rerun) rather be manually removed and then rebuilt?

Otherwise, make's behavior of rebuilding any stale includes would trigger, and it'd be rebuilt on ever use.

Copy link
Contributor Author

@benpicco benpicco Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Otherwise it will skip with make: 'Makefile.ci' is up to date.

I did consider that a feature 😉

Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, with .PHONY make just eats CPU time without ever doing anything...

@benpicco benpicco force-pushed the dist/tools/create_makefile.ci.sh branch from c96f97b to 0b3e4cd Compare November 15, 2021 16:42
@chrysn chrysn mentioned this pull request Nov 15, 2021
Merged
@chrysn chrysn 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 Nov 15, 2021
@chrysn
Copy link
Member

chrysn commented Nov 15, 2021

Aehm ... I just ran a make Makefile.ci in an example dir, it went for half an hour, and concluded with make: 'Makefile.ci' is up to date. wihtout having written Makefile.ci :-/

@benpicco benpicco merged commit 7c85041 into RIOT-OS:master Nov 15, 2021
@benpicco benpicco deleted the dist/tools/create_makefile.ci.sh branch November 15, 2021 17:12
@chrysn
Copy link
Member

chrysn commented Nov 15, 2021

and concluded with make: 'Makefile.ci' is up to date. wihtout having written Makefile.ci :-/

OK, that seems to be on me: All failures were due to different things than "out of memory", so leaving it empty was the right call.

@chrysn chrysn mentioned this pull request Nov 15, 2021
Closed
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrysn chrysn chrysn left review comments

@miri64 miri64 miri64 left review comments

@maribu maribu maribu approved these changes

@aabadie aabadie Awaiting requested review from aabadie

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@kfessel kfessel Awaiting requested review from kfessel

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: tools Area: Supplementary tools 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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2022.01
Development

Successfully merging this pull request may close these issues.
7 participants
@benpicco @miri64 @aabadie @chrysn @maribu @MrKevinWeiss @fjmolinas