Add 'console' kwarg to run_target to let them be run in parallel

[mortie]

run_target isn't always used for things which are singular long-running things which require direct console access like test suites. I have a project where I use one run_target per file for clang-tidy, like so:

if clang_tidy_prog.found()
  tidy_deps = []
  foreach path: tidy_files
    tidy_deps += run_target(
      '.tidy__' + path.replace('/', '__'),
      command: [
        clang_tidy_prog, '--quiet', '-p', meson.current_build_dir(),
        '--config-file=' + meson.current_source_dir() + '/.clang-tidy',
        meson.current_source_dir() + '/' + path,
      ],
    )
  endforeach
  alias_target('tidy', tidy_deps)
endif

Without this PR, those clang-tidy targets run sequentially. With this PR, I can add console: false to the run_target call and the targets run in parallel.

Note: This PR uses 0.65.0 as the since value for the console parameter. This might have to be changed.

[mortie]

I force-pushed a modified commit with the since changed to 1.0.0 after discussion in IRC.

[codecov[bot]]

Codecov Report

Merging #11142 (c2960ef) into master (9e8a3b9) will decrease coverage by 2.48%. The diff coverage is n/a.

:exclamation: Current head c2960ef differs from pull request most recent head 0ba7b63. Consider uploading reports for the commit 0ba7b63 to get more accurate results

@@            Coverage Diff             @@
##           master   #11142      +/-   ##
==========================================
- Coverage   68.44%   65.96%   -2.49%     
==========================================
  Files         412      207     -205     
  Lines       87964    44999   -42965     
  Branches    20776     9310   -11466     
==========================================
- Hits        60210    29685   -30525     
+ Misses      23219    12953   -10266     
+ Partials     4535     2361    -2174     

Impacted Files	Coverage Δ
scripts/clangtidy.py	0.00% <0.00%> (-93.34%)	:arrow_down:
modules/cuda.py	0.00% <0.00%> (-71.16%)	:arrow_down:
templates/cudatemplates.py	37.50% <0.00%> (-62.50%)	:arrow_down:
compilers/cuda.py	19.63% <0.00%> (-45.40%)	:arrow_down:
dependencies/cuda.py	19.23% <0.00%> (-42.79%)	:arrow_down:
compilers/cython.py	43.18% <0.00%> (-38.64%)	:arrow_down:
compilers/d.py	38.33% <0.00%> (-18.44%)	:arrow_down:
dependencies/coarrays.py	45.00% <0.00%> (-17.50%)	:arrow_down:
compilers/mixins/clike.py	70.12% <0.00%> (-6.26%)	:arrow_down:
cmake/executor.py	58.68% <0.00%> (-5.99%)	:arrow_down:
... and 241 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[mortie]

Done.

[tristan957]

The docs need updating. Also a release notes snippet

[jpakkane]

Maybe this should be called is_parallel to match the signature of test.

[xclaesse]

Maybe this should be called is_parallel to match the signature of test.

I agree the term console seems to be leaking ninja specific implementation. It also very badly describe what that does, even for ninja it's a terrible naming. OTOH, we already have console kwarg on custom_target(), so it seems we lost on consistency already... My personal suggestion would be to go with is_parallel and deprecate console on custom_target() too.

[eli-schwartz]

The point of the kwarg is very specifically to leak that "implementation detail".

There's no fundamental objective to preventing parallelism. Currently run_target disabled parallelism as a side effect of setting a ninja option that is intended to "guarantee that a target which is manually/interactively run, correctly shows up on the console".

That's why we depend on this implementation detail of ninja. Because... it isn't an implementation detail after all. It's the intended, documented, public purpose of that detail and we are using it for its intended, documented, public purpose.

The purpose of this PR is so that people can say "hey look actually I don't care about the console output of this target, so just do whatever feels right while ignoring console-ness".

Very hard NACK against naming it anything to do with parallelism.

