Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modern CMake configuration refactoring #130

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@mloskot
Copy link
Contributor

commented Oct 20, 2018

Minimal build and install features implemented according to Modern CMake best practices for all GEOS targets, libraries and tests.

Require CMake 3.5 or later.
Add cmake/CMakeSettings.json sample with convenient defaults for users of CMake integration in Visual Studio 2017 (usage: copy to root folder).
Add cmake/geos-configure-file.cmake from Dan Baston (@dbaston).


I tested it with CMake 3.11+ on Linux (GCC, clang) and Windows (Visual Studio).


@robe2, @pramsey, @dbaston - following our discussions on IRC, here I pushed complete update of my ml/modern-cmake branch, rebased on the latest master @ c818506

Tasklist

  • Iteration of CMake script clean-ups adding missing unset, removing any unnecessary bits and bobs (eg. variables), etc.

  • Add GEOS client example configured with CMake as a test to verify geos-config.cmake package configuration file deployment.

  • Update README.md with building, testing, installation and using GEOS with CMake

  • Update .travis.yml with complete workflow: configure, build, test, install, configure and build client. Install latest CMake. Remove Autotools jobs.

  • Update .appveyor.yml with workflow: configure, build, test. Install latest CMake. Remove NMake jobs.

  • Update .appveyor.yml with rest of workflow: install, configure and build client.

  • Tag version with git hash, tag or -dirty indicator for modified working copy (see CMakeVersionSource.cmake and related scripts) in CMake's codebase)

  • Review compilation flags for supported compilers for both consumers, client and developer

  • Replace ugly old-school if-s setting flags per compiler with generator expressions passed to target_compile_options and target_compile_definitions, for example:

    $<$<CXX_COMPILER_ID:GNU>:-Wall;-Wextra;-Wpedantic>
    $<$<CXX_COMPILER_ID:MSVC>:NOMINMAX>
  • Add support for OSX Framework

  • Deal with tools/CMakeLists.txt or remove alogn with geos-config script

  • Add CPack integration

  • Add target for Doxygen build

  • ...

Since I'm not sure if/when I'd be able to complete those TODOs, I'll hand the work over to you now.

@mloskot mloskot force-pushed the mloskot:ml/modern-cmake branch 15 times, most recently from de14d18 to 33564e0 Oct 20, 2018

@mloskot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2018

The latest CI builds of this PR are failing due to xmltester issues:

AppVeyor - https://ci.appveyor.com/project/dbaston/geos-rxhmj/builds/19662254

  1. 64-bit target - https://ci.appveyor.com/project/dbaston/geos-rxhmj/builds/19662254/job/u4qkysyek1qyugep
2/7 Test #2: test_xmltester ...................***Failed  216.99 sec
symDiffArea frac: 0.682742 tolerated 0.001
isSymDiffAreaInTolerance failed
BufferResultMatcher FAILED
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case1: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case2: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case3: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case4: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case5: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case6: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case7: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case8: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case9: test1: samedirection(A, B): skipped (unrecognized).
Expected result is of type GeometryCollection; obtained result is of type Polygon
Using an overlay tolerance of 2.09175e-12
Using an overlay tolerance of 1.70445e-11
Files: 88
Tests: 2948
Failed: 4
Succeeded: 2935
  1. 32-bit target - https://ci.appveyor.com/project/dbaston/geos-rxhmj/builds/19662254/job/78b72uhfd2l26gw3
2/7 Test #2: test_xmltester ...................***Failed   41.02 sec
symDiffArea frac: 0.682742 tolerated 0.001
isSymDiffAreaInTolerance failed
BufferResultMatcher FAILED
C:/projects/geos-rxhmj/tests/xmltester/tests/rt/TestSameDirection.xml: case1: test1: samedirection(A, B): skipped (unrecognized).
C:/projects/geos-rxhmj/tests/xmltester/tests/rt/TestSameDirection.xml: case2: test1: samedirection(A, B): skipped (unrecognized).
C:/projects/geos-rxhmj/tests/xmltester/tests/rt/TestSameDirection.xml: case3: test1: samedirection(A, B): skipped (unrecognized).
C:/projects/geos-rxhmj/tests/xmltester/tests/rt/TestSameDirection.xml: case4: test1: samedirection(A, B): skipped (unrecognized).
C:/projects/geos-rxhmj/tests/xmltester/tests/rt/TestSameDirection.xml: case5: test1: samedirection(A, B): skipped (unrecognized).
C:/projects/geos-rxhmj/tests/xmltester/tests/rt/TestSameDirection.xml: case6: test1: samedirection(A, B): skipped (unrecognized).
C:/projects/geos-rxhmj/tests/xmltester/tests/rt/TestSameDirection.xml: case7: test1: samedirection(A, B): skipped (unrecognized).
C:/projects/geos-rxhmj/tests/xmltester/tests/rt/TestSameDirection.xml: case8: test1: samedirection(A, B): skipped (unrecognized).
C:/projects/geos-rxhmj/tests/xmltester/tests/rt/TestSameDirection.xml: case9: test1: samedirection(A, B): skipped (unrecognized).
EXCEPTION on case 1 test 1: bad allocation
Using an overlay tolerance of 2.09175e-12
Using an overlay tolerance of 1.70445e-11
Files: 88
Tests: 2948
Failed: 4
Succeeded: 2935

