-
Notifications
You must be signed in to change notification settings - Fork 30
Add support for controlling the number of jobs used by some cmake tasks #107
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
base: master
Are you sure you want to change the base?
Changes from 7 commits
3848b2a
3b8132e
9bcbaec
c726514
9fc9315
4bb784c
9afca3b
2b936c2
3c2ba6f
2b4dcdd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |||||
| from colcon_cmake.task.cmake import get_variable_from_cmake_cache | ||||||
| from colcon_cmake.task.cmake import get_visual_studio_version | ||||||
| from colcon_cmake.task.cmake import has_target | ||||||
| from colcon_cmake.task.cmake import is_jobs_based_generator | ||||||
| from colcon_cmake.task.cmake import is_multi_configuration_generator | ||||||
| from colcon_core.environment import create_environment_scripts | ||||||
| from colcon_core.logging import colcon_logger | ||||||
|
|
@@ -63,6 +64,12 @@ def add_arguments(self, *, parser): # noqa: D102 | |||||
| '--cmake-force-configure', | ||||||
| action='store_true', | ||||||
| help='Force CMake configure step') | ||||||
| parser.add_argument( | ||||||
| '--cmake-jobs', | ||||||
| type=int, | ||||||
| help='Number of jobs to use for supported generators (e.g., Ninja ' | ||||||
| 'Makefiles). Negative values subtract from the maximum ' | ||||||
| 'available, so --jobs=-1 uses all bar 1 available threads.') | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a typo |
||||||
|
|
||||||
| async def build( # noqa: D102 | ||||||
| self, *, additional_hooks=None, skip_hook_creation=False, | ||||||
|
|
@@ -221,6 +228,8 @@ async def _build(self, args, env, *, additional_targets=None): | |||||
| if additional_targets: | ||||||
| targets += additional_targets | ||||||
|
|
||||||
| jobs_base_generator = is_jobs_based_generator( | ||||||
| args.build_base, args.cmake_args) | ||||||
| multi_configuration_generator = is_multi_configuration_generator( | ||||||
| args.build_base, args.cmake_args) | ||||||
| if multi_configuration_generator: | ||||||
|
|
@@ -240,8 +249,8 @@ async def _build(self, args, env, *, additional_targets=None): | |||||
| cmd += ['--clean-first'] | ||||||
| if multi_configuration_generator: | ||||||
| cmd += ['--config', self._get_configuration(args)] | ||||||
| else: | ||||||
| job_args = self._get_make_arguments(env) | ||||||
| if jobs_base_generator: | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, the removal of the |
||||||
| job_args = self._get_jobs_arguments(args, env) | ||||||
| if job_args: | ||||||
| cmd += ['--'] + job_args | ||||||
| completed = await run( | ||||||
|
|
@@ -281,38 +290,53 @@ def _get_msbuild_environment(self, args, env): | |||||
| env['CL'] = ' '.join(cl_split) | ||||||
| return env | ||||||
|
|
||||||
| def _get_make_arguments(self, env): | ||||||
| def _get_jobs_arguments(self, args, env): | ||||||
| """ | ||||||
| Get the make arguments to limit the number of simultaneously run jobs. | ||||||
|
|
||||||
| The arguments are chosen based on the `cpu_count`, e.g. -j4 -l4. | ||||||
|
|
||||||
| This only handles some generators, specifically; Makefiles and Ninja | ||||||
| derivatives. | ||||||
|
KazNX marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| :param dict env: a dictionary with environment variables | ||||||
| :returns: list of make arguments | ||||||
| :rtype: list of strings | ||||||
| """ | ||||||
| # check MAKEFLAGS for -j/--jobs/-l/--load-average arguments | ||||||
| makeflags = env.get('MAKEFLAGS', '') | ||||||
| regex = ( | ||||||
| r'(?:^|\s)' | ||||||
| r'(-?(?:j|l)(?:\s*[0-9]+|\s|$))' | ||||||
| r'|' | ||||||
| r'(?:^|\s)' | ||||||
| r'((?:--)?(?:jobs|load-average)(?:(?:=|\s+)[0-9]+|(?:\s|$)))' | ||||||
| ) | ||||||
| matches = re.findall(regex, makeflags) or [] | ||||||
| matches = [m[0] or m[1] for m in matches] | ||||||
| if matches: | ||||||
| # do not extend make arguments, let MAKEFLAGS set things | ||||||
| return [] | ||||||
| # Use the number of CPU cores | ||||||
| jobs = os.cpu_count() | ||||||
| with suppress(AttributeError): | ||||||
| # consider restricted set of CPUs if applicable | ||||||
| jobs = min(jobs, len(os.sched_getaffinity(0))) | ||||||
| if jobs is None: | ||||||
| # the number of cores can't be determined | ||||||
| return [] | ||||||
| generator = get_generator(args.build_base) | ||||||
| if 'Makefiles' in generator and args.cmake_jobs is None: | ||||||
| # check MAKEFLAGS for -j/--jobs/-l/--load-average arguments | ||||||
| # Note: Ninja does not support environment variables. | ||||||
| makeflags = env.get('MAKEFLAGS', '') | ||||||
| regex = ( | ||||||
| r'(?:^|\s)' | ||||||
| r'(-?(?:j|l)(?:\s*[0-9]+|\s|$))' | ||||||
| r'|' | ||||||
| r'(?:^|\s)' | ||||||
| r'((?:--)?(?:jobs|load-average)(?:(?:=|\s+)[0-9]+|(?:\s|$)))' | ||||||
| ) | ||||||
| matches = re.findall(regex, makeflags) or [] | ||||||
| matches = [m[0] or m[1] for m in matches] | ||||||
| if matches: | ||||||
| # do not extend make arguments, let MAKEFLAGS set things | ||||||
| return [] | ||||||
| # Use command line specified jobs if positive. | ||||||
| jobs = 0 | ||||||
| if args.cmake_jobs is not None: | ||||||
| jobs = args.cmake_jobs | ||||||
| # If positive, use jobs as is, even if it's more than available. | ||||||
| # Excessive jobs specified is a user error. | ||||||
| if jobs <= 0: | ||||||
| # Base off the number of CPU cores if jobs arg non-positive. | ||||||
| cores = os.cpu_count() | ||||||
| with suppress(AttributeError): | ||||||
| # consider restricted set of CPUs if applicable | ||||||
| cores = min(cores, len(os.sched_getaffinity(0))) | ||||||
| if cores is None: | ||||||
| # the number of cores can't be determined | ||||||
| return [] | ||||||
| # Finalize jobs as as CPU count deducting the limit specified. | ||||||
| jobs = max(cores + jobs, 1) | ||||||
| return [ | ||||||
| '-j{jobs}'.format_map(locals()), | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rename of the function is based on this result. It can only return jobs. Will revert if appropriate. |
||||||
| '-l{jobs}'.format_map(locals()), | ||||||
|
|
@@ -325,7 +349,8 @@ async def _install(self, args, env): | |||||
| raise RuntimeError("Could not find 'cmake' executable") | ||||||
| cmd = [CMAKE_EXECUTABLE] | ||||||
| cmake_ver = get_cmake_version() | ||||||
| allow_job_args = True | ||||||
| allow_job_args = is_jobs_based_generator( | ||||||
| args.build_base, args.cmake_args) | ||||||
| if cmake_ver and cmake_ver >= parse_version('3.15.0'): | ||||||
| # CMake 3.15+ supports invoking `cmake --install` | ||||||
| cmd += ['--install', args.build_base] | ||||||
|
|
@@ -343,8 +368,8 @@ async def _install(self, args, env): | |||||
| args.build_base, args.cmake_args) | ||||||
| if multi_configuration_generator: | ||||||
| cmd += ['--config', self._get_configuration(args)] | ||||||
| elif allow_job_args: | ||||||
| job_args = self._get_make_arguments(env) | ||||||
| if allow_job_args: | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||||||
| job_args = self._get_jobs_arguments(args, env) | ||||||
| if job_args: | ||||||
| cmd += ['--'] + job_args | ||||||
| return await run( | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally made this change on top of PR 106 then pulled this out. You can view this as a defensive or pre-emptive change. It should have little to no impact until then as the ninja multi-config won't really work yet.