[xclaesse]

There's no fundamental objective to preventing parallelism. Currently run_target disabled parallelism as a side effect of setting a ninja option that is intended to "guarantee that a target which is manually/interactively run, correctly shows up on the console".

TBH I see that the opposite way, the main goal is to disable parallelism, console output is a natural side effect of that.

[eli-schwartz]

TBH I see that the opposite way, the main goal is to disable parallelism, console output is a natural side effect of that.

I am not sure why you think that, considering that disabling parallelism is a subtly implied side effect of ninja's implementation of the console pool that is documented as:

It has the special property that any task in the pool has direct access to the standard input, output and error streams provided to Ninja, which are normally connected to the user’s console (hence the name) but could be redirected. This can be useful for interactive tasks or long-running tasks which produce status updates on the console (such as test suites).

While a task in the console pool is running, Ninja’s regular output (such as progress status and output from concurrent tasks) is buffered until it completes.

But you say that you see it contrary to the ninja documentation. Okay, let's roll with that. So -- who is it that specifically wants run_target to not run in parallel, but doesn't want to use -j1 for this, and doesn't want to mark some run_targets as depending on others? What is an example use case of the feature "disable the parallelism, I dislike parallelism"?

The classic use case for disabling parallelism but not with -j1, is for linking specifically -- meson uses backend_max_links for that.

[xclaesse]

But you say that you see it contrary to the ninja documentation. Okay, let's roll with that. So -- who is it that specifically wants run_target to not run in parallel, but doesn't want to use -j1 for this, and doesn't want to mark some run_targets as depending on others? What is an example use case of the feature "disable the parallelism, I dislike parallelism"?

When running any script that itself will spawn as many jobs as there are CPUs. Like running make/cargo in a custom target.

[mortie]

So, the console option (both in custom_target and now potentially in run_target) controls whether the spawned child process inherits the stdin/stdout/stderr file descriptors from the ninja process, or if the stdin is closed and stdout/stderr buffered. This has serious consequences which people care about (especially in the case of run_target):

• I can write a script which expects user input such as prompting the user for stuff. This doesn't even seem like an unintended use of run_target.
• Lots of tools change how their output looks based on whether stdout is a tty or not. Again, especially for run_target, there will be a lot of cases where the intention is for a human to invoke a run_target manually on the command line and look at the output.

There's a reason console=true is the default (and currently only) behavior, after all.

But then there are cases where we really don't care if the program inherits stdin/stdout/stderr or not. Running clang-format on a source file is a prime example of that.

So if we view the naming as UX, we have to decide whether we want to ask the question "Do you want this target to inherit its stdio from ninja?" or the question "Do you want to prevent this target from running in parallel with other targets where you have either implicitly or explicitly answered 'yes' to this question?". The phrasing of that second option is insane, but it's genuinely the best way I can think to phrase it:

• A run_target with is_parallel=false or an unspecified is_parallel would run in parallel with all other targets, except for other run_targets which also set is_parallel=false or leave is_parallel unspecified, or custom_targets which set is_parallel=false.
• A run_target with is_parallel=true would run in parallel with all other targets.
• A custom_target with is_parallel=false would run in parallel with all other targets, except for other other custom_targets which also set is_parallel=false, or run_targets which set is_parallel=false or leave is_parallel unspecified.
• A custom_target with is_parallel=true or an unspecified is_parallel would run in parallel with all other targets.

As a secondary effect, a run_target with is_parallel=false or an unspecified is_parallel would inherit its stdio from Ninja, and a custom_target with is_parallel=false (but not with an unspecified is-parallel) would inherit its stdio from Ninja. This is a non-obvious secondary effect which doesn't really make any sense unless you know the implementation detail that is_parallel=false is implemented using pool=console in the ninja file.