Travis CI - https://travis-ci.com/libgeos/geos/builds/88630941

2/7 Test #2: test_xmltester ...................***Failed  216.99 sec
symDiffArea frac: 0.682742 tolerated 0.001
isSymDiffAreaInTolerance failed
BufferResultMatcher FAILED
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case1: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case2: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case3: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case4: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case5: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case6: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case7: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case8: test1: samedirection(A, B): skipped (unrecognized).
/home/travis/build/libgeos/geos/tests/xmltester/tests/rt/TestSameDirection.xml: case9: test1: samedirection(A, B): skipped (unrecognized).
Expected result is of type GeometryCollection; obtained result is of type Polygon
Using an overlay tolerance of 2.09175e-12
Using an overlay tolerance of 1.70445e-11
Files: 88
Tests: 2948
Failed: 4
Succeeded: 2935

@mloskot mloskot force-pushed the mloskot:ml/modern-cmake branch 2 times, most recently from 8c197cf to 914935a Oct 29, 2018

@mloskot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

FYI, updated lowering CMake requirement to 3.11, but it may be possible to lower it further eg. to 3.5. I just have no mean to test it against 3.5.

@strk

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

@mloskot mloskot force-pushed the mloskot:ml/modern-cmake branch from 914935a to 5ff2951 Oct 29, 2018

@mloskot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

Thanks for testing. I've applied workaround to add_executable so it works with earlier than 3.11.
I also lowered CMake requirement to 3.5 - no idea if it will work though.

@strk

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

@mloskot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

@strk

CMake 3.10.2 worked, and subsequent make invocation took:
real 3m36.079s user 3m17.767s sys 0m18.087s

  1. CMake + GNU Make
$ time cmake ..

real    0m11.923s
user    0m0.859s
sys     0m6.906s

$ time make -j 16

real    0m56.148s
user    4m25.109s
sys     4m48.156s

$ time make -j 16 test
real    3m53.705s
user    3m42.078s
sys     0m11.047s
  1. CMake + Ninja
$ time cmake -G Ninja ..
real    0m8.443s
user    0m0.672s
sys     0m6.344s

$ time ninja -j 16
real    0m40.118s
user    4m26.375s
sys     4m22.875s

$ time ninja -j 16 test
real    3m33.519s
user    3m22.313s
sys     0m11.359s

make: *** No rule to make target 'check'. Stop. make: ***

make test

No rule to make target 'dist'. Stop. make: ***
No rule to make target 'distcheck'. Stop. make: ***
No rule to make target 'uninstall'. Stop.

"With CPack enabled, "make help" will show all the valid targets"
~https://cmake.org/pipermail/cmake/2014-October/058850.html

That, however, someone will have to work about.

No .so files (maybe because they were found already present ?)

By default, static libs are generated. Use CMake native BUILD_SHARED_LIBS to ask for shared.

Show resolved Hide resolved CMakeLists.txt Outdated
Modern CMake configuration refactoring
Minimal build and install features implemented according to Modern CMake
best practices for all GEOS targets, libraries and tests.

Require CMake 3.5 or later.
Add cmake/CMakeSettings.json sample with convenient defaults for users
of CMake integration in Visual Studio 2017 (usage: copy to root folder).

Add cmake/geos-configure-file.cmake from Dan Baston (@dbaston).

@mloskot mloskot force-pushed the mloskot:ml/modern-cmake branch from ab87322 to 20e1221 Oct 31, 2018

@mloskot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

@dbaston FYI, I've fixed the PR to make it single-commit again, as I prefer it.

Future of this PR

Since you have expressed interest to expand it, I think it may be a a good idea to consider either

  1. Merge it into GEOS master - CMake is secondary anyway, so users should survive if CMake does not work for a few weeks in the development branch (CI-s jobs can be disabled temporarily)

  2. Push it as modern-cmake branch in GEOS repo and continue work there, and close this PR.

Either way, I expect it to be easier for you to continue the work since I strongly prefer single-commit pull requests, so my workflow involves git rebase master and git push -f what obviously will cause trouble in collaborative work.

/cc @strk @robe2 @pramsey

@dbaston

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

@mloskot

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

@dbaston Great. Shall we close this PR?

@pramsey

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

I'm going to close this PR and hope Dan's branch is active.

@pramsey pramsey closed this Jan 29, 2019

@mloskot

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

Good idea.

@dbaston Can I delete my mloskot:ml/modern-cmake branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.