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

Use 'cmake --install' where available #57

Closed KazNX closed 4 years ago

KazNX commented 4 years ago

Make use of CMake '--install' option introduced in 3.15. Only performs installation, whereas '--build

--target install' would first (re)build. Using '--install' is a significantly faster installation process when using Visual Studio as various overheads are avoided.

dirk-thomas commented 4 years ago

Builds on top of #56. Waiting for that one to be merged first.

dirk-thomas commented 4 years ago

With #56 merged can you please rebase this patch.

KazNX commented 4 years ago

I've resolve the merge conflicts, however, I merged rather than rebasing.

dirk-thomas commented 4 years ago

Thanks for iterating on this. I have one more comment though: atm each package will determine the CMake version separately. It would be good if the function persists the determined version in a global variable and return that directly when being involved repeatedly.

dirk-thomas commented 4 years ago

@KazNX Are you planning on addressing the last comment (determining the CMake version only once)?

KazNX commented 4 years ago

@dirk-thomas Sorry, I've been busy on deliverables. Can you propose the best way to determine the CMake globally?

dirk-thomas commented 4 years ago

Can you propose the best way to determine the CMake globally?

@KazNX Please see my previous comment for that:

It would be good if the function persists the determined version in a global variable and return that directly when being involved repeatedly.

See https://github.com/colcon/colcon-ros/blob/ba7472246ec4235fcae0ce71b055aff07bfec4c4/colcon_ros/task/cmake/__init__.py#L33-L59 for an example.

dirk-thomas commented 4 years ago

@KazNX Friendly ping.

KazNX commented 4 years ago

I have a caching solution in place now.

KazNX commented 4 years ago

All issues should be addressed now.

dirk-thomas commented 4 years ago

Also trying this patch with a CMake version new enough to trigger --install the invocation fails for me on Linux since it passes the job_args which seems to be not supported for the install option.

dirk-thomas commented 4 years ago

@KazNX Friendly ping.

KazNX commented 4 years ago

@dirk-thomas I've been waiting for responses on my comments. I'll have a look at the linux issue and streamlining the regex. Is there a simple example I can follow for the unit test?

dirk-thomas commented 4 years ago

I've been waiting for responses on my comments.

Can you please clarify what comments you are referring to?

As far as I can see I was the last person commenting on all pending conversation threads:

Is there a simple example I can follow for the unit test?

I am not sure what you are referring to? Regarding the failing build you should just try to build any CMake package using your patch.

KazNX commented 4 years ago

I've been waiting for responses on my comments.

Can you please clarify what comments you are referring to?

Looks like this is an unfamiliarity with GitHub on my part. I need to hit submit.

Is there a simple example I can follow for the unit test?

I am not sure what you are referring to? Regarding the failing build you should just try to build any CMake package using your patch.

I was after an example to follow as I'm unsure how you are invoking unit tests. I added test/test_parse_cmake_version.py and it is failing the build because it can't import colcon_cmake. I'm not familiar with the cause.

dirk-thomas commented 4 years ago

^(?:.*) is questionable now. It's a non-capturing group. Better to replace with .*.

.*(\d+\.\d+) isn't correct since for an input string like foo 12.3 the captured group will be only 2.3 because the leading wildcard is greedy. How about checking for the whitespace before the digit?

(\d+\.\d+\..*) matches (#.#.<anything>) to match major.minor.patch+comment

The trailing .* matches anything. Could there be a "+comment" which you can't parse as a version? Why not limit the capture group to end after the digits?

I'm unsure how you are invoking unit tests.

Just invoking pytest - the coverage flags are optional, see https://github.com/colcon/colcon-cmake/blob/c3e809cd57e38d10c15fa80df7fe0876252956a1/.travis.yml#L14

I added test/test_parse_cmake_version.py and it is failing the build because it can't import colcon_cmake. I'm not familiar with the cause.

Locally the linters fail with the following:

test/test_copyright_license.py:10
In some files no copyright / license line was found

as well as 

1     W292 no newline at end of file
1     E302 expected 2 blank lines, found 1
1     E305 expected 2 blank lines after class or function definition, found 1
1     E501 line too long (80 > 79 characters)
2     F401 'pkg_resources.parse_version' imported but unused
1     Q000 Remove bad quotes

Regarding the comments: lets first focus on getting the functional logic of the patch right and working as well as the tests turn over. Then we can revisit the comments and other nitpicks.

KazNX commented 4 years ago

I've updated the regular expression to be less greedy with leading characters. I'm also ignoring any non-numeric suffix. It turns out CMake can have many suffixes including '-dirty' on working versions. I don't see it being worthwhile to distinguish these. I've tried a number of expected and some unexpected strings in test_parse_cmake_version.py including checking the greedy case you've highlighted.

I've fixed the linter issues. I didn't notice these because it's the following error which has my attention:

ImportError while importing test module '/home/travis/build/colcon/colcon-cmake/test/test_parse_cmake_version.py'. Hint: make sure your test modules/packages have valid Python names. Traceback: test/test_parse_cmake_version.py:4: in from colcon_cmake.task.cmake import parse_cmake_version_string E ImportError: No module named 'colcon_cmake'

Clearly it's failing to import the module I'm trying to test. This is where I'm unsure of why though. I can run the test from the command line. By contrast, colcon-core tests do something similar, importing modules from within colcon-core.

dirk-thomas commented 4 years ago

Clearly it's failing to import the module I'm trying to test. This is where I'm unsure of why though. I can run the test from the command line. By contrast, colcon-core tests do something similar, importing modules from within colcon-core.

Please try to create an empty file test/__init__.py.

KazNX commented 4 years ago

That seems to have done the trick. Thanks. That should address all your oustanding issues.

dirk-thomas commented 4 years ago

Some ROS 2 CI builds to double check:

KazNX commented 4 years ago

Some ROS 2 CI builds to double check:

They all look good, but are they running with this change or does something need to be done in order to test this PR with those builds?

dirk-thomas commented 4 years ago

are they running with this change

Yes, I have pushed the changes from this PR to a branch within this repo (https://github.com/colcon/colcon-cmake/tree/enhancement/use-cmake-install) and the jobs are using that branch (see CI parameter colcon_branch).

This is good to be merged. Thank you for this nice improvement. I am sorry for the multiple rounds of iteration and appreciate that you kept iterating on it. :bowing_man: