Change to force a rebuild of the pollset on flag changes #1393
Conversation
This reverts commit 5e2b38e. This change may be considered for backporting to Lunar at a later time.
guillaumeautran
changed the title
| @@ -119,6 +119,8 @@ void close_socket_watcher(int fd) { | |||
| void add_socket_to_watcher(int epfd, int fd) { | |||
| #if defined(HAVE_EPOLL) | |||
| struct epoll_event ev; | |||
| bzero(&ev, sizeof(ev)); | |||
There was a problem hiding this comment.
Fixing valgrind warnings.
| @@ -147,6 +149,8 @@ void del_socket_from_watcher(int epfd, int fd) { | |||
| void set_events_on_socket(int epfd, int fd, int events) { | |||
| #if defined(HAVE_EPOLL) | |||
| struct epoll_event ev; | |||
| bzero(&ev, sizeof(ev)); | |||
There was a problem hiding this comment.
Fixing valgrind warnings.
| @@ -1,6 +1,6 @@ | |||
| catkin_add_gtest(${PROJECT_NAME}-test_version test_version.cpp) | |||
| if(TARGET ${PROJECT_NAME}-test_version) | |||
| target_link_libraries(${PROJECT_NAME}-test_version) | |||
| target_link_libraries(${PROJECT_NAME}-test_version ${catkin_LIBRARIES}) | |||
There was a problem hiding this comment.
I could not build and run the unit tests without the following changes.
There was a problem hiding this comment.
Did you end up just running the tests on Linux but with a build where HAVE_EPOLL was forced to 0?
There was a problem hiding this comment.
That's correct yes.
There was a problem hiding this comment.
And prior to this fix the tests failed to complete successfully?
There was a problem hiding this comment.
Yes, sorry, I should have mentioned that.
Also, I am looking at the current build failure.
There was a problem hiding this comment.
My impression is that that test has been flakey for a while— it definitely has a timing sensitivity and does sometimes pass on the buildfarm. However, if you have further insights on it, that would be great.
|
I cannot figure out why the test fails on the build farm. They obviously pass just fine on my laptop which makes it hard to troubleshoot... |
|
LGTM, and has the thumbs-up from @mpflanzer in #1357 (comment). IMO this should merge now and we can treat the test regression as separate since it predates this change. |
This change fixes issues seen in #1357
Goes with #1281