search

roboticslab-uc3m / questions-and-answers

A place for general debate and question&answer
https://robots.uc3m.es/developer-manual/appendix/repository-index.html
2 stars 0 forks source link

Look for system-installed googletest for use in Travis builds, drop embedded copies #41

Closed PeterBowman closed 6 years ago

PeterBowman commented 6 years ago

Currently, it's a common practice in repos that perform unit testing with Travis CI to host a gtest-x.x.x directory within test/, see kinematics-dynamics. Proposal: remove those files and install Google's testing framework via apt:

PeterBowman commented 6 years ago

Added TODO list in description. @jgvictores:

jgvictores commented 6 years ago

These months xgnitive is going to be a bit crazy, I'd wait till IROS 2018 deadline March 1 2018, ok? I think tools is a great place to start; I'll be working on https://github.com/roboticslab-uc3m/kinematics-dynamics/issues/134 at kinematics-dynamics but you can also work there as I'll just be touching TrajectoryLib on a separate branch.

PeterBowman commented 6 years ago

Funnily enough, libgtest-dev doesn't contain binaries we could use right away to build our unit tests. Per https://www.eriksmistad.no/getting-started-with-google-test-on-ubuntu/:

Note that this package only install source files. You have to compile the code yourself to create the necessary library files. These source files should be located at /usr/src/gtest. Browse to this folder and use cmake to compile the library:

The cost of not embedding GoogleTest framework in every single repository is... having to replicate the build instructions in .travis.yml to cover the compilation of GoogleTest itself.

jgvictores commented 6 years ago

Yes, the package description states Google's framework for writing C++ tests - header files.

I've been reading through a related bug report, where there was a proposal to take advantage of a gtest/gmock merge for a change... Only thing we'll see in the future for now is a name change from libgtest-dev to googletest but still no binaries in the file list.

Not sure what the best solution here is. I guess one practical solution would be to determine which method is faster, but that's only one of the possible criteria.

jgvictores commented 6 years ago

More bugs issued on Ubuntu end up pointing to bugs issued on Debian:

jgvictores commented 6 years ago

Okay, from ref 1 from above:

Because of the C++ "One Definition Rule", gtest must be compiled with exactly the same flags as your C++ code under test.

That... could actually make sense.

David-Estevez commented 6 years ago

You might already know this, but this is what Google recommends:

PeterBowman commented 6 years ago

Use CMake to download GoogleTest as part of the build's configure step. This is just a little more complex, but doesn't have the limitations of the other methods.

YCM could care about such complexity for us. Actually, I forgot that this was already an option (sorry!): https://github.com/roboticslab-uc3m/questions-and-answers/issues/1#issuecomment-284085241.

I'm going to update the title accordingly.

jgvictores commented 6 years ago

Are we sure we do not prefer pulling the source via apt?

PeterBowman commented 6 years ago

It would force us to build and compile it by hand (in Travis, build steps should be carefully replicated) instead of letting CMake do its work (current behavior). Also, the package solution implies compiling with sudo, manually copying/symlinking binaries, etc. (example).

jgvictores commented 6 years ago

The example needs to compile with sudo and manually copy because it does an in-source build. I think we could do better. Anyway, no personal preference. I guess since it's source and has to be compiled, it makes more sense to download the latest code anyway.

PeterBowman commented 6 years ago

it makes more sense to download the latest code anyway

I was thinking about making YCM pull & compile the exact same version we use right now (1.7.0).

jgvictores commented 6 years ago

I was thinking about making YCM pull & compile the exact same version we use right now (1.7.0).

:+1:

PeterBowman commented 6 years ago

It would force us to build and compile it by hand (in Travis, build steps should be carefully replicated) instead of letting CMake do its work (current behavior).

I'm still pondering on this, sorry. I'm definitely positive on using CMake somehow, although the YCM solution might be a bit too convoluted. Vanilla CMake already offers a FindGTest.cmake module, I'll probably test it soon.

jgvictores commented 6 years ago

I'm still pondering on this, sorry. I'm definitely positive on using CMake somehow, although the YCM solution might be a bit too convoluted. Vanilla CMake already offers a FindGTest.cmake module, I'll probably test it soon.

No worries! It's definitely a good idea to have thought it thoroughly, before propagating a solution throughout all repositories.

PeterBowman commented 6 years ago

FindGTest assumes that Google Test framework is already compiled, i.e. it looks for libgtest.so and libgtest_main.so (or .a) and reports an error if not found.

Because of the C++ "One Definition Rule", gtest must be compiled with exactly the same flags as your C++ code under test.

No good, then. Another option: install package and add_subdirectory(/usr/src/gtest).

jgvictores commented 6 years ago

Another option: install package and add_subdirectory(/usr/src/gtest)

Sounds okay. As long as we do an out-of-source build, we won't need the sudo for make.

PeterBowman commented 6 years ago

@jgvictores, @David-Estevez I followed the "package + vanilla CMake" solution: roboticslab-uc3m/kinematics-dynamics@f2ae4ed + Travis builds. What do you think? Some points to consider:

As a side note: perhaps tests would be a better choice than test for the directory name of unit tests? We already have programs and libraries (but: cmake/template). I recall seeing a top-level src somewhere. In fact, directory layout might be better covered in a new issue in best-practices.

Edit: since a new dependency would be required to build unit tests (googletest package, no longer local), should they be disabled by default?

