Cmake build2

[rkanavath]

splitted commits of #289

[wenzeslaus]

Hi, my hopes are high here as I got quite far even with the pre-PR versin, but with this version, I can't get beyond PostgreSQL. If I compile without it (cmake -DWITH_POSTGRES=NO ..), I get -lPOSTGRES in the compile/link command which of course fails:

Linking C shared library ../libgrass_gis.7.9.dev.so
/usr/bin/ld: cannot find -lPOSTGRES

If I compile with PostgreSQL (default), it ends with:

CMake Error at /usr/share/cmake-3.10/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find PostgreSQL (missing: PostgreSQL_TYPE_INCLUDE_DIR) (found
  version "10.12")
Call Stack (most recent call first):
  /usr/share/cmake-3.10/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
  cmake/find_scripts/FindPostgreSQL.cmake:123 (find_package_handle_standard_args)
  thirdparty/CMakeLists.txt:119 (find_package)

I'm on Ubuntu 18.04 (in Docker) with libpq5 and libpq-dev installed with CMake version 3.10.2

[rkanavath]

@wenzeslaus, Thanks for testing.
I think you are missing some headers in postgresql.
Does it help https://stackoverflow.com/questions/13920383/findpostgresql-cmake-wont-work-on-ubuntu?

You can try by running cmake:
cmake -DPostgreSQL_TYPE_INCLUDE_DIR=/usr/include/postgresql/

PS: if it works, I will push a fix that will not require anyone to provide -DPostgreSQL_TYPE_INCLUDE_DIR

[wenzeslaus]

You can try by running cmake:
cmake -DPostgreSQL_TYPE_INCLUDE_DIR=/usr/include/postgresql/

Yes, that's it. Thanks. I already figured that out. It seems I forgot to send a comment with a repo where you can see the test on Ubuntu VM: https://github.com/GRASS-GIS/grass-gis-experimental-ci/actions (branch cmake). (I'll follow up on the current issue there.)

As for figuring it out, I eventually guessed from the error message that missing: PostgreSQL_TYPE_INCLUDE_DIR part means add -DPostgreSQL_TYPE_INCLUDE_DIR=/usr/include/postgresql/. I'm just a CMake beginner. The piece I was actually missing was an equivalent of ./configure --help which now I know is cmake -LAH.

PS: if it works, I will push a fix that will not require anyone to provide -DPostgreSQL_TYPE_INCLUDE_DIR

You mean trying /usr/include/postgresql/ as an alternative automatically? That would make it easier for a lot of people down the road.

Besides that, my other issue was that with -DWITH_POSTGRES=NO, I was not not able to build because I got cannot find -lPOSTGRES when building.

[rkanavath]

@wenzeslaus

You mean trying /usr/include/postgresql

more like search using /postgresql/ among other things. cmake support it using PATH_SUFFIXES.

Besides that, my other issue was that with -DWITH_POSTGRES=NO

okay, I had to push a fix for it but cant do immediately, can you please test if -DWITH_POSGRES=TRUE, everything works?

actually missing was an equivalent of ./configure --help

Have to tried ccmake (curses ui) or cmake-gui.

[rkanavath]

@wenzeslaus ,
Is it possible to trigger a build on https://github.com/GRASS-GIS/grass-gis-experimental-ci/actions

I see an error from ctypegencore on your ci build (2 days ago)
https://github.com/GRASS-GIS/grass-gis-experimental-ci/runs/465728500?check_suite_focus=true

This issue has been fixed in other branch and is pushed here:
#357

You can simply build "ctypesgencore_py3" branch instead of this to test cmake. It contains this PR + more and does not have the error.

Multiple PR are simply a way to split changes that are not related.

[wenzeslaus]

I'm not sure what you want me to test, but the following works:

cmake \
    -DWITH_POSGRES=TRUE \
    -DCMAKE_INSTALL_PREFIX=$INSTALL_PREFIX\
    -DPostgreSQL_TYPE_INCLUDE_DIR=/usr/include/postgresql/ \
    .. 

While leaving out -DPostgreSQL_TYPE_INCLUDE_DIR=/usr/include/postgresql/from the above makes the configuration fail. -DWITH_POSTGRES=NO makes the configuration succeed and the subsequent compilation fails.

I have used ccmake in the past, but it does not really seem to work well with scripting workflow for me.

As for the build at GRASS-GIS/grass-gis-experimental-ci, the idea is to eventually have at least some part of it here, but at this point trying CMake build this way may create too much noise. I can just re-run the last test if needed from the website in this case. Generally, it updates with a change, so e.g. specifying the latest commit there force a particular version to be used, but that would be cumbersome. At this point, I'm expecting there there will be some changes needed anyway which will trigger the build (or on manually triggering the build).

