ros-industrial / ros_industrial_cmake_boilerplate Goto Github PK

I tried to use the method in the readme to generate the Debian package, but the third line does not work. Is it the generation method has changed?

cd <workspace directory>
catkin build -DRICB_PACKAGE=ON
./src/ros_industrial_cmake_boilerplate/.run-cpack

After we have used this on a few projects and get it working, we should tag a 0.1.0 release so we can start pulling version tags instead of master or commits.

For reference, in progress ports are below.

1. Noether: ros-industrial/noether#93
2. OPW_Kinematics: Jmeyer1292/opw_kinematics#40

Is there a reason for not allowing - (minus sign) in package names? Source code here.

@gavanderhoorn I have started to go through the release process but I am not sure how to exclude the gtest package from the release process. Do you know how to do this? I thought about create a patch which removes it but not sure if that will work.

lcov appears to only be listed as a test_depend in the manifest:

this causes problems for downstream users if they depend on ros_industrial_cmake_boilerplate as part of their own build, as lcov will not be installed at that point.

I believe adding <build_depend>lcov</build_depend> would solve this, but haven't investigated everything closely enough to be absolutely sure.

I am getting multiple linking errors when building melodic on Ubuntu 18 with colcon, e.g.,
undefined reference to testing::ScopedTrace::~ScopedTrace()'orundefined reference to testing::ScopedTrace::PushTrace(char const*, int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)' in tf2_geometry_msgs.
Looks like a version conflict: gtest.h header provided by this package overrides default one, while the packages still link to the default gtest library built for each package individually:

$ grep -R PushTrace build/ | grep libgtest.so
Binary file build/reldebug/ros_industrial_cmake_boilerplate-googletest-build/googletest/libgtest.so matches
$ find ./build/ -iname "libgtest.so"
./build/reldebug/angles/gtest/googlemock/gtest/libgtest.so
./build/reldebug/tf2_geometry_msgs/gtest/googlemock/gtest/libgtest.so
./build/reldebug/libmavconn/gtest/googlemock/gtest/libgtest.so
...

gtest linking paths are configured somewhere in catkin guts, overriding them is a pain, and might need to be done for each package individually.

ctest has the option --output-on-failure that will print the output of a target if it fails. This is useful for reviewing logs to see why the test failed. I recommend adding it to the add_run_test_target macro in add_custom_target.

This description in the package.xml

<description>test; test</description>

gives this error

CMake Error at /home/matthew/code/external/devel/lib/cmake/ros_industrial_cmake_boilerplate/extract_package_metadata.cmake:57 (string):
  string sub-command STRIP requires two arguments.

Removing the semi colon fixes it

We've got a similar set of macros in several repos (tesseract, trajopt, opw_kinematics). Should we add it here to an interface target?

This is the macro I'm talking about.

// clang-format off
#if defined(__GNUC__) || defined(__clang__)

#define DEPRECATED(X) __attribute__((deprecated(X)))

#if defined(__clang__)
#define TESSERACT_COMMON_IGNORE_WARNINGS_PUSH				                                                                   \
  _Pragma("GCC diagnostic push") _Pragma("GCC diagnostic ignored \"-Wall\"")                                           \
     _Pragma("GCC diagnostic ignored \"-Wint-to-pointer-cast\"")		                                                   \
         _Pragma("GCC diagnostic ignored \"-Wunused-parameter\"")		                                                   \
             _Pragma("GCC diagnostic ignored \"-Winconsistent-missing-override\"")	                                   \
                 _Pragma("GCC diagnostic ignored \"-Wconversion\"")			                                               \
                     _Pragma("GCC diagnostic ignored \"-Wfloat-conversion\"")		                                       \
                         _Pragma("GCC diagnostic ignored \"-Wsign-conversion\"")
