Skip to content
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.

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

Improve CMake integration #14

Open
friendlyanon opened this issue Jul 1, 2021 · 6 comments
Open

Improve CMake integration #14

friendlyanon opened this issue Jul 1, 2021 · 6 comments
Assignees
Labels
build Related (bug or feature) to the build system enhancement New feature or request

Comments

@friendlyanon
Copy link

At the moment, the CMake build files are messy.

  • There is no clear separation between consumer and developer code paths.
    This makes it impossible to build/install/package the library without heavily patching build files, which is completely avoidable. It's not nice to force consumers to pass 10 flags just so they can dodge everything but the library itself.
  • The build files make network requests during configure.
    If you wish this library to be reach a wide audience, this is an absolute no go. For dependencies like Catch2, you can use vcpkg, even for those you grab from GitHub, like the other ztd libraries.
  • The library does not declare its standard requirement.
    I had to find out from Library Concepts? #13 that this library is targetting C++17. A simple target_compile_features(ztd_text INTERFACE cxx_std_17) would be helpful. Hardcoding CMAKE_CXX_STANDARD is also wrong, because it's purpose is to be used from the CLI to control the build.
  • Globbing sources.
    This is mainly for scripting. Again, if you wish this library to be usable by a wide audience, this is an absolute no go. Mainly because it doesn't even work reliably for most build systems. Please read the big note from the CMake developers and heed their words.
  • Install rules are wrong.

Consumer side of lists files should do nothing more than describe the build and usage requirements of a library and the CMake build scripts should be as minimal as possible with no hardcoded details. It is noone's benefit to force people trying to build/package/use your library to patch things.

I recommend taking a look at cmake-init, which is designed to solve all of these issues. Since you have dependencies from GitHub as well, I recommend taking a look at the vcpkg example repository.
A better solution would be however, if instead of introducing potentially exponential complexity to building the library with optional dependencies, just make a separate library that depends on both this one and that optional dependency.

@ThePhD ThePhD self-assigned this Jul 1, 2021
@ThePhD
Copy link
Contributor

ThePhD commented Jul 1, 2021

Some of this is lost on me, so you'll have to walk me through it a little bit.

There is no clear separation between consumer and developer code paths.

Do you mean things like ZTD_TEXT_SCRATCH? Yeah that's for my ease of use when pulling from anywhere: I can black-hole that variable or mark it as "advanced" or whatever's necessary.

The build files make network requests during configure.

Yes. You can override this by getting a package manager to get the sources yourself, and then overriding the FetchContent source directory so that no download/update step is done, which is the usual CMake Practice now (FETCHCONTENT_SOURCE_DIR_<ucName>, https://cmake.org/cmake/help/latest/module/FetchContent.html). If/when I get around to making vcpkg or Build2 projects for this, I'd just be telling those builds to provide the source using the FETCHCONTENT_SOURCE_DIR_<ucName> parameter, so it can get cmake and version repositories right from the already-downloaded vcpkg/Build2/conan directories.

The library does not declare its standard requirement.

I'd like to avoid this if at all possible. I can get rid of CMAKE_CXX_STANDARD (which only affected executables internally, and not the interface libraries) but I have no intention on giving someone a build error if some check for C++17 fails; I would much rather document we use some C++17 features and if someone back-fills or poly-fills what they need, that's fine.

Install rules are wrong

I'll fix them! Albeit I'm not sure what's wrong: the install directory looks okay to me? I think the only thing missing is the singles. Are the docs not supposed to be nested in "sphinx"? What happens if I have other outputs? -

Looks okay...

Globbing sources

I'm going to be honest with you; if your FS / build system doesn't support globbing or doesn't support letting CMake re-invoke itself to check the generated file list, it's probably not going to have a good time if I start using generator expressions and other things (which I plan to do!). That being said, maybe when I hit 1.0.0 I'll use a raw file list since the file names and everything will have stabilized by then.

@friendlyanon
Copy link
Author

  • You don't need to void anything, just make them not part of the default code paths. A separate "dev mode" works really well for that, as it's opt-in and if you use CMake presets, you won't even have to think about it, you just configure with cmake --preset=dev and move on.
  • Whether FetchContent can be overridden or not, is not the issue. Network requests being opt-out is. In fact, if you just used find_package, then you could keep using FetchContent for your own developer workflow and you'd spare users of your library from having to work around it. Example
  • There is no point in overthinking it. Declare your requirements, so they are clearly visible in your API. Compile features make this extremely convenient for users and developers alike, as it declares a lower bound, which still can be overridden from the CLI using CMAKE_CXX_STANDARD. If you think this would be a problem for those "stuck" on a specific standard, those people already vet their dependencies and compile commands, so it's pointless to worry about that group of people.
  • The install rules only install the headers and that's it. A proper installation should also include a relocatable CMake package that is architecture independent, install components to not clash with projects that decide to vendor this library and for trivial dev pakcage creation for package maintainers. You are also using configure_package_config_file, which is intended for packages that have package components, like find_package(Boost REQUIRED COMPONENTS program_options) corresponding to the Boost::program_options target. You don't have such components, so it's unnecessary. Example
    I'm not sure what documentations sphinx generates. If it's man pages, then you can install those as part of the dev component to an idiomaic location.
  • I'm sorry to let you down, but reality doesn't care about honesty. Globbing simply just doesn't work portably. Ninja, which is a very recent build system, keeled over because of globbing before the 1.10.2 (latest) patch. I don't know what else would get the point across other than this and the CMake developers telling you not to glob.

@ThePhD
Copy link
Contributor

ThePhD commented Jul 3, 2021

* You don't need to void anything, just make them not part of the default code paths. A separate "dev mode" works really well for that, as it's opt-in and if you use CMake presets, you won't even have to think about it, you just configure with `cmake --preset=dev` and move on.

Sure, I can work on that or something. The scratch variables and scratch target will stay, if only because I don't feel like adding a whole new independent CMakeLists.txt just for debugging purposes.

* Whether FetchContent can be overridden or not, is not the issue. Network requests being opt-out is. In fact, if you just used `find_package`, then you could keep using FetchContent for your own developer workflow and you'd spare users of your library from having to work around it. [Example](https://github.com/beached/daw_json_link/pull/239#issuecomment-863882009)

FetchContent isn't for "developer" workflow, as far as I understand it. It's supposed to be THE workflow, for everyone. Letting someone specify the source with FETCHCONTENT_SOURCE_DIR_ZTD.CMAKE=~/code/ztd.cmake, or override any of the other variables directly, is the intended goal. I understand that someone may depend on a library that doesn't have full integration with CMake or FetchContent yet: the best I can do here is a if (NOT TARGET ztd::idk) or if (NOT TARGET ztd::cuneicode) and let someone else provide the target with their own find_package or something else (which I'll add in soon, I guess). Otherwise I'm not really interested in writing find_package/FindZtdCmake or similar since, at least for the moment, all of the dependencies of this library are explicitly controlled by myself and Shep.