For right now, before you rebase this, I switched there to your ctypesgencore_py3, so you can see the result there. The next issue I got before was incomplete make install step, i.e. GRASS GIS was not running because things were not in the right place, but it was possible to somehow run it from the build directory. In the VM, you just see that the main executable is not in place.

https://github.com/GRASS-GIS/grass-gis-experimental-ci/runs/470565935?check_suite_focus=true

[rkanavath]

Thank a lot @wenzeslaus. I really appreciate your efforts in testing this PR. So far there are two issues postgresql and make install. I will push a patch to it soon. Once all cmake and msvc stuff are merged, I plan to add testing using ctest. Currently there is a prototype in r.info https://github.com/rkanavath/grass/blob/cmake_build2/raster/CMakeLists.txt#L136

[wenzeslaus]

I will push a patch to it soon.

Awesome! I'm really glad we are moving forward with this. Thanks for working on it.

Once all cmake and msvc stuff are merged, ...

Having better testing in place before merging msvc might be helpful, but for that, activating testing like in GRASS-GIS/grass-gis-experimental-ci might be enough.

...I plan to add testing using ctest.

Sounds good, but before you do that, please let's discuss what would be the advantage of this approach. I used CTest just once to run some Python tests of C++ code, so I don't know if that would bring any advantage to us. Perhaps more direct testing of C code in modules? Are you familiar with gunittest-based execution of all tests (which is customization on top of Python unittest)? With that said, I think using a standard test execution system such as CTest or pytest, perhaps alongside the more specialized gunittest execution, would be helpful. (I'm the original author of gunittest.)

[rkanavath]

... but before you do that, please let's discuss what would be the advantage of this approach.

Idea is not to replace current testing with ctest. It is to run the same using ctest. Think of it as a wrapper/driver than can run test. ctest allow to have a dashboard (cdash) for these kinds of stuff.
Andwill be able to access configuration variables such as if grass is built with or without postgresql/opengl etc..?. I don't think gunittest handles this case now, One must be aware of config options before running tests. It also given developers who write gunittest to test specific modules that require external dependencies and later add a ctest code like "if(WITH_OPENGL) add_test(nviz_test) endif()" etc..

If one prefer to not use ctest and run gunittest that will be ofcourse doable. After all ctest is simply running them anyway.

Also I prefer to add it after msvc support which has many pending PRs.. Otherwise it will simply spam up the list of pending PR for grass developers and me.

[wenzeslaus]

@rkanavath, I have updated the code in this branch with code from master (e.g. Python 3 fixes) and with CI and submitted that as a PR against your repo, so you can decide how to incorporate that. Alternatively, I think I can commit to the branch directly or I can always create a new PR. What are your thoughts on this?

I'm not really sure what you meant to accomplish with CTest, but I would leave that for later. I would just limit the goal for this PR to running something like:

grass79 --tmp-location EPSG:4326 --exec g.region res=0.1 -p 

which is actually what .github/workflows/test_simple.sh is doing in the current master.

For this PR to be merged, I think we need:

A rebase to get the latest code from master into this PR: Simple git fetch upstream && git rebase upstream/master should do the trick as GitHub is saying this branch has no conflicts with the base branch (I did that in my PR). This may help with some of the issues which are on the your ctypesgencore_py3 branch but not here (it seems to me that it reduces the non CMake changes on the branch).

Even if it won't help with ctypesgencore, it will at least bring more CI tests to this PR which is another thing which needs to be added: CI build using CMake. I already put everything in place for this in the branch and it compiles, but the installation is lacking the main executable (grass.py).

This is already a lot to review, so let's try to remove all what is not needed or put that to different PRs which will precede this one. The following four commits from your branch seem to contain things which are perhaps related, but not directly related to CMake. These should be already reduced on my updated branch.

628e04b Avoid use of bare-exception
2d3d0ba use python3
6ea13bc check for GRASS_CONFIG_DIR to load rc and env.sh from it
6de92e6 Add cmake configure input files

if you want to preserve the commits above in a different branch (and PR), you can just add the commits from this branch to a new one (while being on the new one) using git cherry-pick <commit hash> and then revert the commits on this branch with git revert <commit hash>.

Everybody, this is a lot of reviewing and moving code around in Git, so if you have different suggestions on how to proceed here and with the other related PRs, please share your ideas.

[neteler]

I have now sync'ed the changes in this PR into the new Cmake PR #2684 (which has been rebased to main) in a1c304b.
According to diff all changes have been transferred so that this abandoned PR may longer be needed.

[neteler]

Closing this PR in favour of Cmake PR #2684 (where this work has been carried over). Thanks to @rkanavath!