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

fix tests in foxy/ubuntu-20.04 #749

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from
Draft

Conversation

koonpeng
Copy link
Collaborator

@koonpeng koonpeng commented Dec 8, 2020

I'm not sure if I'm the only one having this problem, the tests are failing for me in foxy/ubuntu-20.04. I actually saw in problem before in other projects and it is due to fastRTPS not correctly exported some cmake files. The simple workaround is to add AMENT_PREFIX_PATH to CMAKE_PREFIX_PATH.

I also try to simplify the compiling step by using colcon and sourcing the output instead of copying files around.

Edit: Actually all the previous build have been failing but it seems tests still passes

> rclnodejs@0.17.0 test /root/rclnodejs
> node ./scripts/compile_tests.js && node --expose-gc ./scripts/run_test.js && npm run dtslint

Starting >>> cpp_nodes

Starting >>> rclnodejs_test_msgs

--- stderr: cpp_nodes
/root/rclnodejs/test/cpp/add_two_ints_client.cpp: In function ‘example_interfaces::srv::AddTwoInts_Response_<std::allocator<void> >::SharedPtr send_request(rclcpp::Node::SharedPtr, rclcpp::Client<example_interfaces::srv::AddTwoInts>::SharedPtr, example_interfaces::srv::AddTwoInts_Request_<std::allocator<void> >::SharedPtr)’:
/root/rclnodejs/test/cpp/add_two_ints_client.cpp:48:23: error: ‘rclcpp::executor::FutureReturnCode’ has not been declared
   48 |     rclcpp::executor::FutureReturnCode::SUCCESS)
      |                       ^~~~~~~~~~~~~~~~
/root/rclnodejs/test/cpp/add_two_ints_client.cpp: In function ‘int main(int, char**)’:
/root/rclnodejs/test/cpp/add_two_ints_client.cpp:94:23: error: ‘rclcpp::executor::FutureReturnCode’ has not been declared
   94 |     rclcpp::executor::FutureReturnCode::SUCCESS)
      |                       ^~~~~~~~~~~~~~~~
/root/rclnodejs/test/cpp/add_two_ints_client.cpp: In function ‘example_interfaces::srv::AddTwoInts_Response_<std::allocator<void> >::SharedPtr send_request(rclcpp::Node::SharedPtr, rclcpp::Client<example_interfaces::srv::AddTwoInts>::SharedPtr, example_interfaces::srv::AddTwoInts_Request_<std::allocator<void> >::SharedPtr)’:
/root/rclnodejs/test/cpp/add_two_ints_client.cpp:54:1: warning: control reaches end of non-void function [-Wreturn-type]
   54 | }
      | ^
make[2]: *** [CMakeFiles/add_two_ints_client.dir/build.make:63: CMakeFiles/add_two_ints_client.dir/add_two_ints_client.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:84: CMakeFiles/add_two_ints_client.dir/all] Error 2
make: *** [Makefile:95: all] Error 2
---
Failed   <<< cpp_nodes [7.06s, exited with code 2]

@koonpeng koonpeng marked this pull request as draft December 8, 2020 05:20
@coveralls
Copy link

coveralls commented Dec 8, 2020

Coverage Status

Coverage remained the same at 91.268% when pulling c78abbd on build/fix-tests-foxy into 45d7bd0 on develop.

@koonpeng
Copy link
Collaborator Author

koonpeng commented Dec 8, 2020

The windows ci is using a ci build of ros2 which mistakenly exports local directories.

  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;C:/ci/ws/install/include"

This causes cpp_nodes to fail, I tried with a release build of ros2 and it works. The test still passes because it seems cpp_nodes is not used in windows tests. Should I use the ci build and exclude cpp_nodes from the build, or switch to a release build? The downside of the release build is that we need to update it when new ros version gets released.

@minggangw
Copy link
Member

I prefer we use the rolling-distribution for develop branch, meanwhile, stable release matches its corresponding brach of rclnodejs, e.g. foxy-fitzroy uses ROS 2 Foxy Fitzroy - Patch Release 3 for ci. Thus, we could track the latest changes and fix the problem before it's officially released on develop branch.

But the rolling-distribution (or nightly build) is not stable although it's called stable. I ever met similar issues before, and the problems often got fixed in the next package release. I think we could exclude cpp_nodes because those tests that use cpp_nodes are not running any more, or we could wait a few days until the next ROS2 package (https://ci.ros2.org/view/packaging/job/packaging_windows/lastSuccessfulBuild/artifact/ws/ros2-package-windows-AMD64.zip) is ready.

@koonpeng
Copy link
Collaborator Author

koonpeng commented Dec 8, 2020

I went back a few weeks and they all have the same error
https://ci.appveyor.com/project/minggangw/rclnodejs/builds/36303858

CMake Error in CMakeLists.txt:
  Imported target "rclcpp::rclcpp" includes non-existent path
    "C:/ci/ws/install/include"
  in its INTERFACE_INCLUDE_DIRECTORIES.  Possible reasons include:
  * The path was deleted, renamed, or moved to another location.
  * An install or uninstall procedure did not complete successfully.
  * The installation package was faulty and references files it does not
  provide.

maybe it's a unreported bug?

@koonpeng
Copy link
Collaborator Author

koonpeng commented Dec 8, 2020

made an issue ros2/ros2#1068

@minggangw
Copy link
Member

I suspect it was misconfigured, let's wait for the reply from the upstream.

@minggangw
Copy link
Member

I notice there is a new Window ROS2 package generated (https://ci.ros2.org/view/packaging/job/packaging_windows/1960/), so the problem may have been fixed.

@minggangw minggangw added this to In progress in v0.18.0 Release Dec 14, 2020
@koonpeng
Copy link
Collaborator Author

koonpeng commented Jan 7, 2021

It looks like the issue still isn't fixed 😞

@minggangw minggangw added this to In progress in v0.18.1 Release Jan 8, 2021
@minggangw minggangw removed this from In progress in v0.18.0 Release Jan 8, 2021
@minggangw minggangw added this to In progress in v0.19.0 Release Feb 10, 2021
@minggangw minggangw removed this from In progress in v0.18.1 Release Feb 10, 2021
@minggangw minggangw removed this from In progress in v0.19.0 Release May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants