soasis / text

A spicy text library for C++ that has the explicit goal of enabling the entire ecosystem to share in proper forward progress towards a bright Unicode future.
https://ztdtext.readthedocs.io/en/latest/
Other
313 stars 23 forks source link

Improve CMake integration #14

Open friendlyanon opened 3 years ago

friendlyanon commented 3 years ago

At the moment, the CMake build files are messy.

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 commented 3 years ago

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 commented 3 years ago
ThePhD commented 3 years ago
* 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!

friendlyanon commented 3 years ago

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 commented 3 years ago

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:

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 commented 3 years ago

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.