`--parallel` improvements #13800

RuRo · 2021-07-30T22:07:41Z

Description

This PR has 2 improvements for --parallel support with make

parallel values less than 1 will now mean "unlimited" (see make --help | grep -- --jobs). This is not a breaking change, because currently non-positive parallel values fail with make: the '-j' option requires a positive integer argument.
If more than 1 job is used, the --output-sync option is also added. The stdout/stderr of each job will be collected by make and displayed synchronously, after each target is finished processing. This will fix [Feature Request] Prettier make output with parallel build #13707.

Types of Changes

Issues Fixed or Closed by This PR

fixes [Feature Request] Prettier make output with parallel build #13707

Checklist

My code follows the code style of this project: C, Python
I have read the PR Checklist document and have made the appropriate changes.
(Not sure, if this is a "Core PR", so submiting it against master for now)
My change requires a change to the documentation.
(Not sure if qmk --help counts as documentation and I didn't find any references to --parallel in the online docs)
I have updated the documentation accordingly.
(I updated the qmk --help for subcommands that have --parallel)
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

drashna · 2021-07-31T00:39:54Z

this should probably target develop

RuRo · 2021-07-31T00:43:07Z

this should probably target develop

Sure, I can open a new PR, that targets develop, if you want. I originally targeted master, because I assumed that "Core" mostly meant the stuff that's written in C like tmk_core.

tzarc · 2021-07-31T00:46:57Z

Sure, I can open a new PR, that targets develop

No need, this is sorted already.

RuRo · 2021-07-31T00:55:43Z

Oh, I didn't know, that GitHub allows changing the target branch after the fact. Neat. Do you want me to force-push the changes with the resolved conflict for develop?

drashna · 2021-07-31T01:03:14Z

Oh, I didn't know, that GitHub allows changing the target branch after the fact. Neat. Do you want me to force-push the changes with the resolved conflict for develop?

Yes, please. And that conflict is part of my concern, since I know that a lot of CLI stuff got changed.

skullydazed

Good change! Besides my recommendations this will need a documentation update before merge.

lib/python/qmk/cli/compile.py

lib/python/qmk/cli/flash.py

lib/python/qmk/cli/multibuild.py

lib/python/qmk/commands.py

RuRo · 2021-07-31T03:50:20Z

AFAIK, the --parallel flags aren't documented anywhere in the docs, so I am not sure, where should it be documented if not in the --help.

skullydazed · 2021-07-31T03:53:45Z

AFAIK, the --parallel flags aren't documented anywhere in the docs, so I am not sure, where should it be documented if not in the --help.

If it's not in docs/cli_commands.md that's an oversight. Seems like a good time to fix that. :)

RuRo · 2021-07-31T04:01:48Z

If it's not in docs/cli_commands.md that's an oversight. Seems like a good time to fix that. :)

Okay, I'll add it to docs/cli_commands.md, but I still think that the special -j 0 variant should be mentioned in --help.
Do you have any reasons for removing it?

skullydazed · 2021-07-31T04:15:09Z

Honestly, I think it may be more about the parens than anything else. If the information needs to be there it should be worked in naturally. The example from make itself is an ok (but not good) example of what I'm looking for:

  -j [N], --jobs[=N]          Allow N jobs at once; infinite jobs with no arg.

That particular wording assumes more knowledge than I wish it did.

RuRo · 2021-07-31T04:40:42Z

Honestly, I think it may be more about the parens than anything else. If the information needs to be there it should be worked in naturally.

There are 2 hard problems in computer science... apparently, writing helpful help messages is one of them 😆.

I really can't come up with anything better. The problem stems from the fact that make has pretty weird --jobs semantics itself, so qmk compile does too. How about just changing the parentheses to a semicolon like in make:
Set the number of parallel make jobs; 0 means unlimited.?

Also, I've added some documentation to docs/cli_commands.md.

zvecr · 2021-07-31T13:01:41Z

--output-sync currently messes with the :flash code (which is used by qmk flash). You can end up with zero output sometimes, looking like it's hanging:

Checking file size of ergosaurus_default.hex                                                        [OK]
 * The firmware size is fine - 23192/28672 (80%, 5480 bytes free)






