Update CMakeLists, prepare for packaging #31
Conversation
nickelpro
requested review from
JeffGarland,
robert-andrzejuk and
wusatosi
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
* Converts beman::scope to interface library * Adds CMake config packaging routines * Removes vestigal non-header source file * Tests/examples link to library instead of raw source include
| 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| 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 }." |
There was a problem hiding this comment.
While we're here we should probably flip this to the recommended practice -- aka off
edit: I completely misread the rule there -- was also thrown off because as I recall optional sets testing off by default.
| @@ -0,0 +1,8 @@ | |||
| include(${CMAKE_CURRENT_LIST_DIR}/beman.scope-targets.cmake) | |||
There was a problem hiding this comment.
do we need to open an issue against exemplar repo since an example of this doesn't appear?
| endif() | ||
|
|
||
| if(BEMAN_SCOPE_BUILD_TESTS) | ||
| enable_testing() |
There was a problem hiding this comment.
another case where exemplar perhaps should be updated -- in that repo ctest is unconditionally included in the top level project
There was a problem hiding this comment.
This has been updated multiple time, the consensus formed is to just use include(CTest).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't think bemanproject/exemplar#153 or bemanproject/exemplar#109 mentioned anything about dashboard.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Didn't realize this was a beehive already thrice stepped on
There was a problem hiding this comment.
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....]
There was a problem hiding this comment.
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.
This is where we need 'examplar as a template' and a tool to generate a starting version of the repo for a new project. @wusatosi weren't you going to work on this? In any case my guess is that header-only will be as or more common in beman than traditional linked libraries. |
JeffGarland
left a comment
JeffGarland
left a comment
There was a problem hiding this comment.
I'm approving this as I have no actually requested changes here -- more questions about if we should open issues against exemplar instead. @nickelpro thanks so much for doing this as my personal expertise doesn't included the finer points of modern cmake (note that I consider this a good thing largely -- just bc I use it everyday doesn't mean I want to know every gory detail)
I hold the same view, but this is currently rejected in exemplar (check: bemanproject/exemplar#113 ), I have brought this up at sync before but the conversation went nowhere. CC @neatudarius @bretbrownjr who rejected the PR.
If you're asking if we can have a template option that creates an repo from exemplar that is header only, I have been told at CppNow that header-only repo have enough difference in CMake to the point that we should have a header-only exemplar. Current HEAD of exemplar is too rapidly updating to permit true template generation, it is currently limited as "a github template". We will need to maintain both "what it will look like in full" and "what this looks like in template land", and sync from one to the other if we want to go full template. Maybe something I will look into once we can start moving a lot of the CMake work into infra. Note that this tool only handles repo creation, it does not handle subsequent update, which seem to be what this PR is about. |
|
|
||
| project(beman.scope DESCRIPTION "Generic Scope Guard" LANGUAGES CXX) | ||
|
|
||
| enable_testing() |
There was a problem hiding this comment.
I think the recommendation is to use include(CTest) instead of enable_testing().
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
This seems to go against: bemanproject/exemplar#153
Seems like include(CTest) is preferred over there. We should either adopt or leave a paper trail.
Sure -- so the template version of exemplar can have files like CMakeLists--header_only--.txt and CMakeLists--build_library--.txt and the template script would fill in one, rename to CMakeLists.txt and stage the template files for deletion. There's probably other ways to do it depending on the templating languaue chosen ( presumably something from python ) where the header-only verus build-library command line option on the script add or remove appropriately from a single cmakelist.txt |
|
So to reply to the why is exemplar not header only question. I asked bret, basically he says: |
|
I don't know that I have time to provide a pithy defense of my thinking on header-only support, but it's worth noting:
https://github.com/bemanproject/execution/blob/main/src/beman/execution/CMakeLists.txt#L19 |
|
"Fixing" a repo to be appropriately header-only or not is a one-time cost per repo and completely trivial If Beman were producing 3 or 4 new libraries a week, I might consider this a real point of discussion. As it is, I don't think it's a discussion with any stakes besides philosophical ideas about what Exemplar is supposed to represent. It certainly has no bearing on |
Well actually, in c++26 almost every part of the standard library is constexpr -- and yep most of it is templates as well. https://wg21.link/P3372R3 // already voted in To be clear, I'm not here to argue that we should change exemplar (right now) bc I agree that building an actual library is the more complicated case and we need to represent. However, I feel like making a template that does both is a failure of imagination -- I feel like even AI could do most of the work on this... |
Maybe 3 per month would be enough -- remember that the barrier to entry impacts this rate.
Agree there's no bearing on this PR. But I'd like to open issues on exemplar to move it forward so we can improve things.... |
I think it's safe to merge this PR if it's ready, we can always come back later. |
|
I was going to let it stew for another day but sure |
Yeah this discourages discussion in this thread so we can centralize the discussion in exemplar. |
4 participants
This isn't a lot of code and normally I would let it speak for itself, but CMake is a tricky DSL so I'm going to provide a commentary on all the changes here to make reviewing this easier for non-experts, and so I can reference the discussion here as I work through other Beman repos.
project(VERSION)The added
VERSIONis necessary for even pre-1.0 packaging, as consumers will want to be able to differentiate between pre/post 1.0, and "none" isn't an option.add_library(beman.scope INTERFACE)beman.scope, like most Beman libraries, is header-only by its very nature. In CMake speak that makes it an "interface" library, nothing needs to be compiled to consumebeman.scope. It only describes a set of include flags that need to be passed to the compiler. Usingadd_library(INTERFACE)formalizes that.We diverge from
beman.exemplarbecausebeman.exemplaris built to accommodate libraries that do need to produce archives or shared objects. We do not need to take advantage of that accommodation forbeman.scope.Removal of
src/beman/scopeThe
srcfolder is fully vestigial now that we've convertedbeman.scopeto an interface library, so we remove it and move the remaining contents into the top-level CMakeLists.txt.Why the top-level CMakeLists.txt?
It might make sense to create a new CMakeLists.txt in
include/beman/scopeand put our code there. However, the total length of the top-level CML is less than 100 lines. When there is so little code to begin with, further nesting inhibits organization and clarity instead of improving it.beman.scope-config.cmakeThe weird boilerplate loop in this file is to avoid a very specific "bug" in consuming
beman.scope, specifically: What should happen if someone callsfind_package(beman.scope COMPONENTS PerlInterpreter)?beman.scopedoes not provide anyfind_package(COMPONENTS), and certainly not a Perl interpreter. Our two options here are to ignorefind_package(COMPONENTS)or fail if any are required.beman.exemplarchooses to fail in this situation, so we follow that example.This strange piece of boilerplate is unfortunately the most compact and obvious way to achieve that behavior, and mirrors closely how CMake documents this loop should look. The other option is to call a code generator to write these 6 lines of code for us, a level of indirection that offends me on a personal level.
Other Odds and Ends
include(CTest)is redundant withenable_testing()in this usagebeman.scopewill use it viatarget_link_libraries(target PRIVATE beman::scope), so we should tooFILE_SET HEADERS DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}is redundant,FILE_SET HEADERSimpliesDESTINATION ${CMAKE_INSTALL_INCLUDEDIR}$<$<CONFIG:Debug>:debug/>generator expressions are meaningless for header-only libraries, headers don't have debug builds