Switch to cmake

Issue #38 resolved
tkeller repo owner created an issue

For issue #31, we need a build system that compiles the planner both on Linux and Windows (and, if we ever want to add it as well, for MacOS).

The pull request where we discuss this can be found here:

https://bitbucket.org/tkeller/prost-dev/pull-requests/16

Comments (27)

  1. Relić Đorđe

    Firstly, for now, everything except test project is implemented. Test currently depend on generating .debug.o in the .obj/ folder. I would suggest rewriting tests using CTest (built in mechanism for tests in CMake) and merging tests for both search and parser into one testing script.

    Secondly, is build target coverage for rddl_prefix_parser and for serach the same? Could it be merged into one build target?

  2. Relić Đorđe

    Side note: Change in issue description iOS support to MacOX support. Also, concerning that, since MacOS is essentially UNIX system, we can easily check if PROST is already supported on it. I don't think PROST has any special requests that require great changes in the code.

  3. tkeller reporter

    @geisserf do you have an opinion on the CTest vs. google test comment of @djoler? From my point of view, it can be changed, but I don't use the tests as frequently as you do.

  4. geisserf

    In general I don't have a strong opinion in favor of GTest, but I if we want to switch the unit test framework, we should first discuss unit testing for PROST in general and also take a look at other testing frameworks (Catch for example).

    In general, I think it should be possible to use the current tests with CMake (e.g. here), I can also take a look at it, if you want.

    As a sidenote, I think we can drop the coverage build target. After all we did not really use it; maybe we can take a second look at it when we discuss unit testing.

  5. tkeller reporter

    In general, I think it should be possible to use the current tests with CMake (e.g. here), I can also take a look at it, if you want.

    @geisserf that would be great!

    As a sidenote, I think we can drop the coverage build target. After all we did not really use it; maybe we can take a second look at it when we discuss unit testing.

    I think @djoler dropped this, didn't he?

  6. Relić Đorđe

    In general, I think it should be possible to use the current tests with CMake (e.g. here), I can also take a look at it, if you want.

    geisserf that would be great!

    @tkeller @geisserf I don't have much experience in writing unit tests so before searching for solution that would incorporate existing tests and CMake (which if done fast would be a hack) I wanted to discuss other possible options and also review existing tests. Some of the tests fail for search project and for parser there aren't that many (for prefix parser only one test and none for flex/bison parser), so reviewing (and maybe refactoring/rewriting/adding more) tests would allow me to get to know testing framework more and modify it so that it corresponds new build system and is easy to scale up if such need occurs.

    As a sidenote, I think we can drop the coverage build target. After all we did not really use it; maybe we can take a second look at it when we discuss unit testing.

    I think Đorđe Relić dropped this, didn't he?

    @tkeller It is not implemented, but I waited for a confirmation to drop it completely.

  7. tkeller reporter
    • I don't have an opinion on the tests. Can you guys find a solutions for this problem?
    • We can drop the coverage build target.
  8. geisserf

    @DjoleR I don't agree that incorporating GTest and CMake would be a hack, after all CMake has its own keywords to locate the gtest framework. However, reviewing the current tests is a very valid point. As you said, we only have a few tests (even when fixing the failing tests) and I think, maybe except the test for evaluation of logical expressions, all of the current tests don't really cover many test cases. Refactoring, rewriting and adding more tests would obviously be great; but I think that this lies out of the scope of this issue.

    @tkeller, @DjoleR I think we should prioritize the switch to CMake and therefore not worry too much about unit tests. As our current tests are not really significant, I think it is the best if we drop them for now and do not worry about the testing framework. When the switch to CMake is complete and everything works under Windows we can thoroughly think about testing and testing frameworks in general.

  9. tkeller reporter

    I fully agree. However, before merging this, I'd like to perform some experiments to make sure that all optimization flags are identical. I'll report the results when they are finished.

  10. geisserf

    Is there an equivalent command to make's -j to allow multiple cores?

    We should also update the README before we merge this to master.

  11. Relić Đorđe

    @geisserf "a hack" is definitely a wrong term without further elaboration. What I meant by saying it is that, considering the way how I implemented CMake build system so far was looking at existing Makefiles and replicating what is done there by writing corresponding CMake code. For parser and search build target (both release and debug), it turned out to be not hard because all the files are in one folder and there are not many hard coded values (such as paths and extensions) while there are more of those for tests. So, before going further with that implementation I wanted to consult with you. I agree that the topic I started is out of scope for this issue but I wanted to bring it up since I ran into CTest. I will then drop the tests and merge this branch with issue-31 branch and proceed with Windows support. I will not remove existing Makefiles since those are needed for comparing the experiments but we should delete them once we the correctness of new build system is confirmed.

    Is there an equivalent command to make's -j to allow multiple cores?

    Every flag that is passed to script build.py, if not recognized as config_param or --help is passed to make so you can even now call ./build.py -jX and make will be started with X cores.

  12. geisserf

    @DjoleR I see, so the issue lies more in the current structure of our tests than with the framework itself I guess.

    Every flag that is passed to script build.py, if not recognized as config_param or --help is passed to make so you can even now call ./build.py -jX and make will be started with X cores.

    Great!

  13. tkeller reporter

    @DjoleR found out that the current cmake version doesn't find libbdd on our grid, which has to be changed. Even though we cannot make sure that the find script finds libbdd on every system, we should make sure it does so on most systems, and especially on our computers and the Freiburg and Basel grids. @geisserf, can you make sure cmake finds libbdd on the Freiburg grid (it suffices to do so once @djoler found a solution for our grid, this is more of a reminder to not merge this before we made sure it runs on most systems)?

  14. Relić Đorđe

    I 'fixed' the issue with changing the message type which is printed from FATAL_ERROR to WARNING. With those modifications, the compilation passes. I chose that patch because none of the proposed solutions for finding a library presented in CMake Wiki work. Some of them I discarded before since there was a simpler way, and for the more complicated ones, BuDDy is not compatible with them. Further more, I wanted to modify the current FindBDD.cmake file but I cannot locate where BuDDy is on the maia cluster. The ways I tried to locate it are listed below:

    [reldor00@maia ~]$ pwd
    /infai/reldor00
    [reldor00@maia ~]$ ldconfig -p | grep buddy
    [reldor00@maia ~]$ ldconfig -p | grep libbdd
    [reldor00@maia ~]$ ldconfig -p | grep libbdd-dev
    [reldor00@maia ~]$ ldconfig -p | grep fdd
    [reldor00@maia ~]$ ldconfig -p | grep fdd.h
    [reldor00@maia ~]$ ldconfig -p | grep lib
    1908 libs found in cache `/etc/ld.so.cache
            libzmq.so.3 (libc6,x86-64) => /usr/lib64/libzmq.so.3
            libz.so.1 (libc6,x86-64) => /lib64/libz.so.1
            ....
    

    Currently, with the modification I made, there is a warning that says that compilation will continue and in case it breaks, way to install BuDDy is suggested. If BuDDy is installed, the linking will pass. Installation is done using build.py script.

  15. tkeller reporter

    @DjoleR I wanted to finally run these experiments, but compilation with CMake still fails on the grid with this error message:

    Building configuration release
    cmake: /scicore/soft/apps/GCC/4.9.3-binutils-2.25/lib64/libstdc++.so.6: version `GLIBCXX_3.4.21' not found (required by cmake)
    Built configuration release failed due to CalledProcessError
    

    Any idea what's wrong?

  16. Relić Đorđe

    @tkeller For simplicity of the answer I'll refer to issue-31 as Windows port and to issue-38 as CMake.

    Latest CMake scripts are actually on branch Windows port.This was because midway through switching to CMake we decided to add Windows support for what we needed CMake and if I remember correctly, we discussed that we will first validate Windows port and then merge branches Windows port to CMake and then validate CMake. This was a mistake that I branched Windows port from CMake but they are tightly connected and I wanted to avoid duplicate code. Furthermore, we opted for removing tests until we find a way to incorporate them with testing frameworks offered by CMake or other testing frameworks so actually work on this branch is not done.

    So, if you want, I can copy the changes from Windows port to CMake and commit them so that you can run experiments on grid and then we can deal with merging of two branches later. Would that be what you would opt to?

  17. tkeller reporter

    @DjoleR, @geisserf:

    I merged the default branch into this, fixed things and everything seems to be compiling. Any reason we have not merged this yet?

  18. geisserf

    Well, from the above comments there seem to be the following topics which were in discussion:

    • perform some experiments to make sure that all optimization flags are identical
    • make sure cmake finds libbdd on the Freiburg grid ("it suffices to do so once @Đorđe Relić found a solution for our grid, this is more of a reminder to not merge this before we made sure it runs on most systems")
    • copy the changes from Windows port to CMake and commit them so that we can run experiments on grid and then we can deal with merging of two branches later

    I was not really involved in this issue, so all I can say that I did not yet perform any tests on the Freiburg grid.

  19. tkeller reporter

    OK, I'll have a look into issue #31 and see if there is anything that should be different on Linux as well. However, I'd rather have the Windows-related changes to cmake in issue #31 and merge this one just for the current (Linux-only) implementation. I'll run experiments in Basel afterwards and merge unless you want to test in Freiburg as well or one of you has any objections.

  20. tkeller reporter

    I ran experiments (see attachment) and, even though cmake seems to perform a little worse, it is actually a bit faster overall, as the following numbers (trials in first decision step and results of IDS training):

    wildfire 1, DP-UCT: make: Performed trials: 9626 cmake: Performed trials: 10338

    recon 5, IPPC2014: make: Performed trials: 29958 IDS: Search Depth 2: 0.00631555 / 250 = 2.52622e-05
    IDS: Search Depth 3: 0.0194599 / 250 = 7.78397e-05
    IDS: Search Depth 4: 0.0519172 / 250 = 0.000207669
    IDS: Search Depth 5: 0.138846 / 250 = 0.000555383
    IDS: Search Depth 6: 0.374515 / 250 = 0.00149806
    IDS: Search Depth 7: 1.58038 / 250 = 0.00632154

    cmake: Performed trials: 8404
    

    IDS: Search Depth 2: 0.00580304 / 250 = 2.32122e-05
    IDS: Search Depth 3: 0.0179654 / 250 = 7.18615e-05
    IDS: Search Depth 4: 0.0482889 / 250 = 0.000193155
    IDS: Search Depth 5: 0.132978 / 250 = 0.000531913
    IDS: Search Depth 6: 0.353311 / 250 = 0.00141324
    IDS: Search Depth 7: 1.17883 / 250 = 0.00471534
    IDS: Search Depth 8: 4.48007 / 250 = 0.0179203

    game 1, UCT-Single: make:Performed trials: 14924 cmake: Performed trials: 16824

    sysadmin8: make: Performed trials: 9913 IDS: Search Depth 2: 0.0238955 / 250 = 9.55821e-05
    IDS: Search Depth 3: 1.02179 / 250 = 0.00408718

     cmake: Performed trials: 11435
    

    IDS: Search Depth 2: 0.021978 / 250 = 8.79119e-05
    IDS: Search Depth 3: 0.884654 / 250 = 0.00353862

    I would have merged based on the results alone and only looked into these files to make sure nothing strange is happening. In all of these, cmake looks even better than make, so I do not see any reason not to merge this issue.

  21. geisserf

    I agree. Let me quickly check out this branch on the Freiburg Grid, just to see that everything compiles and job submission works as before.

  22. tkeller reporter

    I'll change a few more minor things (e.g., remove Makefiles) and create a pull request afterwards. You can then check if things work in Freiburg and propose changes in the pull request if necessary.

  23. Log in to comment