Skip to content
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.

Sign up for GitHub

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

Args options for CMake --build and --install #110

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Args options for CMake --build and --install #110

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6d8dc3
Args options for CMake `--build` and `--install`
Sep 28, 2021
fec5892
Fix typo
KazNX Oct 10, 2021
013736d
Merge branch 'master' into feature/cmake-build-args
cottsay Dec 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 64 additions & 4 deletions colcon_cmake/task/cmake/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ def add_arguments(self, *, parser): # noqa: D102
'--cmake-force-configure',
action='store_true',
help='Force CMake configure step')
parser.add_argument(
'--cmake-build-args',
nargs='*', metavar='*', type=str.lstrip,
help="Pass arguments to 'cmake --build'. "
'Arguments matching other options must be prefixed by a space')
parser.add_argument(
'--cmake-install-args',
nargs='*', metavar='*', type=str.lstrip,
help="Pass arguments to 'cmake --install'. "
'Arguments matching other options must be prefixed by a space')

async def build( # noqa: D102
self, *, additional_hooks=None, skip_hook_creation=False,
Expand Down Expand Up @@ -206,6 +216,40 @@ def _store_cmake_args(self, build_base, cmake_args):
def _get_last_cmake_args_path(self, build_base):
return Path(build_base) / 'cmake_args.last'

def _have_jobs_arg(self, cmd, env):
"""
Check the given command line argument list looking for jobs args.

There are a number of ways to specify the number of jobs to use. This
function checks if any known mechanisms are present. This checks for
the following patterns: [-j#, -j #, --parallel, '-- -j']. The last item
is for pre CMake 3.12, while the prior options are for 3.12+ where
CMake knows how to pass jobs to the make system.

:param list cmd: Command line argument list for the build command.
:returns: True when a jobs argument is found in cmd.
"""
have_dashes = False
cmake_has_jobs = False
cmake_ver = get_cmake_version()
if cmake_ver and cmake_ver >= parse_version('3.12.0'):
cmake_has_jobs = True
# Respect CMAKE_BUILD_PARALLEL_LEVEL environment variable.
cmake_parallel_level = env.get('CMAKE_BUILD_PARALLEL_LEVEL', '')
if cmake_parallel_level:
return True
for arg in cmd:
if arg == '--':
# Now passing through args to the make system
have_dashes = True
if re.match('-j[0-9]*', arg) and (cmake_has_jobs or have_dashes):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex is insufficient to catch other job-like flags in Make.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With respect to the functional overlap, I agree there's some conceptual overlap between _have_job_arg and _get_make_arguments, but the logic is distict. They are both capturing information from different places and for different use cases. _have_job_arg is testing whether the command line has specified a number of jobs to use, while _get_make_arguments is implicitly looking up MAKEFLAGS from the environment variable for similar arguments in order to pass this through to the build. Really _have_job_arg is trying to ascertain whether we need to call _get_make_arguments.

I'm not sure how this could gracefully merged into _get_make_arguments becasue we don't actually need to do anything when _have_job_arg is true. The semantics of the name _get_make_arguments really don't fit this either. The name implies it's pulling various make arguments into the command line being constructed, but it actually only extracts the job arguments.

Overall I'm not a fan of the MAKEFLAGS extraction primarily because it's taking an environment variable associated with make and implicitly using it with other, non make programs. Presumbly, invoking make will already get the job arguments from MAKEFLAGS because that's what MAKEFLAGS is designed to do. Ninja doesn't look like it has an equivalent and this looks like a bit of a hack and explicitly supporting job arguments when invoking colcon makes it obsolute.

I'd argue that coclon is actually subverting MAKEFLAGS and would suggest that having this PR makes this MAKEFLAGS usage obsolete. Both functions could be deprecated and removed in future.

I had concidered renaming _get_make_arguments but decided to keep it siloed and avoid diving to deeply into the intended behaviour of _get_make_arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With respect to the regular expression what flags are you thinking it will miss?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With respect to the regular expression what flags are you thinking it will miss?

All of these are flags affect the job server:

colcon-cmake/colcon_cmake/task/cmake/build.py

Lines 294 to 302 in ecf417d

# 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|$)))'
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually avoiding handling -l becuause that's on its own doesn't use jobs.

However, your link does highlight a way I could resolve both issues above. Would you be satisfied with pulling out the job args parsing regular expressions into its own function for both _have_job_arg and _get_make_arguments to use? This way the parsing is unified, but each function can continue to parse from different places as is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, some of these are not valid for the command line and are only valid for make. It looks like ninja doesn't only uses the -j and -l abbreviations and does not support --jobs or --load-average. I can see the complexity of determining whether to parse MAKEFLAGS or not exploding.

# CMake 3.12+ and direct -j option, or pre 3.12 and after '--'
return True
if arg.startswith('--parallel') and cmake_has_jobs:
# CMake 3.12 and '--parallel' present.
return True
return False

async def _build(self, args, env, *, additional_targets=None):
self.progress('build')

Expand Down Expand Up @@ -240,10 +284,18 @@ async def _build(self, args, env, *, additional_targets=None):
cmd += ['--clean-first']
if multi_configuration_generator:
cmd += ['--config', self._get_configuration(args)]
else:
# Add any additional build arguments.
if args.cmake_build_args:
cmd += args.cmake_build_args
if (not multi_configuration_generator and
not self._have_jobs_arg(cmd, env)):
# Add jobs from MAKEFLAGS or max out.
job_args = self._get_make_arguments(env)
if job_args:
cmd += ['--'] + job_args
# add '--' pass through if not already present
if '--' not in cmd:
cmd += ['--']
cmd += job_args
completed = await run(
self.context, cmd, cwd=args.build_base, env=env)
if completed.returncode:
Expand Down Expand Up @@ -343,9 +395,17 @@ 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:
allow_job_args = False
if args.cmake_install_args:
cmd += args.cmake_install_args
if self._have_jobs_arg(cmd, env):
allow_job_args = False
if allow_job_args:
job_args = self._get_make_arguments(env)
if job_args:
cmd += ['--'] + job_args
# add '--' pass through if not already present
if '--' not in cmd:
cmd += ['--']
cmd += job_args
return await run(
self.context, cmd, cwd=args.build_base, env=env)