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

Suggestion: build _install() should invoke cmake --install #55

Closed KazNX closed 4 years ago

KazNX commented 4 years ago

In build.py, _install(), the command invoked to run installation uses either cmake --build <dir> --target install or invoked MSBUILD to achieve a similar result. This works out OK for make platforms, but inccurs unnecessary overheads with Visual Studio as Visual Studio ensures all the dependencies of the INSTALL target are up to date.

Installation is normally invoked immediately after a build, so we essentially build twice. Windows slower file IO and the additional work Visual Studio does here makes for a significantly slower build process than with Linux/make.

I suggest using cmake --install <dir> or cmake --install <dir> --config <config> instead. This is significantly faster than the MSBUILD approach. I have one simple project where this is ~75 ms with cmake install vs. 1.6s with MSBUILD. The MSBUILD overhead scales for larger projects.

This would also unify the code executed by _install() no longer needing to do the generator check, just check where it needs the --config argument.

Note: this suggestion is also immune to bug https://github.com/colcon/colcon-cmake/issues/54

KazNX commented 4 years ago

Correction: cmake --install was introduced in CMake 3.15. Using cmake --build <dir> --target install is equivalent to the MSBUILD solution.

Given the signification performance difference, I still suggest using --install with CMake 3.15+.

Out of interest, why does the Visual Studio generator code path not use cmake --build <dir> --target install --config <cfg>?

dirk-thomas commented 4 years ago

That sounds like a good enhancement if that CMake version is available. Would you be interested in contributing a pull request for this?

why does the Visual Studio generator code path not use cmake --build <dir> --target install --config <cfg>?

I would guess that we are using msbuild and the INSTALL project file since when this was implemented first there was no cmake --build. It could probably be unified with the non-Windows logic now. Would you like to contribute a separate pull request for that?

KazNX commented 4 years ago

First pull request: https://github.com/colcon/colcon-cmake/pull/56

This removes the special case code for installing with Visual Studio. That is always use cmake --build <dir> --target install and not MSBUILD.

KazNX commented 4 years ago

Second pull request: https://github.com/colcon/colcon-cmake/pull/57

This checks for CMake 3.15+ the uses --install <dir> instead of --build <dir> --target install.

KazNX commented 4 years ago

Note: the second pull request is a superset of the first.

dirk-thomas commented 4 years ago

Addressed by #57.