Conversation
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Borrowing actually doesn't make much sense; we have to hold onto it until we are done using it. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
We need to hold onto references until after we have finished using them. This change rewrites the last two users of "borrowString" to properly manage references, and also checks for errors. It then removes borrowString completely. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
|
CI:
|
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
|
All right, the latest fix should fix the flakiness on Windows. Re-running CI on everything to check, but I think this is ready for review now:
|
| { | ||
| // Simulate doing some work in here; otherwise, we can complete the goal | ||
| // before the rclcpp_action interface ever has time to do any work. | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); |
There was a problem hiding this comment.
Do you think this is a bug in rclcpp_action (ie. should it be reasonable to succeed/abort immediately)?
There was a problem hiding this comment.
Yeah, it's a good question. I've gone back and forth on it in my head. On the one hand, I think there is an argument that in a "real" action, there is always some delay between the action client send the goal and the action server getting it and then acting on it. On the other hand, I can imagine a scenario where the client and server are composed together, and there is some kind of "fast-path" on the server that allows it to succeed immediately.
So I could go either way here; what's your thinking?
There was a problem hiding this comment.
The composition scenario also came to my mind. I'll take a closer look at the rclcpp_action implementation to see what could be done.
| find_package(python_cmake_module REQUIRED) | ||
| find_package(PythonExtra REQUIRED) | ||
|
|
||
| set(_PYTHON_EXECUTABLE "${PYTHON_EXECUTABLE}") |
There was a problem hiding this comment.
We may want to capture/document this bit of logic somewhere, as it will likely come up anytime pytest is used on Windows Debug.
There was a problem hiding this comment.
Yeah, that's a good idea. Are you thinking of a cmake function/macro, or documentation? In either case, any thoughts on where we would put it?
There was a problem hiding this comment.
Not sure. We have a small section on Windows debug in the installation guide, but I don't think that it fits here: https://github.com/ros2/ros2_documentation/blob/master/source/Installation/Eloquent/Windows-Development-Setup.rst#extra-stuff-for-debug-mode
Can it be fixed in ament_cmake_pytest? https://github.com/ament/ament_cmake/blob/master/ament_cmake_pytest/cmake/ament_add_pytest_test.cmake#L61
The individual commits have more details, but essentially this PR fixes up tf2_py so that it works under Windows Debug. There are two main things here:
python_d.exewhen in debug mode.This should fix ros2/build_farmer#240 . CI upcoming.