search

eProsima / foonathan_memory_vendor

foonathan/memory vendor package for Fast DDS. Looking for commercial support? Contact info@eprosima.com
Apache License 2.0
23 stars 47 forks source link

Add dependency required to set BUILD_TESTING env var #20

Closed Blast545 closed 4 years ago

Blast545 commented 4 years ago

This is a fix required for #19 to work completely. The BUILD_TESTING variable was not set, and the code calling the linters was not entered. Didn't note this behavior before because CMake saved this variable on the build folder of the package, and was working. However, in a clean environment, this addition is necessary to actually run the tests.

Added optional dependency for ament_cmake_test.

Please check this out @MiguelCompany

Signed-off-by: Jorge J. Perez jjperez@ekumenlabs.com

MiguelCompany commented 4 years ago

Bringing in @nuclearsandwich, who is also a maintainer of this repo and has more experience with ament packages than me.

Blast545 commented 4 years ago

More info to reproduce the issue for this fix. Delete previously generated build info for the package, and later run the workflow for testing the package:

rm -rf build/foonathan_memory_vendor/
colcon build --symlink-install --cmake-force-configure --packages-up-to foonathan_memory_vendor
colcon test --packages-select foonathan_memory_vendor

After running these commands, and inspecting log/latest_test/ it can be seen that no test was run on this package. Adding the missing package addresses this.

nuclearsandwich commented 4 years ago

Thanks @Blast545. Reviewing is in my TODO queue. I trust that the change addresses the issue. But my understanding is that the BUILD_TESTING variable is a common part of CMake/CTest so while this may be the fix I'm not sure if the "why" behind it is accurate.

Blast545 commented 4 years ago

@nuclearsandwich Yes, you are right there's something weird about it... I think it might be related to the fact that this is a pure Cmake package, and not an ament one.

According to colcon discussion the environment testing variable should be set by default. However, when I was testing this I realized that including CTest (as described here) in the CMakelists file, also enabled the needed flag for testing. I imagine colcon does not enable testing by default on pure CMake packages.

nuclearsandwich commented 4 years ago

I've force pushed a rebase onto the latest master for this branch.

nuclearsandwich commented 4 years ago

ROS 2 CI building up to Fast-RTPS to make sure this didn't somehow break the build and testing foonathan_memory_vendor.

nuclearsandwich commented 4 years ago 
* macOS [![Build Status](https://camo.githubusercontent.com/2ff1cb1ea2aed1bc553cb861173e254b51d19fe0/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f6f7378266275696c643d38353538)](http://ci.ros2.org/job/ci_osx/8558/)

The warnings in this job weren't present in last night's nightlies. Running Build Status to see if they're present on master or if my modified repos file missed something.

nuclearsandwich commented 4 years ago

The warnings in this job weren't present in last night's nightlies. Running Build Status to see if they're present on master or if my modified repos file missed something.

These warnings are showing up in all jobs run on our mini3 CI host so whether they're new or spurious they're not introduced by this change. I'll go ahead and merge.