ros / rosconsole

17 stars 61 forks source link

Non-log4cxx backends should disable testing and inform the user #27

Open matthew-reynolds opened 5 years ago

matthew-reynolds commented 5 years ago

If the backend is unspecified, the backend selection attemps to find log4cxx. If it fails, it looks for glog, and if that fails finally settles with print. If using glog or print implicitly, this means that LOG4CXX_LIBRARIES will be set to NOTFOUND when find_package(Log4cxx QUIET) fails.

Since #13, utest and thread_test link LOG4CXX_LIBRARIES (Which makes sense, they use log4cxx). However, this means that when building rosconsole and implicitly using glog or print, LOG4CXX_LIBRARIES is set to NOTFOUND and then is used when linking utest and thread_test, causing a CMake error:

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
LOG4CXX_LIBRARIES
    linked by target "rosconsole-thread_test" in directory [...]
    linked by target "rosconsole-utest" in directory [...]

Note that this only occurs when implicitly using glog or print, because explicitly specifying -DROSCONSOLE_BACKEND=<glog|print> skips over the failing find_package(Log4cxx). Easy fixes for this issue would be unsetting LOG4CXX_LIBRARIES if it isn't found and ROSCONSOLE_BACKEND is not log4cxx, or just setting CATKIN_ENABLE_TESTING to 0 when using glog or print.

However, this kind of opens up a bigger issue. There should be some cleaner mechanism to inform the user that tests are unable to be run when not using log4cxx. If we just overwrite CATKIN_ENABLE_TESTING to 0, running tests will fails saying that there are no tests. Not very informative. Similarly, if we just silence the NOTFOUND error by unsetting LOG4CXX_LIBRARIES, then the tests will all fail due to the log4cxx not existing.

I'm not sure what the best mechanism is to inform the user that tests are unavailable when they try to use glog or print. We could just use add:

message(WARNING "Tests are unavailable when using glog or print backends, please use log4cxx to run tests")
set(CATKIN_ENABLE_TESTING 0)

when not using log4cxx, but this feels insufficient. Is there a way to grab onto the --catkin_make_args run_tests to print a full, informative fatal error message?

The first issue is easy to fix, but I would like some input on the second before I open a PR.