PeterBowman commented 6 years ago

Note to self: Establish gtest version (SO).

David-Estevez commented 6 years ago

Edit: since a new dependency would be required to build unit tests (googletest package, no longer local), should they be disabled by default?

What about disabling them if googletest cannot be found? (as devices do in yarp-devices if dependency is not found)

PeterBowman commented 6 years ago

I'm not sure about this. You'll usually want to build tests in Travis and abort them if not successful, other scenarios are non-critical. Silently hiding an unsatisfied dependency or even just warning about it is perhaps a bit too risky in this case.

Edit: see last comments at #18.

PeterBowman commented 6 years ago

I kinda like it. @jgvictores, @David-Estevez what do you think?

As a side note: perhaps tests would be a better choice than test for the directory name of unit tests? We already have programs and libraries (but: cmake/template). I recall seeing a top-level src somewhere. In fact, directory layout might be better covered in a new issue in best-practices.

See #46.

Edit: since a new dependency would be required to build unit tests (googletest package, no longer local), should they be disabled by default?

You'll usually want to build tests in Travis and abort them if not successful, other scenarios are non-critical. Silently hiding an unsatisfied dependency or even just warning about it is perhaps a bit too risky in this case.

New issue?

What about disabling them if googletest cannot be found? (as devices do in yarp-devices if dependency is not found)

See #18.

jgvictores commented 6 years ago

IMHO, everything is fine. I'd even go for a solution like #18. Travis will fail with pretty obvious messages when attempting to run tests which have not been compiled if we ever forget to yamlYCM gtest.

PeterBowman commented 6 years ago

I'd even go for a solution like #18. Travis will fail with pretty obvious messages when attempting to run tests which have not been compiled if we ever forget to yamlYCM gtest.

Title updated, this is no longer a YCM issue according to the latest proposal.

I'm not sure about applying #18 here. We may forget to install libgtest-dev, but that should happen at most once. The question is: are unit tests valuable/relevant outside Travis CI? Should regular users care, and see annoying warnings about a missing package which would led them to dig into the installation guides?

PS sorry for "dizzying the partridge"!

jgvictores commented 6 years ago

Title updated, this is no longer a YCM issue according to the latest proposal.

Yes, I was about to do that, just couldn't think of an accurate description.

I'm not sure about applying #18 here. We may forget to install libgtest-dev, but that should happen at most once. The question is: are unit tests valuable/relevant outside Travis CI? Should regular users care, and see annoying warnings about a missing package which would led them to dig into the installation guides?

IMHO, the mechanism proposed at #18 only warns once, which isn't annoying. We are all developers and I think it's good for us to be informed if tests are not going to be compiled on our machine and why. Workflow: get tired of warnings -> install gtest -> have tests compiled which at least checks if we break an API.

PS sorry for "dizzying the partridge"!

Big LOL!

PeterBowman commented 6 years ago

Done at kinematics-dynamics, added install guide:

PeterBowman commented 6 years ago

These months xgnitive is going to be a bit crazy, I'd wait till IROS 2018 deadline March 1 2018, ok?

@jgvictores OK! All other tasks are done, I leave xgnitive up to you. Closing issue.

PeterBowman commented 6 years ago

XGNITIVE done at https://github.com/roboticslab-uc3m/xgnitive/compare/5d81fbc...06506e2.

jgvictores commented 6 years ago

Thanks a lot!!

PeterBowman commented 6 years ago

Note that googletest 1.8.0 forces installation of headers and generated binaries (https://github.com/google/googletest/commit/98d988deac06637364f6cd41c45c3db4a8a0b6bc). This behaviour may be suppressed on current googletest master (1.9.0?) with the INSTALL_GTEST CMake option (https://github.com/google/googletest/commit/0e8e0e07d6c4bc8c9cd6df5407452c12752ab45c), although it won't affect us since we don't process the root CMakeLists.txt where this option is defaulted to ON (FindGTestSources.cmake skips this directory and looks for googletest/CMakeLists.txt).

Affects artful (17.10) and bionic (18.04) Ubuntu distros (ref) as well as regular git clones (as recommended on Windows platforms). Things get a bit weird on the former: copy GTest headers into /usr/include/gtest as a result of installing libgtest-dev, process googletest's sources as part of another project (e.g. kinematics-dynamics), install it, and you'll get a copy of said headers at /usr/local/include/gtest.

PeterBowman commented 5 years ago

Alternatively, use FetchContent module from CMake 11. This allows to fetch external projects on configure time. Added in YCM 0.10 (not released yet).

PeterBowman commented 5 years ago

Note that googletest 1.8.0 forces installation of headers and generated binaries (google/googletest@98d988d). This behaviour may be suppressed on current googletest master (1.9.0?) with the INSTALL_GTEST CMake option (google/googletest@0e8e0e0), although it won't affect us since we don't process the root CMakeLists.txt where this option is defaulted to ON (FindGTestSources.cmake skips this directory and looks for googletest/CMakeLists.txt).

Things are getting weird on cosmic (18.10) and disco (19.04), the libgtest-dev package is no longer a mere placeholder for googletest (bionic, although it still depends on it) and now contains binaries along with gtest headers.

Alternatively, use FetchContent module from CMake 11. This allows to fetch external projects on configure time. Added in YCM 0.10 (not released yet).

This is currently broken, a fix is expected for YCM v0.10.2: https://github.com/robotology/ycm/pull/247.