Install sets #73
Install sets #73
Conversation
Create named file sets and use those for installation. Exclude tests from installation, and the empty static library. Use a project prefixed name to enable or disable tests, and default to building tests if this is the top level project.
Further CML lint
| ${PROJECT_IS_TOP_LEVEL} | ||
| ) | ||
|
|
||
| # Build the tests if enabled via the option OPTIONAL26_ENABLE_TESTING |
There was a problem hiding this comment.
Can you remind me why BUILD_TESTING is not enough/OK ?
The Beman Standard specify to have BUILD_TESTING.
There was a problem hiding this comment.
It doesn't compose. Best practice for Cmake is to prefix options with the project they apply to, and to default the value to true for a top level project.
Patten documented in Professional CMake.
There was a problem hiding this comment.
Agree that it's the best practice from CMake docs, but I would expect to first make a PR in the Beman Standard.
There was a problem hiding this comment.
Fortunately, it's a recommendation, but I'll PR that, now that I've seen it. But since this actually breaks trying to compose this into another beman project, it would be nice to fix it now.
|
It's probably a better requirement than producing the Cmake file for
FindPackage or ExternalProject use. I don't think we have any evidence that
we're doing that correctly?
…On Sat, Nov 9, 2024, 15:28 Darius Neațu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In CMakeLists.txt
<#73 (comment)>
:
> @@ -12,51 +12,49 @@ include(FetchContent)
set(TARGETS_EXPORT_NAME ${CMAKE_PROJECT_NAME}Targets)
-# Build the tests only if enabled via the CLI flag: BUILD_TESTING.
-if(BUILD_TESTING)
+option(
+ OPTIONAL26_ENABLE_TESTING
+ "Enable building tests and test infrastructure"
+ ${PROJECT_IS_TOP_LEVEL}
+)
+
+# Build the tests if enabled via the option OPTIONAL26_ENABLE_TESTING
@JeffGarland <https://github.com/JeffGarland> should we make it a
requirement?
—
Reply to this email directly, view it on GitHub
<#73 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVNZ5UE2G4YLU5UQMK6TDTZ7ZWAHAVCNFSM6AAAAABRPH2FA2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRVGU3TONZSGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
3 participants
This changes the install and include directory strategy to use FILE_SETs with type HEADERS.
Moves the creation of the targets and named header file sets upward so they exist in order to add the files locally in the src and include directories. Change the installs to use components so we can install just the necessary headers and not the empty library.
Also make the option to build headers named for the project so it doesn't affect other included projects.
Exclude google from all so it doesn't get installed for the "all" component.
Remove the cmake export install, at least for now. It was not working correctly. It's probably not strictly necessary for vcpkg or conan integration.