^CWaiting for USB serial port - reset your controller now (Ctrl+C to cancel)..............make[1]: *** [tmk_core/avr.mk:338: flash] Interrupt
make: *** [Makefile:529: ergosaurus:default:flash] Interrupt

I dont get the "port - reset your controller now" till i ctrl+c.

With that, it might be only qmk compile that can have the proposed change.

RuRo · 2021-07-31T13:20:25Z

@zvecr Unfortunately, I am currently unable to test the flash step as I temporarily don't have my keyboard. ~~Can you double check, that qmk flash -j N (N>1) works for you with the current develop branch?~~

--output-sync just buffers the text output to the terminal, so in theory it shouldn't affect the actual result. Keep in mind, that with --output-sync the output/progress of each build target is only printed after the target is completed, so it might look like it's stuck. From the output, it seems that you killed the flash process early.

Edit: nvm, I understand that the problem is just that the prompt doesn't appear, since it's buffered. I'll take a look at what can be done to fix this. Probably tomorrow tho.

RuRo · 2021-07-31T16:13:18Z

Okay, I think, I've found a solution. @zvecr can you please test qmk flash with the latest changes?

It's not that pretty of a solution, but it should work. Basically, --output-sync=target only buffers the outputs from "normal" commands. Recursive make commands are only buffered with --output-sync=recurse.

A leading + plus sign tells make to treat the following line as if there is a $(MAKE) variable present, which temporarily disables the buffering. After that, we just need to add $(UNSYNC_OUTPUT_CMD) to every command, that requires user interaction and should not be buffered (like the various flash targets).

For now, I added it to arm_atsam.mk, avr.mk and chibios.mk in:

EXEC_TEENSY
EXEC_DFU/EXEC_DFU_UTIL
EXEC_AVRDUDE
EXEC_USBASP
EXEC_BOOTLOADHID
EXEC_HID_LUFA
custom PROGRAM_CMD for the flash targets

Not sure, if I missed anything.

RuRo · 2021-08-07T00:09:44Z

Btw, I checked qmk flashing in parallel with my moonlander and the prompt now appears as expected.

RuRo · 2021-08-11T21:53:53Z

@zvecr can you please verify, that your issue was resolved with the latest changes?

drashna

Have tested with stm32-dfu, atmel-dfu, and teensy halfkay bootloader, and all work as expected.

RuRo · 2021-08-17T11:11:50Z

I've resolved the conflicts with recent refactoring changes, that moved parts of tmk_core/*.mk to platforms/*/flash.mk.

Also, I'm marking the changes requested by @skullydazed as resolved for now. I've added the required documentation and the long options. The remaining requested changes don't seem to apply any more. Feel free to reopen them if I've missed anything.

drashna · 2021-08-25T06:17:54Z

Hmm, this breaks the :bootloader target (for avr).

Specifically, outputs:

Making handwired/tractyl_manuform/5x6_right with keymap drashna and target bootloader

