Skip to content

Update CMakeLists, prepare for packaging #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nickelpro merged 1 commit into from
May 28, 2025
Merged

Update CMakeLists, prepare for packaging #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,6 @@
# ignore emacs temp files
*~
\#*\#

# ignore vscode settings
.vscode
72 changes: 65 additions & 7 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,87 @@

cmake_minimum_required(VERSION 3.25)

project(beman.scope DESCRIPTION "Generic Scope Guard" LANGUAGES CXX)

enable_testing()
Copy link
Member

@wusatosi wusatosi May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the recommendation is to use include(CTest) instead of enable_testing().

See: bemanproject/exemplar#153

Copy link
Member

@JeffGarland JeffGarland May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see conversation above

Copy link
Member Author

@nickelpro nickelpro May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fully opposed to reverting this back to include(CTest), but we should formalize something here.

A default, always-on test target is of questionable utility and, in my experience, is more misleading than helpful. Users running the test target without realizing that tests are not enabled are not met with loud failure, which would be the expectation. Instead they must consult the logs to recognize anything has gone wrong at all. To be clear: running the test target with no tests is not a failure. The tests "pass".

include(CTest) also drags in many other targets that are totally useless for Beman and make interacting with the generated build system a bear, ie:

$ cmake --build . --target help
[1/1] All primary targets available:
Experimental: phony
Nightly: phony
Continuous: phony
NightlyMemoryCheck: phony
NightlyStart: phony
NightlyUpdate: phony
NightlyConfigure: phony
NightlyBuild: phony
NightlyTest: phony
NightlyCoverage: phony
NightlyMemCheck: phony
NightlySubmit: phony
ExperimentalStart: phony
ExperimentalUpdate: phony
ExperimentalConfigure: phony
ExperimentalBuild: phony
ExperimentalTest: phony
And dozens more...

This is undesirable unless you're getting a lot out of the rest of the CTest module. Beman gets nothing out of the CTest module besides the default test target discussed above.

Copy link
Member Author

@nickelpro nickelpro May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move this discussion to an exemplar issue, I'll let this settle for a day or two and if I can't convince the peanut gallery to side with me unwind the testing change.

Copy link
Member

@bretbrownjr bretbrownjr May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A default, always-on test target is of questionable utility and, in my experience, is more misleading than helpful. Users running the test target without realizing that tests are not enabled are not met with loud failure, which would be the expectation. Instead they must consult the logs to recognize anything has gone wrong at all. To be clear: running the test target with no tests is not a failure. The tests "pass".

I disagree that it's of questionable utility. It's very helpful for the higher level workflow orchestrator (me as a contributor, a common conan workflow, or a shared github workflow) to know exactly how and when to run tests, especially from a plain vanilla checkout of the code in a plain vanilla environment. Having an always-available test target that, yes, sometimes runs zero tests is a positive validation that all available tests are passing.

We could talk about alternative ways to let the higher-level workflows tool know that there are tests available and in what circumstances, but having an always-on test target that can be augmented or pared down by various MYREPO_TEST_THIS=ON and MYREPO_TEST_THAT=OFF options is a good combination of intuitive and flexible in my experience.

I agree that the extra targets are annoying, especially in IDEs that try to expose these via dropdowns, though I'd consider that more annoying than a problem as such. I'm also OK with enable_testing(), but people feel strongly about being able to inspect and respect BUILD_TESTING, so include(CTest) being on always seems to be a compromise in the truest form -- nobody's entirely happy but everyone can live with it. If anyone knows any core CMake engineers that can come up with something that everyone can love whole-heartedly, please let them know!

project(
beman.scope
DESCRIPTION "Generic Scope Guard"
LANGUAGES CXX
VERSION 0.0.1
)

# [CMAKE.SKIP_TESTS]
option(
BEMAN_SCOPE_BUILD_TESTS
"Enable building tests and test infrastructure. Default: ON. Values: { ON, OFF }."
"Enable building tests and test infrastructure. Default: ${PROJECT_IS_TOP_LEVEL}. Values: { ON, OFF }."
Copy link
Member

@JeffGarland JeffGarland May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here we should probably flip this to the recommended practice -- aka off

https://github.com/bemanproject/beman/blob/eb49fca3b1c7330f5a5850cbce68f51d5dd3d2c9/docs/BEMAN_STANDARD.md?plain=1#L291

edit: I completely misread the rule there -- was also thrown off because as I recall optional sets testing off by default.

${PROJECT_IS_TOP_LEVEL}
)

# [CMAKE.SKIP_EXAMPLES]
option(
BEMAN_SCOPE_BUILD_EXAMPLES
"Enable building examples. Default: ON. Values: { ON, OFF }."
"Enable building examples. Default: ${PROJECT_IS_TOP_LEVEL}. Values: { ON, OFF }."
${PROJECT_IS_TOP_LEVEL}
)

option(
BEMAN_SCOPE_INSTALL_CONFIG_FILE_PACKAGE
"Enable creating and installing a CMake config-file package. Default: ${PROJECT_IS_TOP_LEVEL}. Values: { ON, OFF }."
${PROJECT_IS_TOP_LEVEL}
)

include(GNUInstallDirs)
add_library(beman.scope INTERFACE)
add_library(beman::scope ALIAS beman.scope)

# gersemi: off
set_target_properties(
beman.scope
PROPERTIES
VERIFY_INTERFACE_HEADER_SETS ON
EXPORT_NAME scope
)

target_sources(
beman.scope
INTERFACE
FILE_SET HEADERS
BASE_DIRS include
FILES include/beman/scope/scope.hpp
)

