Args options for CMake --build and --install

[KazNX]

Add --cmake-build-args to support passing arguments through to cmake --build
Add --cmake-install-args to support passing arguments through to cmake --install

This PR supercedes #107 . See that PR for discussion history.

[hidmic]

This LGTM. @cottsay ?

[cottsay]

This LGTM. cottsay ?

On my list for this week.

[hidmic]

@cottsay ping.

[cottsay]

The addition of _have_jobs_arg duplicates much of the logic in _get_make_arguments. I'd really like to see this go one way or the other - either _have_jobs_arg gets folded into _get_make_arguments, or the conditional logic from _get_make_arguments gets moved to _have_jobs_arg.

As it stands, tracing the logic is not trivial.

[cottsay]

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

[KazNX]

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.

[KazNX]

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

[cottsay]

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

[KazNX]

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.

[KazNX]

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.

[hidmic]

@KazNX will you pick this up again in the near future? If not, I'll try and push this over the line. It'll benefit folks using colcon with ninja (like myself).

[KazNX]

@hidmic Been busy. I've posted some questions for @cottsay regarding how to go about implementing what he proposes.

[KazNX]

@ruffsl Follow up from IROS. I'm waiting on feedback on how to address the MAKEFLAGS questions. Personally, I'm not a fan of pulling from environment variables which are specific to a particular CMake generator - e.g., MAKEFLAGS is not relevant to MSBuild.

[sagiben-bw]

this is very useful addition, cmake --install supports also --strip argument for stripping debug symbols while installing binaries, which is not supported by colcon.
What needs to be done in order to merge this PR ? Thank you