If we ask "Do you want this target to inherit its stdio from ninja?", the primary effect is simply whether the process inherits its stdio or gets buffered. run_target defaults to inheriting stdio, custom_target defaults to buffered stdio. And the secondary effect that only one process can inherit stdio at a time... makes all the sense in the world.

With tests, the situation is completely different. Whether tests inherit stdio or not is usually uninteresting, but we often care a whole lot about whether tests are run sequentially or in parallel with other tests. If I know my tests are messing with the filesystem or a database or anything else that's global state external to the process, I want the tests to run sequentially. If I know my tests are all isolated, I want the tests to run in parallel. If I have one test which I know messes with external global state, I want to ask Meson to run that test alone, but it can run all the other tests in parallel if it wants. In fact, I would probably prefer it if both is_paralllel=false tests and is_parallel=true tests use buffered output, just so that I don't have to deal with the test's behaviour differing based on the unrelated decision on whether to run them in parallel or not, and from what I can tell from the docs, the current implementation of test() agrees with me.

I would go as far as to argue that it would make sense to add a console option to test(). A test with console=false, is_parallel=true would run in parallel with everything, a test with console=false is_parallel=false would run in parallel with everything except other tests, one with console=true is_parallel=true would inherit stdio and run in parallel with everything which doesn't also inherit stdio, and one with console=true is_parallel=false would run in parallel with everything except for other tests or other targets which inherit stdio. The fact that every quadrant makes total sense indicates strongly to me that there is a very weak connection between test's is_parallel option and the other targets's console option, so trying to force some spurious naming consistency here would be a large mistake in my opinion.

This got very long, sorry. TL;DR: I don't think console should be renamed to is_parallel.

[eli-schwartz]

When running any script that itself will spawn as many jobs as there are CPUs. Like running make/cargo in a custom target.

This is totally bogus, because what you actually want is for ninja to have GNU make jobserver/client support. There are unofficial patches for client support, and some people distribute a ninja edition that has those patches.

With recent versions of GNU Make, jobserver cooperation is much easier, so you can launch make as your top-level program, have it run the kitware fork of ninja, and have that run another make, and both ninja and the custom_target() retrieve job slots from your coordinating Make, sharing resources.

You don't want to arbitrarily disable this just because of some Meson kwarg that says "please attach stdin/stdout/stderr directly to the console, don't buffer it" which you were abusing in order to work around the original lack of jobserver support.

If you do have meson.build files which abuse this, do they have a project option for -Dusing_kitware_ninja=true that disables the console hack?

[xclaesse]

This is totally bogus, because what you actually want is for ninja to have GNU make jobserver/client support

I wouldn't say it's "totally bogus" just because there might be unofficial patches somewhere to make it work in some cases.

But I did not know that "console" means stdin also gets redirected, that's a good explanation why that's called "console", it's not even mentioned in our doc:

Keyword argument conflicts with capture, and is meant for commands that are resource-intensive and take a long time to finish. With the Ninja backend, setting this will add this target to [Ninja's console pool](https://ninja-build.org/manual.html#_the_literal_console_literal_pool), which has special properties such as not buffering stdout and serializing all targets in this pool.

I think that doc does a poor job at saying what it does.

• No mention of stdin.
• What is a "pool"? That's a very cryptic way of saying "does not run in parallel to other jobs" IMHO. Not even sure that's what it really means.
• What does that kwarg on other backends?

I'm not against using the term "console", but maybe we could just tweak a bit the doc to better explains what are the guarantees Meson gives, regardless of the implementation details in ninja. I guess that's

• Not buffered stdout.
• Inherit stdin.
• Not run in parallel to other jobs.

[Akaricchi]

Not run in parallel to other jobs.

In the case of custom_target, it definitely is ran parallel to other non-console jobs, so that's not entirely accurate.

[xclaesse]

In the case of custom_target, it definitely is ran parallel to other non-console jobs, so that's not entirely accurate.

That needs to be documented properly then.

[eli-schwartz]

Ping?