* There is no point in overthinking it. Declare your requirements, so they are clearly visible in your API. Compile features make this extremely convenient for users and developers alike, as it declares a lower bound, which still can be overridden from the CLI using `CMAKE_CXX_STANDARD`. If you think this would be a problem for those "stuck" on a specific standard, those people already vet their dependencies and compile commands, so it's pointless to worry about that group of people.

Well, okay. I'll figure something out that hopefully won't error people on C++14-with-polyfills.

* The install rules only install the headers and that's it. A proper installation should also include a relocatable CMake package that is architecture independent, install components to not clash with projects that decide to vendor this library and for trivial `dev` pakcage creation for package maintainers. You are also using `configure_package_config_file`, which is intended for packages that have package components, like `find_package(Boost REQUIRED COMPONENTS program_options)` corresponding to the `Boost::program_options` target. You don't have such components, so it's unnecessary. [Example](https://github.com/friendlyanon/cmake-init-header-only/blob/master/cmake/install-rules.cmake)

I still don't get this part but I'll dig into it and figure out how to make that apply here.

  I'm not sure what documentations sphinx generates. If it's man pages, then you can install those as part of the `dev` component to an idiomaic location.

Well, okay. It seems to be going to the right place right now for the DOCDIR, so I'l leave it like that.

* I'm sorry to let you down, but reality doesn't care about honesty. Globbing simply just doesn't work portably. Ninja, which is a very recent build system, keeled over because of globbing before the 1.10.2 (latest) patch. I don't know what else would get the point across other than this and the CMake developers telling you not to glob.

I am just going to have to say that it's unfortunate; the globs are going to stay. They're fast, convenient, and work with plenty of build systems. CMake even has a special "glob checking" step is performs for build systems that aren't glob-smart yet, so it can just regenerate the content before-hand! I'm not really worried about ninja messing up, because I've been using globs for the last ~2 years with all 3 of Makefiles (normal + MinGW), ninja, and MSBuild w/ Visual Studio. If you've got an example of a place where globs will fail and take the build, do let me know and I'll reconsider, but otherwise I consider it a non-issue.

I'll get to work on everything else!