make[2]: Nothing to be done for `clean'.
touch obj/BootloaderDFU.o
touch obj/Descriptors.o
touch BootloaderDFU.elf
touch BootloaderDFU.hex
touch BootloaderDFU.eep
touch BootloaderDFU.bin
touch BootloaderDFU.lss
touch BootloaderDFU.sym
BootloaderDFU.hex copied to handwired_tractyl_manuform_5x6_right_drashna_bootloader.hex

Rather than actually generating the bootloader. Using the make command version works just fine, though.

RuRo · 2021-08-25T10:00:22Z

@drashna

I am not sure, if I understand, what is the issue here. Can you provide more info?

What command did you run? What did you expect to happen? What happened instead? Is it still working as expected, if you run it without --parallel jobs? Was it working as expected before this PR was merged? Is the lufa submodule clean and unmodified?

Are you running qmk flash -kb handwired/tractyl_manuform/5x6_right -km drashna -bl bootloader? Are you sure, that this is a correct usage? Running this command with -b lists the following valid options for the bootloader:

Ψ Here are the available bootloaders:
	dfu
	dfu-ee
	dfu-split-left
	dfu-split-right
	avrdude
	BootloadHID
	dfu-util
	dfu-util-split-left
	dfu-util-split-right
	st-link-cli
	st-flash
For more info, visit https://docs.qmk.fm/#/flashing

Running make manually prints Please set BOOTLOADER to "qmk-dfu" or "qmk-hid" first!. Stop. for me. Did you modify the rules.mk?

After manually setting the BOOTLOADER option to qmk-dfu in rules.mk, the qmk flash ... -bl bootloader command seems to produce almost the same output as make ...:bootloader.

RuRo · 2021-08-31T22:04:31Z

It's that time of the week, where I go around all the PRs I am participating in and ping everyone!

@drashna am I correct in assuming, that you've resolved your issue?

* improve make parallel jobs support * document the --parallel option * disable the output-sync for interactive targets

github-actions bot added cli qmk cli command python labels Jul 30, 2021

zvecr requested review from skullydazed, Erovia and a team July 30, 2021 23:47

tzarc approved these changes Jul 31, 2021

View reviewed changes

drashna requested a review from a team July 31, 2021 00:40

tzarc changed the base branch from master to develop July 31, 2021 00:46

skullydazed requested changes Jul 31, 2021

View reviewed changes

github-actions bot added the documentation label Jul 31, 2021

github-actions bot added the core label Jul 31, 2021

RuRo requested a review from skullydazed August 11, 2021 21:53

drashna approved these changes Aug 12, 2021

View reviewed changes

RuRo added 2 commits August 17, 2021 13:39

improve make parallel jobs support

e14894a

document the --parallel option

9e2fad6

disable the output-sync for interactive targets

f82f0f5

tzarc merged commit 3b28178 into qmk:develop Aug 17, 2021

nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021

--parallel improvements (qmk#13800)

5717876

* improve make parallel jobs support * document the --parallel option * disable the output-sync for interactive targets

BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024

--parallel improvements (qmk#13800)

dd16f1e

* improve make parallel jobs support * document the --parallel option * disable the output-sync for interactive targets

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`--parallel` improvements #13800

`--parallel` improvements #13800

RuRo commented Jul 30, 2021 •

edited

Loading

drashna commented Jul 31, 2021

RuRo commented Jul 31, 2021

tzarc commented Jul 31, 2021

RuRo commented Jul 31, 2021 •

edited

Loading

drashna commented Jul 31, 2021

skullydazed left a comment

RuRo commented Jul 31, 2021

skullydazed commented Jul 31, 2021

RuRo commented Jul 31, 2021

skullydazed commented Jul 31, 2021

RuRo commented Jul 31, 2021

zvecr commented Jul 31, 2021

RuRo commented Jul 31, 2021 •

edited

Loading

RuRo commented Jul 31, 2021 •

edited

Loading

RuRo commented Aug 7, 2021

RuRo commented Aug 11, 2021

drashna left a comment

RuRo commented Aug 17, 2021

drashna commented Aug 25, 2021

RuRo commented Aug 25, 2021

RuRo commented Aug 31, 2021

--parallel improvements #13800

--parallel improvements #13800

Conversation

RuRo commented Jul 30, 2021 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

drashna commented Jul 31, 2021

RuRo commented Jul 31, 2021

tzarc commented Jul 31, 2021

RuRo commented Jul 31, 2021 • edited Loading

drashna commented Jul 31, 2021

skullydazed left a comment

Choose a reason for hiding this comment

RuRo commented Jul 31, 2021

skullydazed commented Jul 31, 2021

RuRo commented Jul 31, 2021

skullydazed commented Jul 31, 2021

RuRo commented Jul 31, 2021

zvecr commented Jul 31, 2021

RuRo commented Jul 31, 2021 • edited Loading

RuRo commented Jul 31, 2021 • edited Loading

RuRo commented Aug 7, 2021

RuRo commented Aug 11, 2021

drashna left a comment

Choose a reason for hiding this comment

RuRo commented Aug 17, 2021

drashna commented Aug 25, 2021

RuRo commented Aug 25, 2021

RuRo commented Aug 31, 2021

`--parallel` improvements #13800

`--parallel` improvements #13800

RuRo commented Jul 30, 2021 •

edited

Loading

RuRo commented Jul 31, 2021 •

edited

Loading

RuRo commented Jul 31, 2021 •

edited

Loading

RuRo commented Jul 31, 2021 •

edited

Loading