install(
TARGETS beman.scope
EXPORT beman.scope-targets
COMPONENT beman.scope
FILE_SET HEADERS
)
# gersemi: on

if(BEMAN_SCOPE_INSTALL_CONFIG_FILE_PACKAGE)
include(GNUInstallDirs)
include(CMakePackageConfigHelpers)

write_basic_package_version_file(
${CMAKE_CURRENT_BINARY_DIR}/beman.scope-config-version.cmake
COMPATIBILITY ExactVersion
)

# todo rm add_subdirectory(src/beman/scope)
install(
FILES
cmake/beman.scope-config.cmake
${CMAKE_CURRENT_BINARY_DIR}/beman.scope-config-version.cmake
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/beman.scope
COMPONENT beman.scope
)

install(
EXPORT beman.scope-targets
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/beman.scope
NAMESPACE beman::
COMPONENT beman.scope
)
endif()

if(BEMAN_SCOPE_BUILD_TESTS)
enable_testing()
Copy link
Member

@JeffGarland JeffGarland May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another case where exemplar perhaps should be updated -- in that repo ctest is unconditionally included in the top level project

Copy link
Member

@wusatosi wusatosi May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been updated multiple time, the consensus formed is to just use include(CTest).

See: bemanproject/exemplar#153

Copy link
Member

@JeffGarland JeffGarland May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe @nickelpro is correct and any consensus that has been formed around this is incorrect. The question comes down to are we using 'the dashboard' -- we most certainly are not currently.

https://discourse.cmake.org/t/is-there-any-reason-to-prefer-include-ctest-or-enable-testing-over-the-other/1905

Copy link
Member

@wusatosi wusatosi May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think bemanproject/exemplar#153 or bemanproject/exemplar#109 mentioned anything about dashboard.

Copy link
Member

@wusatosi wusatosi May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more about consistently define the test target.

Please check the back-and-forth reasoning behind these PRs:
bemanproject/exemplar#153
bemanproject/exemplar#109
bemanproject/exemplar#91

Copy link
Member Author

@nickelpro nickelpro May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't realize this was a beehive already thrice stepped on

Copy link
Member

@wusatosi wusatosi May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a couple lines in exemplar's CMake that has undergone a lot of discussion and back-and-forth edits.
It's not like we can put a #WARNING: BEFORE YOU UPDATE, CHECK [PR numbers....]

Copy link
Member

@bretbrownjr bretbrownjr May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the expectation is that we add something to the beman standard explaining when we have a recommendation or a requirement. I won't have time in the next few weeks to follow up on that, so if someone else wants to take a pass, please do. Otherwise, hopefully it can wait my other activities (like finalizing the CppCon program) can wrap up.

add_subdirectory(tests)
endif()

Expand Down
8 changes: 8 additions & 0 deletions cmake/beman.scope-config.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
include(${CMAKE_CURRENT_LIST_DIR}/beman.scope-targets.cmake)
Copy link
Member

@JeffGarland JeffGarland May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to open an issue against exemplar repo since an example of this doesn't appear?


foreach(comp IN LISTS beman.scope_FIND_COMPONENTS)
if(beman.scope_FIND_REQUIRED_${comp})
set(beman.scope_FOUND FALSE)
return()
endif()
endforeach()
2 changes: 1 addition & 1 deletion examples/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ message("Examples to be built: ${ALL_EXAMPLES}")
foreach(example ${ALL_EXAMPLES})
add_executable(${example})
target_sources(${example} PRIVATE ${example}.cpp)
target_include_directories(${example} PRIVATE ${CMAKE_SOURCE_DIR}/include)
target_link_libraries(${example} PRIVATE beman::scope)
Copy link
Member

@JeffGarland JeffGarland May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting and something I didn't realize. So I guess modern cmake practice is to add the 'link library', even if there isn't one and let cmake infer the includes. So really the macro has become target_depenencies

Copy link
Member

@bretbrownjr bretbrownjr May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. target_requires or something would be a better name at this point. Possibly we should recommend an alternative spelling in upstream CMake, even, to really drive the point home.

To address something you commented on here, beman::scope will always exist, at least in the context of this repo structure. It's expected that you, as someone reading this CMakeLists.txt, implicitly know and guarantee that. I'm not entirely thrilled with that developer experience (see old jokes about assumptions), so I'm also working with upstream CMake to advocate for a more declaratove require_targets(beman::scope) helper to be used to clarify that the target needs to exist in a more abstract but semantic way. Along with other upsides. No ETA on that yet unfortunately.

But for now, you just have to know that find_package(SomeName REQUIRED) is used when you need something outside your project and not when the dependency is in the same source tree.

endforeach()
25 changes: 0 additions & 25 deletions src/beman/scope/CMakeLists.txt
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions src/beman/scope/scope.cpp
View file
Open in desktop

This file was deleted.

6 changes: 2 additions & 4 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,14 @@ set(ALL_TESTNAMES scope_success scope_exit scope_fail unique_resource)

message("Tests to be built: ${ALL_TESTNAMES}")

include(CTest)
include(Catch)

foreach(testname ${ALL_TESTNAMES})
add_executable(test.${testname})
target_sources(test.${testname} PRIVATE ${testname}.test.cpp)
target_include_directories(
target_link_libraries(
test.${testname}
PRIVATE ${CMAKE_SOURCE_DIR}/include
PRIVATE Catch2::Catch2WithMain beman::scope
)
target_link_libraries(test.${testname} PRIVATE Catch2::Catch2WithMain)
catch_discover_tests(test.${testname})
endforeach()