#else
#define TESSERACT_COMMON_IGNORE_WARNINGS_PUSH                                                                          \
  _Pragma("GCC diagnostic push") _Pragma("GCC diagnostic ignored \"-Wall\"")                                           \
      _Pragma("GCC diagnostic ignored \"-Wint-to-pointer-cast\"")                                                      \
          _Pragma("GCC diagnostic ignored \"-Wunused-parameter\"")                                                     \
              _Pragma("GCC diagnostic ignored \"-Wsuggest-override\"")                                                 \
                  _Pragma("GCC diagnostic ignored \"-Wconversion\"")                                                   \
                      _Pragma("GCC diagnostic ignored \"-Wfloat-conversion\"")                                         \
                          _Pragma("GCC diagnostic ignored \"-Wsign-conversion\"")
#endif

#define TESSERACT_COMMON_IGNORE_WARNINGS_POP _Pragma("GCC diagnostic pop")

#elif defined(_MSC_VER)
#define DEPRECATED(X) __declspec(deprecated(X))
#define TESSERACT_COMMON_IGNORE_WARNINGS_PUSH
#define TESSERACT_COMMON_IGNORE_WARNINGS_POP
#else
#pragma message("WARNING: You need to implement MACROS for this compiler")
#define DEPRECATED(X)
#define TESSERACT_COMMON_IGNORE_WARNINGS_PUSH
#define TESSERACT_COMMON_IGNORE_WARNINGS_POP
#endif

#define UNUSED(x) (void)(x)

// clang-format on

I just overhauled my MoveIt-and-associated workspace, which includes stomp_ros, which now depends on stomp, which depends on ros_industrial_boilerplate.
Now, the last of these bundles gtest and actually installs a gtest/ include folder in the standard workspace includes.
I would argue this is a bug already because gtest

• should be system-provided for linux in /usr/src in the first place
• should definitely not overshadow gtest for every package using the workspace - so includes should be installed to a different include path
• The package does not bundle gmock as well, so packages that include gmock will include the system installation of gmock, but the ros_industrial version of gtest

Because my system version of googletest is 1.10.0, this combination produces incompatibilities:

/usr/src/googletest/googlemock/include/gmock/internal/gmock-internal-utils.h:460: error: no template named 'IndexSequence'; did you mean 'std::index_sequence'?
In file included from moveit_task_constructor/core/test/test_cost_queue.cpp:6:
In file included from /usr/src/googletest/googlemock/include/gmock/gmock-matchers.h:57:
/usr/src/googletest/googlemock/include/gmock/internal/gmock-internal-utils.h:460:37: error: no template named 'IndexSequence'; did you mean 'std::index_sequence'?

I'm not sure which fix you prefer/deem feasible, but please address this @Levi-Armstrong .

Mentioned briefly in #26 (comment), but IMHO it's a bug that this repo includes a package called gtest. When building large workspaces from source checkouts (rather than GBP sources), this repo's version of gtest aliases the standard rosdep and makes itself a dependency of catkin:

https://github.com/ros/catkin/blob/ff31d451ce1c68d47bc058a4693aad6c0fb63a43/package.xml#L37-L38

The "real" gtest is of course here:

https://github.com/ros/rosdistro/blob/6a0459d84fba6e83f99a74e7959ce26ce829d26d/rosdep/base.yaml#L1696-L1709

This caused me to have to add a manual exclusion for an in-distro gtest to the nix-ros demo from ROSCon 2022:

https://github.com/clearpathrobotics/nix-ros-base/blob/fc02ac30d8ee20198c48b381501c7902e9c7a6f5/nix_generator/defaults.py#L15-L17

Some possible resolutions could be:

• Withdrawing the source entry from rosdistro for this repository.
• Moving gtest out of this repo to a separate one that is release-only in rosdistro.
• Renaming the package to gtest_vendor or ros_industrial_gtest, similar to how a number of ROS 2 packages work.

It looks like the following lines need to be outside the if statement.

I see the configure_package macro relies on a template file in the source package. The example templates I've seen are very simple with the same structure. Is it possible we could remove the need for a template for the standard base case and have this package generate it automatically?