-
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 3 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 |
|---|---|---|
|
|
@@ -139,6 +139,27 @@ def get_generator(path, cmake_args=None): | |
| return generator | ||
|
|
||
|
|
||
| def is_jobs_base_generator(path, cmake_args=None): | ||
| """ | ||
| Check if the used CMake generator supports jobs base arguments (-jN). | ||
|
|
||
| :param str path: The path of the directory contain the CMake cache file | ||
| :param list cmake_args: The CMake command line arguments | ||
| :rtype: bool | ||
| """ | ||
| known_jobs_base_multi_configuration_generators = ( | ||
| 'Ninja Multi-Config', | ||
|
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. 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. |
||
| ) | ||
| if not is_multi_configuration_generator(path, cmake_args): | ||
| # Historically we assume any non-multi-config generator is jobs based. | ||
| return True | ||
| generator = get_generator(path, cmake_args) | ||
| for multi in known_jobs_base_multi_configuration_generators: | ||
| if multi in generator: | ||
| return True | ||
| return False | ||
|
|
||
|
|
||
| def is_multi_configuration_generator(path, cmake_args=None): | ||
| """ | ||
| Check if the used CMake generator is a multi configuration generator. | ||
|
|
||
| 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_base_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_base_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_make_jobs_arguments(args, env) | ||||||
| if job_args: | ||||||
| cmd += ['--'] + job_args | ||||||
| completed = await run( | ||||||
|
|
@@ -281,7 +290,7 @@ def _get_msbuild_environment(self, args, env): | |||||
| env['CL'] = ' '.join(cl_split) | ||||||
| return env | ||||||
|
|
||||||
| def _get_make_arguments(self, env): | ||||||
| def _get_make_jobs_arguments(self, args, env): | ||||||
|
hidmic marked this conversation as resolved.
Outdated
|
||||||
| """ | ||||||
| Get the make arguments to limit the number of simultaneously run jobs. | ||||||
|
|
||||||
|
|
@@ -291,28 +300,37 @@ def _get_make_arguments(self, env): | |||||
| :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_args = args.cmake_jobs if args.cmake_jobs is not None else 0 | ||||||
| jobs = 0 | ||||||
| if jobs_args <= 0: | ||||||
|
hidmic marked this conversation as resolved.
Outdated
|
||||||
| # Base off the number of CPU cores if jobs arg non-positive. | ||||||
| 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 [] | ||||||
| # Finalize jobs as as CPU count deducting the limit specified. | ||||||
| jobs = max(jobs + jobs_args, 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 +343,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_base_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 +362,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_make_jobs_arguments(args, env) | ||||||
| if job_args: | ||||||
| cmd += ['--'] + job_args | ||||||
| return await run( | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.