hbanzhaf / steering_functions

Apache License 2.0
229 stars 94 forks source link

Fix catkin failed build when testing is not enabled #13

Closed MCFurry closed 3 years ago

MCFurry commented 3 years ago

On a clean catkin_build I get thrown this error:

Errors     << steering_functions:check /home/mcfurry/ros/melodic/system/logs/steering_functions/build.check.000.log                                                                                               
CMake Error at /opt/ros/melodic/share/catkin/cmake/test/tests.cmake:18 (message):
  () is not available when tests are not enabled.  The CMake code should only
  use it inside a conditional block which checks that testing is enabled:

  if(CATKIN_ENABLE_TESTING)

    (...)

  endif()

Call Stack (most recent call first):
  CMakeLists.txt:207 (catkin_add_gtest)

make: *** [cmake_check_build_system] Error 1

This fixes this.

hbanzhaf commented 3 years ago

I never experienced that error before. How about you, @Timple?

Timple commented 3 years ago

The error seemed self-explanatory. But interestingly it doesn't reproduce in a default setting. Tested using Dockerfile below:

FROM ros:noetic
RUN apt-get update -qq && apt-get install -qqy python3-catkin-tools python3-osrf-pycommon git
RUN git clone https://github.com/hbanzhaf/steering_functions src/steering_functions

# Install dependencies used by packages
RUN rosdep update && rosdep install -y -i --from-paths src

# Compile ROS package
RUN bash -c 'source /opt/ros/*/setup.bash && catkin build'

But this layout is a-typical for a ROS package (see catkin_lint output). So probably doesn't hurt to conform to the standard to prevent issues in edge-cases.

MCFurry commented 3 years ago

The catkin build command by default enables testing, but in several usecases testing is not enabled. Hence it reproduces with:

FROM ros:melodic
RUN apt-get update -qq && apt-get install -qqy python-catkin-tools git
RUN git clone https://github.com/hbanzhaf/steering_functions src/steering_functions

# Install dependencies used by packages
RUN rosdep update && rosdep install -y -i --from-paths src

# Compile ROS package
RUN bash -c 'source /opt/ros/melodic/setup.bash && catkin build -DCATKIN_ENABLE_TESTING=off'
Timple commented 3 years ago

If the default is ON, it's probably safe to merge this then. No regressions for current users (default ON). And for users who have testing OFF, it failed anyway to compile.