Build appfwk using "Modern CMake" style

[philiprodrigues]

We should build appfwk using standard CMake conventions. WIP PR #82 has a proposal for how to implement that

[alessandrothea]

Preliminary question: where should the discussion take place? here or in the PR? I'll assume here for the moment.

The idea of introducing a per-package DAQModule that allows doing find_package(appfwk) is great and should be definitely pursued. In practice the example in the PR is confusing to look at as goes agains several guidelines that were established for daq packages aimed at minimising code duplication in CMakeList files. For instance,

• Dependency requirements are explicitly stated in the CMakeFile, which we agreed to avoid and have a central place where to put them.
• Importing DAQ.cmake and Findcetlib.cmake makes (a bit more) difficult to see how this model fits in the greater picture
• Vendoring nlohmann/json.hpp poses serious versioning issues, but I think this has already addressed by @brettviren

Also, it would be nice to see how the re-styled CMake files behave in a multi-package scenario with inter-dependencies i.e. user package that depends on appfwk, installed as an UPS product.

[philiprodrigues]

Preliminary question: where should the discussion take place? here or in the PR? I'll assume here for the moment.

Now we've started here, let's continue here.

I don't quite follow this:

* Dependency requirements are explicitly stated in the CMakeFile, which we agreed to avoid and have a central place where to put them.

in light of this:

The idea of introducing a per-package DAQModule that allows doing find_package(appfwk) is great and should be definitely pursued.

Are you just concerned about version numbers in the find_packages() commands duplicating information from the single-source-of-truth for dependencies, or something else?

* Importing `DAQ.cmake` and `Findcetlib.cmake` makes (a bit more) difficult to see how this model fits in the greater picture

If they're only needed by appfwk and packages that depend on it, then it makes sense for them to do in appfwk, but if we foresee having packages that don't depend on appfwk, but which will need the functions they define, then we can move them back to daq-buildtools. Partly I just moved them so I could make a single-package proof-of-concept PR without having to make a separate daq-buildtools PR too.

* Vendoring `nlohmann/json.hpp ` poses serious versioning issues, but I think this has already addressed by @brettviren

Agreed. Laziness on my part. daq-externals PR #2 has a ups product for nlohmann::json, so we can handle that package in the usual way.

Also, it would be nice to see how the re-styled CMake files behave in a multi-package scenario with inter-dependencies i.e. user package that depends on appfwk, installed as an UPS product.

I'll try to put together a small example

[brettviren]

My suggestions:

• remove any version string from calls to find_package() unless a truly minimum version is known (eg, we use some function introduced in Boost 1.21.0, then state that version). In most cases we need not do the detective work to find the true minimum until some poor sucker stumbles upon an incompatibility with their ancient environment. We absolutely should not list as "minimum" the version we happen to see in our local environment.

• rely on transitive dependencies so that a package only must "find" what it directly needs. If a package needs appfmk and has no other dependency not shared with appfmk then it must only "find" appfmk.

• if there is a way to package common info into a single CMake function call we should try to provide that. Ideally, the CMakeLists.txt file for a simple appfmk-based package needs two lines: one to find a CMake support file provided by appfmk and one to call a function from that library to list any dependencies. The rest follows from package file tree layout conventions. If a particular package needs something more then it's CMakeLists.txt file can grow as needed.

[philiprodrigues]

* remove any version string from calls to `find_package()` unless a truly **minimum** version is known (eg, we use some function introduced in Boost 1.21.0, then state that version).  In most cases we need not do the detective work to find the true minimum until some poor sucker stumbles upon an incompatibility with their ancient environment.  We absolutely should **not** list as "minimum" the version we happen to see in our local environment.

Good point. Fixed in f26ea7b

* rely on transitive dependencies so that a package only must "find" what it directly needs.  If a package needs `appfmk` and has no other dependency not shared with `appfmk` then it must only "find" `appfmk`.

The "Modern CMake" style, done right, ensures this, both in terms of find_package() and in terms of linking against libraries. For the find_package() part, the Config file (cmake/Config.cmake.in in this case), which cmake examines at find_package() time, uses find_dependency() to find_package() all of the transitive dependencies. For linking, the "exported" targets (like appfwk::appfwk) depend on other targets, which in turn pull in their dependencies.

