PR: https://github.com/aibasel/downward/pull/164

So far, we collected the following recommendations, best-practices, and wishes for the new setup:

(1) cmake_minimum_required should be at least 3.4, on Ubuntu 20.04, a good choice would be 3.16
(2) properties should be set on targets, not globally (I think we already do this in most places)
(3) Find scripts should be used instead of hard-coding paths (we already do this)
(4) Modern CMake uses PRIVATE and PUBLIC when setting properties (in the old style, you could leave them out)
(5) Internal CMake variables (CMAKE_*) should not be modified with some exceptions (see next point).
(6) The C++ standard should be set and checked with CMAKE_CXX_STANDARD and CMAKE_CXX_STANDARD_REQUIRED.
(7) The old style was to use add_subdirectory() for each subdirectory, the new style is to group code into targets that are independent on the directory structure.
(8) Not sure how relevant this is for us because we wrap the CMake call in build.py: the old way of running cmake was
        mkdir build
        cd build
        cmake PATH_TO_SOURCE_DIR
        make OPTIONS_FOR_GENERATOR
    The new way is
        cmake -S PATH_TO_SOURCE_DIR -B PATH_TO_BUILD_DIR
        cmake --build PATH_TO_BUILD_DIR -- OPTIONS_FOR_GENERATOR
    We also want to use "-j NUM_CORES" on all operating systems (cmake supports -j directly)
(9) We ideally would like a better way to make the CPLEX DLL usable on Windows. So far, we copy it into the build directory with a rather hacky script. An alternative that we had in out 
GitHub actions was to set PATH to include its location but this would require user setup. Both 
seem not ideal solutions.
(10) Ideally, we would also like to avoid having to list explicit versions in the CPLEX find script.
(11) SoPlex installs with a cmake directory. Maybe this includes all configuration we need, so we do not require a find script for it?
(12) If there is a way to conditionally compile the specific LP solver interfaces for SoPlex and CPLEX, we maybe could avoid the large sections of #ifdef'ed code in them.
(13) If possible, we want a setup where the main part of the code is compiled as a C++ library, which is then linked against planner.cc to produce the binary, and against some other 
file to produce the python library. Maybe we can also have more fine-grained libraries with 
dependencies among each other to replace our current CMake-plugins.
(14) Remove the profile build target
(15) Find a better solution for copying the translator to the build directory.
(16) Set meta data when defining the project (languages, homepage, description)
(17) Control versioning from CMake
(18) Test if we still need to call cmake twice for multi-config builds. If so, document this.
(19) Document in our wiki that after the soplex installation, the environment variables soplex_DIR should be set (DOWNARD_SOPLEX_ROOT is no longer needed).
(20) make subdirectory search self-contained and remove hack with FAST_DOWNWARD_MAIN_CMAKELISTS_READ.
(21) Have the CMake find script for CPLEX return an imported library target rather than paths, this should also deal with the issue of copying the dll file to the output.
(22) check if we still need to set CMAKE_C_COMPILER/CMAKE_CXX_COMPILER before setting up the project.
(23) Find the right interface library to depend on rt and psapi.
(24) Update the leak check test.
(25) Update the dependency check test.
(26) Restructure CMake files. (which definition should go into which file in which order) In this context, get rid of FAST_DOWNWARD_MAIN_CMAKELISTS_READ.
(27) Check that the documentation about adding sources is up to date.
(28) Reverse order of compilation of all source files.

Thanks, Remo!

In case it's not clear, the reversed file order in the old CMake was there to make the planner run faster, not to make it build faster.

Regarding memory usage and runtime of the debug build, it's absolutely OK to lose a little bit here and there, not something we'd be concerned with as long as it's still usable for its purpose. (The huge difference due to USE_GLIBCXX_DEBUG affects usability, so it was good to catch that.)

During last week's Sprint we ran experiments to see whether performance is affected by the new build system. Through this testing we 
discovered a bug in the debug build, namely that the USE_GLIBCXX_DEBUG flag was enabled instead of disabled by default, which made runs with 
the debug build much slower. The experiments after the fix look as follows (`5ecdaef95` is the old build system, `87a61c5d3` is new):

- opt: https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1097/data/cmake-v2-opt-eval/cmake-v2-opt-5ecdaef95-87a61c5d3-compare.html
- sat: https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1097/data/cmake-v2-sat-eval/cmake-v2-sat-5ecdaef95-87a61c5d3-compare.html

For the debug build, the new system solves at least as many tasks as the old across all tested configurations.
The release build for the optimal configurations has some variance in time scores that can be explained by random noise and a constant 2MB 
increase in memory usage. The release build for satisficing search also has the constant 2MB memory increase (with some outliers in agricola-
sat18-strips and tidybot-sat11-strips) and the time scores deviate evenly in both directions and also look like noise.

Besides the performance of the compiled planner, we also looked at whether building time is affected. To do so I built the planner locally, 5 
times each for the old and new build system, with the following results:

Old (5 runs)
  Time (mean ± σ):     187.382 s ±  4.151 s
  Range (min … max):   180.084 s … 190.112 s

New (5 runs)
  Time (mean ± σ):     193.187 s ±  8.064 s
  Range (min … max):   183.160 s … 201.696 s

Unfortunately the new system is slightly slower on average, but not significantly enough to stop us from modernizing our CMake setup. This 
small experiment also somewhat resolves the concern about source file order raised in point 28 in the summary. We do not try to enforce a 
particular order in the current setup and I'm not sure whether this would even be possible. In the end, I think that the build times are not 
concerning enough to warrant further investigation into file orders.

