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?
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.
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.
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
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.
@Đorđe Relić 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, @Đorđe Relić 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.
@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.
@Đorđe Relić 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 @Đ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)?
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:
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.
@Đorđe Relić 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
@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?