mjcarroll
commented
4 years ago With renaming those headers, this will definitely have to land in foxy.
Is there any way to do a deprecation cycle on the current header locations?
Closed Martin-Idel closed 4 years ago
mjcarroll
commented
4 years ago With renaming those headers, this will definitely have to land in foxy.
Is there any way to do a deprecation cycle on the current header locations?
Martin-Idel
commented
4 years ago I would have assumed this landed in foxy - so for me that'd be okay ;-) .
However, I can understand if you also want to make it available in eloquent or earlier, so here is what I see we could do:
.h files. I don't particularly like this, but it's possible. .hpp and add a new file with .h which just includes the .hpp file and adds a deprecation warning. I added a commit to show how I'd do this. If you want, I can quickly do this for all headers. The build still compiles and is error free. Any build I can see which uses the .hfile will then still compile but issue a "note" on the command line
I'm not exactly sure that this is completely portable though but at least there is no warning from -Wcpp.CI failure is unrelated, I can't reproduce this and I also didn't really change image_transport in this last commit and the removal of the filesystem_helper fixes this again.
Martin-Idel
commented
4 years ago I also saw that the windows build fails due to problems with the filesystem helper.
My solution would be to delete it and use rcpputils/filesystem_helper. From a library perspective, this should still make it possible to backport, if rcpputils still exists.
mjcarroll
commented
4 years ago My solution would be to delete it and use rcpputils/filesystem_helper.
I agree with that decision, and that's available in previous versions as well.
mjcarroll
commented
4 years ago Martin-Idel
commented
4 years ago Of course there had to be some MSVC warnings... Hopefully fixed them all.
Regarding the header switch/deprecation, do you want me to also implement it for the image_transport subproject? If yes, would you like a followup-PR or should I do it here?
mjcarroll
commented
4 years ago Of course there had to be some MSVC warnings...
Always ;)
If yes, would you like a followup-PR or should I do it here?
Yes, and here is fine, since we need at least one more CI run, and currently waiting to bring buildfarm back until the outage is over, so we have a bit of time.
Martin-Idel
commented
4 years ago So I added the .h deprecated headers to image_transport - except for the tutorials and tests, where I only renamed the headers to .hpp.
Since I had to adapt the header guards anyways, I also updated them to the style preferred by cpplint. Some includes got moved around by my IDE in the process of renaming files.
In order to make everything still somewhat reviewable I left the commits separated (the PR overview is completely messed up). Since I had to rebase on master, the two new commits are the last two of the bunch.
To test it (at least on Linux), I build rviz_default_plugins with this PR https://github.com/ros2/rviz/pull/523 still containing the .h headers.
colcon build runs without errors but as expected I get stderr output:
[Processing: rviz_default_plugins]
--- stderr: rviz_default_plugins
In file included from /home/martin/gitRepos/ros/rviz_ws/src/rviz_default_plugins/include/rviz_default_plugins/displays/image/image_transport_display.hpp:40:0,
from /home/martin/gitRepos/ros/rviz_ws/src/rviz_default_plugins/include/rviz_default_plugins/displays/camera/camera_display.hpp:51,
from /home/martin/gitRepos/ros/rviz_ws/src/rviz_default_plugins/src/rviz_default_plugins/displays/camera/camera_display.cpp:32:
/home/martin/gitRepos/ros/image_transport/install/include/image_transport/image_transport.h:41:89: note: #pragma message: Warning: This header is deprecated. Use 'image_transport.hpp' instead
#pragma message ("Warning: This header is deprecated. Use 'image_transport.hpp' instead")So it's hard to ignore but not a compiler warning (because I didn't find a portable way that isn't compiler specific)
mjcarroll
commented
4 years ago Martin-Idel
commented
4 years ago Argh... I did another pass at fixing MSVC warnings - I grepped for all uint32_t and found a number of further cases which originate from the node signature using a size_t (all transferred to size_t).
Regarding the test failures, this might be related to my replacement of getenv, at least the old implementation is wrong: It leaked a char * pointer and there was also UB if the environment variable was empty.
Trouble is, I don't currently have a local Windows installation to test this, which would make turnaround time shorter.
EDIT: But it turns out I still have CI access, so let's try Windows again (hopefully didn't mess up the parameters):
Martin-Idel
commented
4 years ago Actually, the test failures are weird - they are only concerned with image_transport, where I didn't change the logic at all and they seem to also change from run to run (filesystem helper and getenv changes are for one of the other packages).
I guess I'll have to actually debug this on Windows, which means I have to get my build working again, which will probably take a couple of days.
Martin-Idel
commented
4 years ago So the test failures seem to already be present on master (at least on my machine). I'm trying to get a debug build working on Windows, but that's complicated...
The errors are weird - it seems to me as if some environment variables were wrong, but they aren't. Do you happen to have a good idea @mjcarroll. Maybe that could accelerate things...
mjcarroll
commented
4 years ago @Martin-Idel thanks for diving in, I'll try to take a look, but may be slightly delayed due to getting my Windows VM up-to-date.
The failing to find symbol warnings are quite suspicious, though. It may be that new symbols have been added without setting the visibility correctly?
Martin-Idel
commented
4 years ago @mjcarroll No, this will have nothing to do with visibility - because the symbols we are talking about are sensor_msgs symbols and if those are missing, the problem will be much more widespread.
I tried to do some more digging - but I can't get a debug build to work nor attach symbol files to everything I need and therefore I can't evaluate any further.
Here is what I can see: The following seems to work perfectly (this is also basically the test_subscriber - as test fixture take the one e.g. from the MessagePassingTest):
TEST_F(SomeTest, test1)
{
std::function<void(const sensor_msgs::msg::Image::ConstSharedPtr & msg)> imageCallback =
[](const auto & msg) {(void)msg;};
auto sub = image_transport::create_subscription(node_.get(), "camera/image", imageCallback, "raw");
rclcpp::executors::SingleThreadedExecutor executor;
test_rclcpp::wait_for_subscriber(node_, sub.getTopic());
executor.spin_node_some(node_);
}You can also add new subscribers, wait some more, spin some more, nothing seems to be a problem. Similarly, the following works perfectly on my machine:
TEST_F(SomeTest, test2)
{
std::function<void(const sensor_msgs::msg::Image::ConstSharedPtr & msg)> imageCallback =
[](const auto & msg) {(void)msg;};
auto pub = image_transport::create_publisher(node_.get(), "camera/image");
pub.publish(generate_random_image());
executor.spin_node_some(node_);
}You can also add some publishers, wait, publish again, spin again. However, if you combine this, it doesn't work:
TEST_F(SomeTest, test2)
{
std::function<void(const sensor_msgs::msg::Image::ConstSharedPtr & msg)> imageCallback =
[](const auto & msg) {(void)msg;};
auto pub = image_transport::create_publisher(node_.get(), "camera/image");
auto sub = image_transport::create_subscription(node_.get(), "camera/image", imageCallback, "raw");
rclcpp::executors::SingleThreadedExecutor executor;
test_rclcpp::wait_for_subscriber(node_, sub.getTopic());
executor.spin_node_some(node_);
}This breaks deterministically on my machine. This tells me that this is somehow a tricky problem with the image publishers/subscribers.
Since it's limited to Windows the first idea would be undefined behaviour. I ran ubsan on Linux and it does find something in the SubscriberOptions within rclcpp but I have no idea whether this is related. I also ran asan which found a lot of leakage, all within pluginlib (registration of plugins, I think this is known) as well as within rmw_fastrtps. So nothing helping here. Very unlucky.
Martin-Idel
commented
4 years ago @mjcarroll I got my debug build to work, but I still can't pinpoint the problem and now I'm out of ideas. Maybe the below analysis makes sense to you?
So what the stacktrace is telling me that indeed, the subscription cannot be initialized. In fact, the problem seems to be (as the error messages says) when trying to get the typesupport (and then a second error gets thrown on deletion of the subscription we are about to create after the error is thrown) - but I don't get why.
I tried to see differences between the subscriber test (which passes and correctly constructs the subscription) and the message passing test, which doesn't.
Here is what I see with the message passing test:
0x00007ffc4aa092a0 {sensor_msgs__rosidl_typesupport_cpp.dll!rosidl_message_type_support_t sensor_msgs::msg::rosidl_typesupport_cpp::Image_message_type_support_handle} {...} but we don't get it. get_message_typesupport_handle tries to search for the symbols in a bunch of libraries it stores in some map. Those entries look good (containing stuff concerning sensor_msgs).Here is what I see with the subscription test:
0x00007ffc4aa092a0 {sensor_msgs__rosidl_typesupport_cpp.dll!rosidl_message_type_support_t sensor_msgs::msg::rosidl_typesupport_cpp::Image_message_type_support_handle} {...} but we don't get it. type_support_dispatch.hpp where the type_support_mmap_t map name map seems to contain the exact same entries as the assembly parts the debugger identifies as map in the message passing test. But I can't really debug here any further without running into assembly, too.So the difference is that in one instance we get the correct typesupport handle deep down in fastrtps and in the other we don't. Superficially though, everything seems to be the same. But that's where I'm stuck. Maybe there is indeed some undefined behaviour somewhere? Maybe the underlying objects get destroyed although they shouldn't (a superficial glance doesn't tell me anything).
Finally, the stack traces in that particular moment are identical (above the test stack trace). It's also not how the tests are set up (includes, environment, etc.). If I just change the content of the TEST_F macro with the corresponding other content, the same behaviour occurs.
So I'm back with thinking it might be some local that gets destroyed although it shouldn't or some undefined behaviour somewhere.
(Here is the subscriber stack trace, everything above the last entry reading image_transport-subscriber.exe is the same in both tests at the moment where getting the typesupport fails:
dirk-thomas
Rename certain files to hpp because uncrustify wants to completely change indentation and cpplint is unhappy about this.
This should hopefully fix all errors in CI.
If you want, I can also rename all headers to hpp (also in image_transport) and enable cppcheck. I didn't do it immediately to make the changes as small as possible.