Finally we adapted the documentation on the Fast Downward website to be consistent with the changes introduced (task 27 in the summary), 
merged the issue branch, and I'm now marking this issue as resolved.

Florian, Salomé, and I restructured the CMake files with the aim of improving the structure along with the modernized CMake code.

In the following I list some of the improvements and (broadly) how we implemented them:
- clearer separation between CMake scripts that are needed for the entire project and those only needed by the search component
    - move the contents of the src/cmake_modules directory into src/search/cmake because they are only relevant for building the search component
- simplify file and function names
    - FastDownwardMacros.cmake becomes macros.cmake
    - fast_downward_report_bitwidth() becomes report_bitwidth(), fast_downward_set_up_build_types() becomes set_up_build_types()
- rearrange code between files
    - move option definitions (USE_GLIBCXX_DEBUG, USE_LP from src/search/CMakeLists.txt and DISABLE_LIBRARIES_BY_DEFAULT from src/search/DownwardFiles.cmake) into 
src/search/cmake/options.cmake
    - move CXX flag and warning settings into common_cxx_flags.cmake
    - define Fast Downward libraries in src/search/CMakeLists.txt, making src/search/DownwardFiles.cmake obsolete

Our plan from here is to set up experiments to verify that performance is not affected, and to see if somebody is available for a code review.

I'm not sure we have control over this anymore, since we no longer add all source files at once. Previously, we collected a list of source files and then added them to the project but now each interface library has its own source files and we add the interface libraries as dependencies which automatically adds the files.

We should run an experiment to see if this still impacts performance. If the impact is not too bad, I'd still go with it, because optimizing on this level seems like a never-ending task where the optimal setting could change with every added file. This is true especially if we don't know exactly what makes one order better than another.

I noticed that the sources are now compiled in the order in which libraries are defined. Previously, we reversed the list of sources after collecting all of them for performance reasons (issue67). I added item (28) do restore the "reverse order".

I pushed a new find script for (21). Works on the GitHub actions at least.

(20) make subdirectory search self-contained is also done now. I however only tested two setups: calling ./build.py, and calling cmake in the search directory. It would probably be good if we test other settings.

I added the rt and psapi library as a (interface) dependency of the UTILS library. It's currently done in a very hacky way though. Ideally I would like us to be able to specify external dependencies in DownwardFiles.cmake, but these libraries have the complication that they are only needed under certain conditions (OS in this case).

I updated the leak check. This necessitated a new file conftest.py in the misc/test directory to set up a command line argument. I dislike this since it's only used by test-memory-usage.py, but I did not see a way around it. My theory is that it won't interfere with the other tests as long as they don't use a variable "suppression_files", but it would be good to have someone else double check the code.

As an aside, I also turned finding the soplex package on quiet so one doesn't get a warning when soplex is not present. This was not a documented point to fix but we discussed it in discord.

Resolve item 22. CMake respects CC and CXX variables also on the Basel grid, so the worries mentioned in issue1031 should no longer be a reason to keep these two lines.

I updated the dependency check test in the PR. 
I also made finding the library names more robust. The old test actually had a bug, it did not remove "LP_SOLVER" from the list because it interpreted "DEPENDENCY NAMED_VECTOR" as a new library name "D_VECTOR" and removed that instead :).

(17) is also handled in the follow-up issue because versioning is more important there.

The follow-up issue to try actual libraries is now issue1103.

I updated the TODO list:

(2) is done (we now use targets for everything).
(3) is done (we didn't hard-code any paths).
(4) is done (we now use PRIVATE/PUBLIC/INTERFACE everywhere)
(5) is done (the remaining CMAKE_* variables we set are intentional)
(7) is done (using add_subdirectory is not a problem in itself, we just wanted to switch to a target-based setup. We still have one thing to do here, which is now (20)).
(9) We found out a way to deal with the dlls: FindCplex.cmake should define an IMPORTED library which specifies IMPORTED_LOCATION (dll) as well as IMPORTED_IMPLIB (.lib). This is now (21).
(13) It should now be easy to turn our interface libraries into actual libraries but we decided to defer this to a follow-up issue.

New points: 
(22) check if we still need to set CMAKE_C_COMPILER/CMAKE_CXX_COMPILER before setting up the project.
(23) find the right interface library to depend on rt and psapi.
(24) update the leak check test
(25) update the dependency check test
(26) Restructure CMake files. (which definition should go into which file in which order) In this context, get rid of FAST_DOWNWARD_MAIN_CMAKELISTS_READ.
(27) Check that the documentation about adding sources is up to date.

Item 18: it is not necessary to use two cmake runs

We can get rid of our SoPlex find script if we replace DOWNARD_SOPLEX_ROOT with soplex_DIR in our instructions.

Remo handled points (15) and (16). Regarding the translator there seems to be no better option than what we already do. I reduced the code in #ifdefs in the LP solver code which we can do now because we only compile the solver interfaces if the solver is present.

I added another point that Silvan brought up:

(18) Document in our wiki that cmake must be run twice for mult-config generators

I added two more points to the overview:

(16) Set meta data when defining the project (languages, homepage, description)
This should be straight-forward to do.

(17) Control versioning from CMake
This is a bit trickier because we have to make sure to make the version known to the source code in some way, but I think there are ways to do this. It also interacts with our release scripts that are currently responsible for bumping the version number.

I cross out items 6 and 10 as Florian has already finished them.

8 and 14 are done in the PR.

some more TODOs from a meeting

I created a pull request so we can track the changes. I started with the easy thing and upgraded the minimum required version.

I also added another point to the list: we have a profile build that nobody uses as far as I know. I suggest to remove it.

We want to use more modern CMake features. I put a list of desired changes in the summary.