mp-units icon indicating copy to clipboard operation
mp-units copied to clipboard

Update CMake from vcpkg patch

Open bansan85 opened this issue 1 year ago • 7 comments

This patch will simply make build of tests, example and documentation optional. And CONFIG option is missing in find_dependency in Config.cmake

bansan85 avatar Jul 12 '22 08:07 bansan85

gitpod-io[bot] avatar Jul 12 '22 08:07 gitpod-io[bot]

Prefixing options with MP_UNITS_ should help avoid clashes with other projects' variables.

JohelEGP avatar Jul 12 '22 14:07 JohelEGP

I understand the idea. But I don't think it's a good idea.

For example, when you include(CTest), it automatically set BUILD_TESTING to 'ON'. You can't set the name.

If you want to avoid clashes, you should pass EXCLUDE_FROM_ALL to add_subdirectory(units). So you will build only the needed target.

bansan85 avatar Jul 12 '22 14:07 bansan85

Sounds good. I also forgot about that consumers should use ./src, so it's all OK as is.

JohelEGP avatar Jul 12 '22 15:07 JohelEGP

For example, when you include(CTest), it automatically set BUILD_TESTING to 'ON'. You can't set the name.

So it means that if someone will include(CTest) in the customer project before adding dependencies then all the dependencies will be built with tests and influence the compile times or possibly even make the compilation of the customer's project fail. I do not think it is a good idea. So many times I saw third-party unit tests in CTest that I never wanted to compile and run.

In general, I do not think it is a good idea. It explodes the number of CMake configurations to support and test. Why vcpkg cannot just use ./src/CMakeLists.txt for a build?

mpusz avatar Jul 19 '22 15:07 mpusz

The choice about prefixing or not CMake global variables is complicated. I didn't find an official response about it.

If you don't prefix and you use add_subdirectory from another project, you will set variable from this subproject that you may don't want. The solution is to use the feature EXCLUDE_FROM_ALL.

If you prefix, then you break some features of CMake.

As units library looks to decide to prefix CMake variables, I will modify my PR according to this.

bansan85 avatar Jul 23 '22 05:07 bansan85

Is it possible for vcpkg to just use src/CMakeLists.txt directly? In such a case, no changes are needed, and the customer just gets the library code.

mpusz avatar Jul 23 '22 16:07 mpusz

It seems that this PR is no longer needed as a patch to user ./src was applied to vcpckg.

mpusz avatar Sep 18 '23 08:09 mpusz