CMake Updates, main branch (2024.11.10.) #41
Conversation
There was a problem hiding this comment.
I'm not generally opposed to this, but the changes are far too extensive; let's not update the examples, tests, or benchmarks; they are not installed anyway so there are no name collisions with them. For the library targets, can't we just install them into a namespace to solve the problem?
| add_library(covfie_benchmark test_field.cpp) | ||
|
|
||
| target_include_directories(benchmark PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) | ||
| target_include_directories(covfie_benchmark PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) | ||
|
|
||
| target_link_libraries(benchmark PUBLIC core) | ||
| target_link_libraries(covfie_benchmark PUBLIC covfie::core) | ||
|
|
||
| target_compile_definitions(benchmark PRIVATE _CRT_SECURE_NO_WARNINGS) | ||
| target_compile_definitions(covfie_benchmark PRIVATE _CRT_SECURE_NO_WARNINGS) |
There was a problem hiding this comment.
I'm not on board with the benchmarks, tests, and examples being renamed; these should never be installed anyway, so there is no way for any name collisions to occur.
| # | ||
| # This Source Code Form is subject to the terms of the Mozilla Public License, | ||
| # v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
| # obtain one at http://mozilla.org/MPL/2.0/. | ||
|
|
||
| # Create the benchmark executable from the individual files. | ||
| add_executable(benchmark_cpu benchmark_cpu.cpp) | ||
| add_executable(covfie_benchmark_cpu benchmark_cpu.cpp) |
There was a problem hiding this comment.
Let's keep the original name on this.
| @@ -12,15 +12,15 @@ enable_language(CUDA) | |||
| include(covfie-compiler-options-cuda) | |||
|
|
|||
| # Create the benchmark executable from the individual files. | |||
| add_executable(benchmark_cuda benchmark_cuda.cu) | |||
| add_executable(covfie_benchmark_cuda benchmark_cuda.cu) | |||
| set_target_properties( | ||
| covfie_core | ||
| PROPERTIES | ||
| EXPORT_NAME | ||
| core | ||
| ) | ||
|
|
||
| install(TARGETS covfie_core EXPORT ${PROJECT_NAME}Targets) |
There was a problem hiding this comment.
Do we really need to make this so complicated? Can't we just set install(... NAMESPACE something_)?
There was a problem hiding this comment.
I didn't touch the code that specifies the NAMESPACE. The EXPORT_NAME needs to be specified, because without it, the exported library would now be called covfie::covfie_core.
This is exactly how the "trick" of using vecmem::core, detray::core, etc. is achieved, whether or not we're using the project directly, or we're finding an installed version of it. (With the "actual target names" being vecmem_core, detray_core, etc.)
There was a problem hiding this comment.
Right, but this is what I am saying, just like Google Benchmark does it, the target should be named e.g. core and then the namespace should be added to it, so that the full name becomes covfie::core.
|
The problem is not with installed versions of Covfie. The problem is when you "compile" Covfie together with a client project. Whether they are installed or not, unless the user makes sure not to build any of Covfie's tests or benchmarks, those target names will now have to be reserved for Covfie. So better no client use I really insist that we give covfie specific names to all these targets. |
|
Also, note that you are not able to currently build GoogleBenchmark together with / as part of covfie, because of: https://github.com/google/benchmark/blob/main/src/CMakeLists.txt#L20 Google also decided to use the target name |
Indeed, as Google Benchmark also makes handy use of the namespacing feature, see https://github.com/google/benchmark/blob/main/src/CMakeLists.txt#L114 |
As a compromise, we can make it a configuration error for |
Why would that be a good setup? Is it evil from clients to want to build the covfie tests / benchmarks as part of their own project? |
But you understand the issue, right? 😕 You can't have two subdirectories in a single project declaring libraries with the same names. The aliases don't matter in this case. If CMake sees two targets with the same name ( |
I really don't see a legitimate use case for this. 🤔 |
With Covfie being a header-only library, clients wanting to test their own build setup through the built-in tests and benchmarks of the project may not be as crazy as you might think. 🤔 |
The C++20 standard is always required in the code since a while, the flag was not doing anything.
This is to ensure that common names like "core" and "cpu" would not clash with target names used in client projects.
krasznaa
force-pushed
the
CMakeUpdates-main-20241110
branch
from
d387f02 to
b40a9d3
Compare
Following the discussion in acts-project#41, this commit makes it a configuration error to request the builds of tests, benchmarks, or examples if covfie is not the top level project. Also removes vestigial Google Test code.
FYI I thought about it a bit more and became increasingly convinced that this is the way to go, so I implemented it this way and then renamed the library targets. I also took your |
While helping @CarloVarni in setting up the build of traccc on his Mac laptop, we bumped into a warning coming from covfie. While trying to silence that warning, I found that I really didn't like the target names used by this project. 🤔
The main change of this PR is to add a
covfie_prefix to the name of every library and executable of the project. So that it wouldn't claim basic names likecore,cpu,benchmark, etc. for itself. (The last one,benchmark, is I guess the reason why the project is able to build GoogleTest by itself, but not GoogleBenchmark... Since the latter uses the target namebenchmarkitself for its own library target...)I have some other ideas/plans as well, but these updates should not be too controversial. (The others may be...)
The removal of
COVFIE_REQUIRE_CXX20is just an added bonus. 😉