@ThePhD ThePhD added build Related (bug or feature) to the build system enhancement New feature or request labels Jul 3, 2021
@friendlyanon
Copy link
Author

  • You don't need to remove anything, like I said a "dev mode" code path is all that's necessary. With a preset enabling this code path, you won't even notice it's there. The consumer code path shall not contain anything other than the build and usage requirements and the install rules. If you want a project to be FetchContent ready, you need to do some extra things, due to the scoping of targets, variables and the lack of SYSTEM for includes. All of these is something cmake-init covers.
  • find_package is the canonical way to consume dependencies. The fact that find_package just stores the folder of the script it found as package_DIR (which you can predefine/overwrite from the CLI/in a preset) and delegates to the script in that folder makes it a far better customization point, because it's just another script that gets executed which could contain any arbitrary code.
  • As a last-ditch effort to dissuade you from globbing, I can refer the Alex Reinking's blog post that touches on this subject: https://alexreinking.com/blog/how-to-use-cmake-without-the-agonizing-pain-part-2.html

Build tooling for C++ using CMake is a topic I have been heavily looking into for the better half of the last year and cmake-init culmination of that effort. I can also probably count on one hand who cares about build tooling as much as I do besides myself and it's no secret that it shows very much on the ecosystem.

@ThePhD
Copy link
Contributor

ThePhD commented Jul 4, 2021

As a last-ditch effort to dissuade you from globbing, I can refer the Alex Reinking's blog post that touches on this subject: https://alexreinking.com/blog/how-to-use-cmake-without-the-agonizing-pain-part-2.html

From the article:

Besides, manually listing source files is typically only annoying at the start of a new project, when the code structure is much more fluid.

This project is not even in version 1 yet. I am literally in the process of tearing out an enormous amount of the shims and detail code to be placed somewhere else. I am absolutely not going to stop globbing, as I said earlier, until the project's structure becomes more fixed

find_package is the canonical way to consume dependencies.

find_package is the canonical way to consume binary or system dependencies through which you have no control and may need to share with others. There is no reason that:

  • ztd.idk (https://github.com/soasis/idk) needs to be a binary/system dependency: it belongs to the people writing ztd.text and is header-only so far
  • ztd.cmake (https://github.com/soasis/cmake) needs to be a binary/system dependency: it belongs to the people writing ztd.text and is only for some helpful build presets and functionality that may expand over time
  • Catch2 (https://github.com/catchorg/Catch2) needs to be a binary/system dependency: it is for developers of the library who want to run the tests and is self-contained (does not spill up-stream to people depending on this library)

If and when this changes, I'd be more than happy to fully explore adding Findztd.cmake and Findztd.idk and what that means in general. For now, given that these are source-based distributions with no compiled dependencies, I have no intention of writing or copy-pasting or generating find_package infrastructure for them, especially since the CMake team-proper is already working on a "find_package_or_fetch_content" feature for newer CMake (which I'd probably migrate to after this is done).


As for "dev mode", I still do not understand what you are getting at, at all. The "dev mode" is something I have by having my own CMakeUserPresets.json that turns on all the variables I'm interested in. Otherwise, they are off and the user only gets the library targets and the install targets. I still need to figure out how to export a CMake package declaration or whatever, but that's still a separate thing. I also do not understand half of the output from cmake-init, so I need to spend some time studying that.

@friendlyanon
Copy link
Author

Sure, early in the development globbing might be more convenient, however this is a header-only library and unless you add new .cpp files to the tests often, there is still no advantage to globbing. There is no need to add header files to add_library and add_executable command calls.

I'd be curious where you get that information regarding find_package. find_package is basically just an include on steroids and it is in fact the way to consume any dependency via relocatable CMake package config scripts or if the dependency (unfortunately) does not use CMake and does not provide CMake support, then find module scripts. A dependency can be neither binary nor system, e.g. when consuming a header only library from vcpkg or Conan, or when installing things to custom prefixes:

# "Backtrace" when importing a dependency:
# find_package(example REQUIRED) in your CMakeLists.txt
# include("${CMAKE_CURRENT_LIST_DIR}/exampleTargets.cmake") in exampleConfig.cmake

# In the CMake generated exampleTargets.cmake script:
add_library(example::example INTERFACE IMPORTED)

set_target_properties(example::example PROPERTIES
  INTERFACE_COMPILE_FEATURES "cxx_std_17"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/example"
)

You can completely forget about find modules, those are only for dependencies that don't use CMake and don't provide CMake support. You also don't need to copy-paste anything, CMake has for the longest time provided a way to generate CMake packages. You can already substitue dependencies that aren't installed anywhere on your system using the technique outlined here.

The dev mode is something that I came up to exclude absolutely everything not necessary to build a library, which also means less things to pollute the build tree with in case the project is vendored (e.g. using FetchContent). The less cache variables there are the better, because anyone looking at the cache variables as a consumer using ccmake or cmake-gui will see less options to mess with. Same goes for targets that are only relevant for developers, like the ones that come with include()ing the CTest module and any other targets like a scratch target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related (bug or feature) to the build system enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants