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

Add tests to CMake build #92

Merged
merged 9 commits into from
Sep 5, 2016
Merged

Conversation

mwoehlke-kitware
Copy link
Contributor

Add necessary CMake logic to build and run the unit and client/server tests with CMake/CTest.

Demo:

Test project /usr/local/build/matthew/work-mit/lcm
      Start  1: C::memq_test
 1/13 Test  #1: C::memq_test .....................   Passed    0.01 sec
      Start  2: C::eventlog_test
 2/13 Test  #2: C::eventlog_test .................   Passed    0.00 sec
      Start  3: C::client_server
 3/13 Test  #3: C::client_server .................   Passed    0.12 sec
      Start  4: CPP::memq_test
 4/13 Test  #4: CPP::memq_test ...................   Passed    0.01 sec
      Start  5: CPP::client_server
 5/13 Test  #5: CPP::client_server ...............   Passed    0.09 sec
      Start  6: Python::bool_test
 6/13 Test  #6: Python::bool_test ................   Passed    0.02 sec
      Start  7: Python::byte_array_test
 7/13 Test  #7: Python::byte_array_test ..........   Passed    0.02 sec
      Start  8: Python::lcm_file_test
 8/13 Test  #8: Python::lcm_file_test ............   Passed    0.04 sec
      Start  9: Python::lcm_memq_test
 9/13 Test  #9: Python::lcm_memq_test ............   Passed    0.04 sec
      Start 10: Python::lcm_thread_test
10/13 Test #10: Python::lcm_thread_test ..........   Passed    0.33 sec
      Start 11: Python::client_server
11/13 Test #11: Python::client_server ............   Passed    1.99 sec
      Start 12: Java::client_server
12/13 Test #12: Java::client_server ..............   Passed    7.19 sec
      Start 13: Lua::client_server
13/13 Test #13: Lua::client_server ...............   Passed    0.97 sec

100% tests passed, 0 tests failed out of 13

Total Test time (real) =  10.87 sec

(Note: the Java client/server test may be flaky; I've seen it fail at least once, then pass when re-run. Also, trying to run the tests in parallel will end badly for fairly obvious reasons. Don't do that 😁.)

I haven't actually tried to run the tests using the old framework, so this is based on what I could figure out reading the various Makefiles and scripts. Please let me know if I missed something.

There are also some warning fixes in the branch because I build with -Werror=reorder -Werror=cast=qual, and the tests were tripping these. (Note especially the change in code generation. There should be no behavior changes, however.) I also noticed and fixed an error with the Python documentation generation on Windows.

Note that the client/server tests require Python. The old driver scripts are also Python, so this isn't a new. Also, this conveniently provides a good example of generating LCM types with CMake. (Especially for C++, which is a little screwy to set up, but works beautifully for consumers.)