* if there is a way to package common info into a single CMake function call we should try to provide that.  Ideally, the `CMakeLists.txt` file for a simple appfmk-based package needs two lines: one to find a CMake support file provided by appfmk and one to call a function from that library to list any dependencies.  The rest follows from package file tree layout conventions.  If a particular package needs something more then it's `CMakeLists.txt` file can grow as needed.

I don't quite follow this bit. There's an example appfwk-based package CMakeLists.txt file in the PR text. To find appfwk (and all its dependencies), the package just needs find_package(appfwk). If it needs the functions provided by DAQ.cmake, it should just be include(DAQ).

[brettviren]

@dingp gave some presentations in the past about using package file system tree conventions to determine the semantic meaning of sets of files which will then be interpreted by the build system. I forget the exact convention now but like src/ holds library source and any private headers, include/PackageName/ holds public headers, apps/ holds source files each providing a main(), etc.

Given such a convention there need not be any information in CMakeLists.txt except expressing general dependencies and anything "special" that is not covered by the conventions.

So, my third suggestion is, if it is reasonable to avoid essentially all boilerplate in CMakeLists.txt.

Here is an example of this pattern applied to a Waf build:

https://github.com/WireCell/wire-cell-toolkit/blob/master/sigproc/wscript_build

It's maybe easier to do this with Waf as one can bundle a lot of support directly in the waf executable. But, what I wonder is if CMake allows us define some function so that almost the entirety of CMakeList.txt may look like:

appfmk_package(MyPackageName DEPENDENCIES appfmk outside_external)

All this said, what you show in #82 is certainly reasonable.

[philiprodrigues]

Ah right, I see what you mean. I think that would be possible, although it might take a bit of work to get right, like making sure that the fallback path (what the user has to do when the single-command version doesn't quite do the right thing) is straightforward and clear. I expect there are examples out there that we could borrow/steal from. Your suggestion reminded me a little of the art_make() command, but that command is a per-directory thing that tries to build the correct sorts of art plugins based on file names:

https://cdcvs.fnal.gov/redmine/projects/art/repository/revisions/develop/entry/Modules/ArtMake.cmake

whereas appfwk_package() would be a per-package command that descends into directories.

[philiprodrigues]

Also, it would be nice to see how the re-styled CMake files behave in a multi-package scenario with inter-dependencies i.e. user package that depends on appfwk, installed as an UPS product.

I couldn't come up with a way to provide a nice example, so I'll try to just describe it: there's a wrapper in daq-externals that will create a ups product for appfwk, so when we tag a version of appfwk, we would make a ups product for it and install the product in cvmfs. Then, a possible option to build some package userpackage would be for the user to have a CMakeLists.txt like the one I showed in the PR conversation. They could then setup the necessary environment with:

source /cvmfs/dune.opensciencegrid.org/dunedaq/DUNE/products/setup
setup appfwk vX_Y_Z -q e19:prof

and build their package "like normal":

git clone https://github.com/user/userpackage
mkdir build && cd build
cmake -DCMAKE_INSTALL_PREFIX=/path/to/install ../userpackage
make              # or ninja, say
make install

(The userpackage cmake call will find the currently-setup ups appfwk from cvmfs via find_package() using cmake's "config mode", described in great detail at https://cmake.org/cmake/help/latest/command/find_package.html . Basically, appfwk installs a file appfwkConfig.cmake in a well-known directory in $CMAKE_PREFIX_PATH (an environment variable, which is set by ups setup). This config file in turn finds appfwk's dependencies, and exports targets like appfwk::appfwk that userpackage can use).

None of this deals with how to make userpackage available at runtime: that's the job of the package management system. I sort of like the idea of having the userpackage cmake file create a ups product, but the cmake-ups integration scripts I know about (the ones in cetbuildtools) are quite intrusive, so I think it would be tricky to include ups-product capability inside the userpackage build scripts now, and remove it later if/when we no longer use ups. That's where the system I've used so far, of having a separate wrapper package that creates the ups product, has an advantage: if we drop support for ups, we just stop using the wrapper, and nothing in appfwk (say) has to change.

[alessandrothea]

I think this issue has been extensively solved by the the recent versions of daq-buildtools and daq-cmake