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

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

Open
matthew-reynolds opened this issue Jan 28, 2019 · 0 comments
Open
Labels
enhancement New feature or request
Milestone

Comments

@matthew-reynolds
Copy link

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.

@dirk-thomas dirk-thomas added the enhancement New feature or request label Feb 3, 2020
@dirk-thomas dirk-thomas added this to the untargeted milestone Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants