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.

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

dist/tools/insufficient_memory: Minor improvements #19303

Conversation

maribu
Copy link
Member

@maribu maribu commented Feb 23, 2023

Contribution description

create_makefile.sh:

  • address all shellcheck warnings
  • make script POSIX shell compatible
  • use nproc to set the number of parallel jobs to increase throughput
  • print error messages when building fails
  • run make info-boards-supported with EXTERNAL_BOARD_DIRS="" to avoid adding out-of-tree boards to Makefile.ci.
  • classify output as "not supported" also when used features are blacklisted, not only when required features are missing

add_insufficient_memory_board.sh:

  • classify output as "not supported" also when used features are blacklisted, not only when required features are missing

Testing procedure

Run the script; it should still work.

Issues/PRs references

None

@maribu maribu added Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Area: tools Area: Supplementary tools labels Feb 23, 2023
@maribu maribu requested a review from benpicco February 23, 2023 08:13
@benpicco benpicco added 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 labels Feb 23, 2023
@riot-ci
Copy link

riot-ci commented Feb 23, 2023

Murdock results

✔️ PASSED

fe176f5 dist/tools/insufficient_memory: Minor improvements

Success Failures Total Runtime
1 0 1 53s

Artifacts

@maribu
Copy link
Member Author

maribu commented Feb 23, 2023

bors merge

bors bot added a commit that referenced this pull request Feb 23, 2023
19303: dist/tools/insufficient_memory: Minor improvements r=maribu a=maribu

### Contribution description

- address all shellcheck warnings
- make script POSIX shell compatible
- use nproc to set the number of parallel jobs to increase throughput
- print error messages when building fails
- run `make info-boards-supported` with `EXTERNAL_BOARD_DIRS=""` to avoid adding out-of-tree boards to `Makefile.ci`.

### Testing procedure

Run the script; it should still work.

### Issues/PRs references

None

Co-authored-by: Marian Buschsieweke <[email protected]>
@maribu
Copy link
Member Author

maribu commented Feb 23, 2023

bors cancel

@bors
Copy link
Contributor

bors bot commented Feb 23, 2023

Canceled.

@maribu maribu force-pushed the dist/tools/insufficient_memory/create_makefile.ci.sh branch from e804706 to db106f8 Compare February 23, 2023 12:10
@maribu maribu requested a review from benpicco February 23, 2023 12:11
@maribu maribu force-pushed the dist/tools/insufficient_memory/create_makefile.ci.sh branch 2 times, most recently from 508fd4d to a779fd0 Compare February 23, 2023 12:28
@maribu
Copy link
Member Author

maribu commented Feb 23, 2023

Sorry, I forgot to also classify as not supported when a feature that is used is blacklisted (e.g. when an app depends on !arch_avr8), not only when a required feature is missing. I added that and updated the commit message. Should now be good.

@maribu maribu force-pushed the dist/tools/insufficient_memory/create_makefile.ci.sh branch from a779fd0 to 72bf076 Compare February 23, 2023 12:37
@maribu
Copy link
Member Author

maribu commented Feb 23, 2023

Another instance of missing not supported classification slipped through. I'll report back here when the script ran successfully and I'm (hopefully) done adding new magic phrases to correctly detect other instances of not supported.

`create_makefile.sh`:
- address all shellcheck warnings
- make script POSIX shell compatible
- use nproc to set the number of parallel jobs to increase throughput
- print error messages when building fails
- run `make info-boards-supported` with `EXTERNAL_BOARD_DIRS=""` to
  avoid adding out-of-tree boards to `Makefile.ci`.
- classify output as "not supported" also when used features are
  blacklisted, not only when required features are missing
- classify output as "not supported' also when output contains
  `not supported.  Stop.`, e.g. as raised by pkg/tinyusb on unsupported
  CPUs / CPU families.

`add_insufficient_memory_board.sh`:
- classify output as "not supported" also when used features are
  blacklisted, not only when required features are missing
- classify output as "not supported' also when output contains
  `not supported.  Stop.`, e.g. as raised by pkg/tinyusb on unsupported
  CPUs / CPU families.
@maribu maribu force-pushed the dist/tools/insufficient_memory/create_makefile.ci.sh branch from 72bf076 to fe176f5 Compare February 23, 2023 12:47
@maribu
Copy link
Member Author

maribu commented Feb 23, 2023

OK, looks like I have won this game of whack a mole this time :) Should now be ready for review

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 24, 2023

Build succeeded:

@bors bors bot merged commit 0dfc05c into RIOT-OS:master Feb 24, 2023
@maribu maribu deleted the dist/tools/insufficient_memory/create_makefile.ci.sh branch February 24, 2023 05:35
@maribu
Copy link
Member Author

maribu commented Feb 24, 2023

Thx :)


for BOARD in $(make --no-print-directory info-boards-supported -C "${APP_DIR}"); do
for BOARD in $(EXTERNAL_BOARD_DIRS="" make --no-print-directory info-boards-supported -C "${APP_DIR}"); do
Copy link
Contributor

Choose a reason for hiding this comment

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

hm for some reason this does not work, my list of EXTERNAL_BOARD_DIRS still gets iterated.

What works is doing

EXTERNAL_BOARD_DIRS= make generate-Makefile.ci

but that's easily forgotten and we end up with internal boards in Makefile.ci

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
for BOARD in $(EXTERNAL_BOARD_DIRS="" make --no-print-directory info-boards-supported -C "${APP_DIR}"); do
for BOARD in $(make EXTERNAL_BOARD_DIRS="" --no-print-directory info-boards-supported -C "${APP_DIR}"); do

Could you give this a try?

Copy link
Contributor

Choose a reason for hiding this comment

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

nope that doesn't make a difference

Copy link
Member Author

Choose a reason for hiding this comment

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

But, it should! 😢

@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants