search

colcon / colcon-cmake

Extension for colcon to support CMake packages
http://colcon.readthedocs.io
Apache License 2.0
16 stars 25 forks source link

Add support for controlling the number of jobs used by some cmake tasks #107

Open KazNX opened 2 years ago

KazNX commented 2 years ago

This change adds support for build arg --cmake-jobs to set the number of jobs to use to preserve CPU for the OS. It also renames _get_make_arguments() to _get_make_jobs_arguments() to better reflect the responsibilities of the function.

There is currently support to set the number of job threads to the maximum available, unless the MAKEFLAGS environment variable is set. The assumption is that MAKEFLAGS will be used by the build framework to control the number of jobs. However, this is only true for make - as far as I can tell, ninja does not have any such environment variable based control (related issues reported against ninja 922, 1482).

When MAKEFLAGS is not set, colcon will maxout the -j argument for ninja. Since ninja is so efficient, this can make the computer unresponsive for the OS and other applications. This change has been made to allow the OS to remain somewhat responsive.

As part of the change, I noted that _get_make_arguments() appears to be misnamed as the only thing it does is set the number of jobs to use, as evidenced by the use of the returned values. I've renamed it _get_make_jobs_arguments() to better reflect this. Happy to revert this change if required.

The --cmake-jobs args supports the following usage patterns:

KazNX commented 2 years ago

PR has been rebased onto master fixing the CI check failures.

KazNX commented 2 years ago

@cottsay Looking at the CMake 3.12 release notes, it looks like they added --parallel and --jobs arguments as well as CMAKE_BUILD_PARALLEL_LEVEL. This is great, but I need to target a minimum version of CMake 3.10 for now (the default CMake version for Ubuntu 18.04).

This changes the context of this PR a bit and I see the need to revise the original code I modified to deal with that as well. I propose the ideal solution is to use the changes I've made with CMake < 3.12. For CMake 3.12+, change this function to return --parallel and --jobs instead, which special provisions not to append these after -- is added to the command line. _is_jobs_based_generator() can then return True in call cases.

My question is, are the changes better made in this PR or a to push this through and start a new PR?

cottsay commented 2 years ago

are the changes better made in this PR or a to push this through and start a new PR?

I see two conceptual changes:

  1. We should to stop mucking with the MAKEFLAGS if CMAKE_BUILD_PARALLEL_LEVEL is specified in the environment, which would support this use case for any system with CMake 3.12 or newer. This should be a trivial change. We should not remove the existing MAKEFLAGS arguments outright in favor of CMAKE_BUILD_PARALLEL_LEVEL because it uses --load-average, which achieves different behavior than --jobs alone.
  2. We should introduce a mechanism to support your use case with CMake older than 3.12. I'm not sure if a full-fledged command line option is the right move or something a little more "hidden" like an environment variable. Maybe a deprecated command line option would be good - or one that shows a deprecation warning if you're already using 3.12 and should switch to the supported environment variable for doing this.

The latter can be done in this PR, I think. The former should probably be done in a new PR. Thoughts?

KazNX commented 2 years ago

I see two conceptual changes:

I fully agree with point 1, but would add that it's not just using CMAKE_BUILD_PARALLEL_LEVEL. It needs to be either/or with passing cmake args for --parallel and/or --jobs. I forsee some annoying permutations and need to learn more about how these interact.

On point 2, the deprecation warning only makes sense if CMAKE_BUILD_PARALLEL_LEVEL or --parallel and/or --jobs are present. Something like:

if cmake_ver >= 3.12 and (have_env_var('CMAKE_BUILD_PARALLEL_LEVEL') or have_cmake_arg('--parallel') or have_cmake_arg('--jobs')):
  print('deprecation warning')

Again, I need more information aboug how the command line arguments interact with CMAKE_BUILD_PARALLEL_LEVEL. Will get back to you on that.

KazNX commented 2 years ago

FYI, -j --parallel and CMAKE_BUILD_PARALLEL_LEVEL are all the same. Command line help from CMake 3.12;

-j [] --parallel [] = Build in parallel using the given number of jobs. If is omitted the native build tool's default number is used. The CMAKE_BUILD_PARALLEL_LEVEL environment variable specifies a default parallel level when this option is not given.

KazNX commented 2 years ago

@cottsay @hidmic I've made a significant revision of this PR based on CMake 3.12+ supporting --parallel or -jN. Note that it sill uses a new colcon build argument --cmake-jobs in all cases. I can't use --cmake-args because that only affects CMake configuration and we need to inject arguments into cmake --build.

The changes are summaried as:

Note we no longer check if we have a jobs based generator. This is now managed internally to _get_jobs_arguments().

Also note we now add -- inside _get_jobs_arguments() as it is not needed for CMake 3.12+

dirk-thomas commented 2 years ago

Since I don't have the spare time to review this change I leave it up to the co-maintainers to review and test this.

A general - fairly significant - concern I have: the Travis CI integration for all colcon repositories has been broken for a while. Merging any changes has an increased risk to introduce regressions. I strongly suggest to first "get the house in order" and reestablish CI coverage for the lost platforms - e.g. by adding GitHub action support to all repos.

KazNX commented 2 years ago

@cottsay @hidmic Correct me if I'm wrong, but I read Dirk's comments as concerns over the current state of master rather than a applying to this PR. In that light, are there any further changes required to get this PR merged?

cottsay commented 2 years ago

...over the current state of master rather than a applying to this PR.

Neither this PR nor master are getting test coverage on Linux or macOS because of the missing Travis builds.

are there any further changes required to get this PR merged?

I'm still not feeling great about merging a PR that adds a command line option to support such a narrow use case, especially one that has an official mechanism available in newer versions of CMake.

Here's a completely different suggestion - could we expose a free-form --cmake-build-args option to correspond to the existing free-form --cmake-args option? That would be useful outside of the Ninja use case, and wouldn't be obsoleted by CMake 3.12+.

KazNX commented 2 years ago

Here's a completely different suggestion - could we expose a free-form --cmake-build-args option to correspond to the existing free-form --cmake-args option? That would be useful outside of the Ninja use case, and wouldn't be obsoleted by CMake 3.12+.

Yes, I like this option. It does mean removing the _get_jobs_arguments() method (previously _get_make_arguments()) completely because to prevent it from adding implied -j arguments.

KazNX commented 2 years ago

I've created a new PR for the --cmake-build-args approach. See here https://github.com/colcon/colcon-cmake/pull/110

I managed to maintain _get_make_arguments(), though I think that functionality is questionable.

We can deprecate this PR if the 110 is the preferred solution.