In general I sympathize with @davydden here. Also a minor naming comment: I think the splitted functions should be named something like _make_has_target and ninja_has_target instead of _if_ninja_target. That in my opinion sounds better when reading the code. For instance:

if self._make_has_target('test'):
    inspect.getmodule(self).ctest()

versus:

if self._if_make_target('test'):
    inspect.getmodule(self).ctest()

These changes would make #8223 much easier to unit test.

how does one use it? Could you please add a sentence in class documentation?

there is a dedicated PR on documentation for build systems #5015 but it is not merged yet.

@davydden If you're talking specifically about the cmake.py change you noted above, "one" doesn't "use" it. It's a change in the implementation to provide the parallelism that one could reasonably expect for the test target in the event that it is CMake-generated (i.e. invokes ctest). This implementation uses the expanded functionality of MakeExecutable, which is actually explained in the class documentation therefor.
In your case, where the test target is hand-provided by your package rather than generated by CMake, there will be no effect unless (of course) your target invokes ctest.

@davydden I'm happy that it does what it set out to do. If everyone else thinks it's a reasonable implementation, I'm happy for it to be merged.

IMO the following would be more straightforward (I don't see anything setting the new jobs_arg variable so IMO it isn't needed):

jobs_arg = 'j{0}'.format(self.jobs)
args = (jobs_arg,) + args
if jobs_env:
  ...

(I also think this avoids setting multiple jobs arguments, which may be valid and appears to be handled here, but I'm not aware of any system that needs it so I'd prefer to avoid the complexity)

Does it make sense to avoid adding a -jN argument when jobs_env is set?

I think it's worth adding a note that you are using kwargs.pop because jobs_env is intended for use by this function specifically and that the remaining kwargs are to be passed to the make invocation.

jobs_arg is there for a little more flexibility for new make-like execs, for wrappers using long-form options (ctest has a --parallel alias, for example) or separating the N from the -j, and for the case where one actually does want to avoid -jN when job_env is set. Note that in the case jobs_env was put in place to address (make test for CMake projects) we can't assume for the general CMake case because any given project can provide its own make test target which may or may not honor -jN. @davydden mentioned dealii as a package for which this is done.

I can certainly add a note about the use of kwargs.pop(). I'll defer to you on whether I should go for simplicity or flexibility on jobs_arg -- let me know what you think after my explanation.

Is there any known case where we can't just supply a flag? Like could we do make -j 4 instead of make -j4? That would make things a bit simpler in my mind. I suppose there could be a flag that means "use all cores" and no option to supply a specific number. But I think that would break this anyway? That wouldn't need to be a MakeExecutable anyway.

@adamjstewart The generated test target for CMake projects requires the CTEST_PARALLEL_LEVEL environment variable to be set for parallelism (ignoring the -j option to make), which is the whole reason for this PR. Different make-like applications handle non-specific parallelism differently anyway: ninja defaults to core-level parallelism without any -j, whereas make omits the number for "infinite" parallelism. It's messy. I could always go back to the old system of assuming -j{0} in the absence of use cases, at the cost of flexibility for the future. Is that the consensus?

Personally, I would be fine with the previous behavior, the proposed behavior, or the behavior I'm suggesting. To be clear, what I'm suggesting is replacing jobs_arg='-j{0}' with jobs_flag='-j'. The only difference from our current behavior would be adding a space after the -j.

But if you want to avoid debate and just get this merged, I would avoid making changes to the UI that aren't absolutely necessary until we find a use case for those changes. Although @alalazo and I are generally fans of making things as generic as possible...

for wrappers using long-form options (ctest has a --parallel alias, for example)

Like a wrapper around ctest, called ctest, which only accepts --parallel?

separating the N from the -j

Do any systems require this?

and for the case where one actually does want to avoid -jN when job_env is set

How does one accomplish that? e.g. if jobs_arg=None the next line the line args = tuple([j.format(self.jobs) for j in jobs]) + args fails. The only way I see to do that now is to pass an empty iterable container, which IMO is not straightforward.

Also, I didn't see any of those cases explaining the handling of jobs_arg as a tuple (other than my inference on the last one)

Overall I'd be fine if jobs_arg was a single argument like jobs_env.

I have removed the currently-unneeded flexibility, leaving us with the minimum changes required to provide parallel test execution under CMake.

IMO the following is clearer:

If specified, jobs_env names the environment variable which should be used to control the level of parallelism

Sounds reasonable, but I wanted to avoid giving the impression that the -j{} behavior would be turned off. How about:

If specified, jobs_env names an environment variable to be set to the number of parallel jobs used.
?

That sounds good to me, although IMO if you want to make sure the user knows that jobs_env doesn't turn off the -j argument, that stating it explicitly would benefit either explanation.

@scheibelp I think the latest commit resolves all your issues. Please let me know whether you concur.

This looks strange to me comparing to the next line: it looks like make_env can be a list. If jobs_env is a string, it looks like the next line will fail (attempt to index a list with a string). Should the default be {}?

The default should be {}, correct. Apparently the existing tests don't attempt to run the tests for any CMake packages with parallelism enabled.

@scheibelp Should I try to invoke a package build somehow that runs the package's tests? I'm not at all familiar with this part of the testing system.

For this sort of error (and to just cover the lines of code) you could add a simple test to the make_executable python test module (where jobs_env is set).

In Executable.__call__, if env is set at all (including if it's an empty container), then Executable will use it. Overall the effect for MakeExecutable is that if the user:

• sets job_env
• does not set env

that the Executable.__call__ will run with an empty environment, which changes the semantics (normally if you don't set env, it uses the current environment). Was that what you were going for?

@scheibelp Genuinely not sure what to do here. In executable.py:

   env_arg = kwargs.get('env', None)
   if env_arg is None:
       env = os.environ.copy()
       env.update(self.default_env)
   else:
       env = self.default_env.copy()
       env.update(env_arg)

In other words, if the environment is not specified, we overlay our defaults on the current environment; otherwise, we overlay our specified environment on the defaults. It would seem that the desirable behavior in the case of specifying jobs_env would be, "use the environment you were always going to use, but with this added. In the case where env was not specified this would have to be, "The current environment with default_env overlaid on top and then jobs_env added to it." This would not, of course, be robust against changes to executable.py. Thoughts?

It would seem that the desirable behavior in the case of specifying jobs_env would be, "use the environment you were always going to use, but with this added["]

IMO that is the most sensible behavior (e.g. to keep CC etc.)

but with this added. In the case where env was not specified this would have to be, "The current environment with default_env overlaid on top and then jobs_env added to it."

I think I see what you mean: to get the semantics of default_env, you would need to give it priority in MakeExecutable when env was not specified, if Executable were to change that later, MakeExecutable may get left behind.

I don't think you have to deal with that here. However, if desired, you could make it more robust by adding an extra_environment argument to Executable.__call__ that doesn't define the whole environment but rather overlays the specified arguments on top. In that case MakeExecutable would be simplified.

A couple requests:

• I think this should be dump_env (there are no references to env_dump, but there are to dump_env)
• This appears to be used only for testing. IMO it's worth clarifying that in the explanation and also adding a _ to the prefix, i.e. _dump_env.

@scheibelp I wasn't sure what the policy on testing-only was, but I can do that.