...in lcm-lua, for consistency with lcm-python.
"Fix" some C casts between pointer types that also unnecessarily
discarded a `const` qualifier. Use `const_cast` instead of C casts in a
few spots that really do need to remove a const qualifier. (This fixes
spots that were tripping `-Wcast-qual`, including in generated code.)
Rearrange member initializers to match declaration order, i.e. the order
in which the compiler actually executes them. This fixes -Wreorder
violations.
Add CMake logic to build the gtest library and the C/C++ unit test
executables. Register "pure" (i.e. the ones that don't need a server)
C/C++ unit tests so they can be run with CTest.
Fix building Python documentation to use the correct path separator for
PYTHONPATH on Windows.
Create a Python helper script to launch the C test server and run a
specified test client. Set up client/server tests for CTest for C, C++,
Python and Lua.

This should account for all of the existing tests except for the Java
client/server test.
Build dependency JAR's for Java client/server test, and the JAR for the
test itself. Add necessary logic to register and run the test with
CTest.
@ashuang
Copy link
Member

ashuang commented Jul 30, 2016

I get the following configure error:

CMake Error at test/types/CMakeLists.txt:42 (add_dependencies):
  add_dependencies Cannot add target-level dependencies to INTERFACE library
  target "lcm-test-types-cpp".

I'm on cmake 3.2.2

Also, it looks like you've mixed in some C++ code (e.g., I spotted a const_cast<>).

@mwoehlke-kitware
Copy link
Contributor Author

I get the following configure error [...]

Ack, it looks like the ability to add dependencies to INTERFACE targets was added in 3.3.0. I can work around it, but it's somewhat ugly; anyone needing the headers would need two lines (one to pull the include directory, and one to add an ordering dependency), whereas the current code does both with just the INTERFACE target but requires CMake ≥ 3.3.0. How would you feel about bumping the minimum version to 3.3.0?

Also, it looks like you've mixed in some C++ code (e.g., I spotted a const_cast<>).

Yes, but only in C++ sources, no? (The instance in emit_cpp.c is in the emitted code.)

@ashuang
Copy link
Member

ashuang commented Aug 2, 2016

hmm...

The main challenge with requiring such a new version of CMake is that most LCM Linux users will not be running a distro with CMake 3.5. Even users on OS/X and Windows may be running an older install, and it's nice to not force people to upgrade dependencies.

One of the main priorities for LCM is wide portability and compatibility. Requiring a build system that was released less than 6 months ago isn't super conducive to that =/ That said, I'm okay bumping the CMake required version to 3.5 if we:

  1. Keep the autotools build around for a while, and also as the officially recommended way to build
  2. Mark the CMake build as experimental

If you'd like to purge the autotools build system, which I'm assuming you do judging by the frequent comments... ;) then my request is that we maintain backwards compatibility with CMake 2.8 (picked mostly because that's what shipped with Ubuntu 14.04)

Thoughts?

@mwoehlke-kitware
Copy link
Contributor Author

Most LCM Linux users will not be running a distro with CMake 3.5.

3.3 is sufficient for the current state of this branch. Note that Fedora 22, which recently went EOL, has CMake 3.3.1.

Even users on OS/X and Windows may be running an older install, and it's nice to not force people to upgrade dependencies.

...but it's as easy to install a newer CMake on Windows and OS X as it was to obtain whatever version the user has currently.

My request is that we maintain backwards compatibility with CMake 2.8.

This would be... difficult; there are several places where cmake -E env is being used, which requires CMake ≥ 3.1. It's possible to work around this but it adds non-trivial complexity. (Keep in mind that the "obvious" solution, using /usr/bin/env instead, is not portable. Specifically, it's not available on Windows, and at least one of the places using cmake -E env is Windows-specific.)

(We'd also have to go back to carrying a copy of FindLua.cmake... which is much easier, but still...)

What about following what some other projects have done and making 1.3.x a "LTS" branch (still using autotools) and moving just master to CMake-only? (I could live with staying at 3.1, tagged 2014-12-15, as the minimum requirement. In fact, given this discussion, I'll go ahead and add a work-around for pre-3.3 to this branch.)

CMake prior to 3.3 does not support dependencies on INTERFACE targets,
so add a work-around to ensure proper build order (namely, generating
the C++ test type bindings before compiling the C++ tests) for older
versions of CMake. (Just requiring 3.1 is a little controversial at this
time, so we'd rather not bump the minimum requirement even higher just
now.)
@ashuang ashuang merged commit fa6b9a9 into lcm-proj:master Sep 5, 2016
@ashuang
Copy link
Member

ashuang commented Sep 5, 2016

Sorry for the slow action on this. I've merged in this PR and applied a minor fix (#101).

@mwoehlke-kitware
Copy link
Contributor Author

No worries, I've also been busy with other things. Thanks!

@mwoehlke-kitware mwoehlke-kitware deleted the tests-cmake branch September 5, 2016 14:43
mojasp pushed a commit to Barkhausen-Institut/lcm-sec that referenced this pull request May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants