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

remove rmw_localhost_only_t. #255

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

CihatAltiparmak
Copy link
Contributor

@CihatAltiparmak CihatAltiparmak commented Aug 2, 2024

Hello, maybe it's also in your plan, but i wanted to open PR about deprecated usage of rmw_localhost_only_t just in case you have overlooked this PR ( ros2/rcl#1169 ) in rcl. I have discovered this one while running ros-industrial ci in my repo and some errors happen like below in the ROS_REPO=testing case.

       Finished release [optimized] target(s) in 7m 07s
  ---
  Finished <<< zenoh_c_vendor [7min 14s]
  Starting >>> rmw_zenoh_cpp
  --- stderr: rmw_zenoh_cpp
  /root/upstream_ws/src/rmw_zenoh/rmw_zenoh_cpp/src/rmw_init_options.cpp: In function ‘rmw_ret_t rmw_init_options_init(rmw_init_options_t*, rcutils_allocator_t)’:
  /root/upstream_ws/src/rmw_zenoh/rmw_zenoh_cpp/src/rmw_init_options.cpp:50:17: error: ‘rmw_init_options_t’ {aka ‘struct rmw_init_options_s’} has no member named ‘localhost_only’
     50 |   init_options->localhost_only = RMW_LOCALHOST_ONLY_DEFAULT;
        |                 ^~~~~~~~~~~~~~
  /root/upstream_ws/src/rmw_zenoh/rmw_zenoh_cpp/src/rmw_init_options.cpp:50:34: error: ‘RMW_LOCALHOST_ONLY_DEFAULT’ was not declared in this scope
     50 |   init_options->localhost_only = RMW_LOCALHOST_ONLY_DEFAULT;
        |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
  /root/upstream_ws/src/rmw_zenoh/rmw_zenoh_cpp/src/rmw_init_options.cpp: In function ‘rmw_ret_t rmw_init_options_copy(const rmw_init_options_t*, rmw_init_options_t*)’:
  /root/upstream_ws/src/rmw_zenoh/rmw_zenoh_cpp/src/rmw_init_options.cpp:92:7: error: ‘rmw_init_options_t’ {aka ‘struct rmw_init_options_s’} has no member named ‘localhost_only’
     92 |   tmp.localhost_only = src->localhost_only;
        |       ^~~~~~~~~~~~~~
  /root/upstream_ws/src/rmw_zenoh/rmw_zenoh_cpp/src/rmw_init_options.cpp:92:29: error: ‘const rmw_init_options_t’ {aka ‘const struct rmw_init_options_s’} has no member named ‘localhost_only’
     92 |   tmp.localhost_only = src->localhost_only;
        |                             ^~~~~~~~~~~~~~
  gmake[2]: *** [CMakeFiles/rmw_zenoh_cpp.dir/build.make:384: CMakeFiles/rmw_zenoh_cpp.dir/src/rmw_init_options.cpp.o] Error 1
  gmake[2]: *** Waiting for unfinished jobs....
  gmake[1]: *** [CMakeFiles/Makefile2:139: CMakeFiles/rmw_zenoh_cpp.dir/all] Error 2
  gmake: *** [Makefile:146: all] Error 2
  ---
  Failed   <<< rmw_zenoh_cpp [14.0s, exited with code 2]
  
  Summary: 14 packages finished [7min 29s]
    1 package failed: rmw_zenoh_cpp
    2 packages had stderr output: rmw_zenoh_cpp zenoh_c_vendor
'build_upstream_workspace' returned with code '2' after 7 min 29 sec

@Yadunund
Copy link
Member

Yadunund commented Aug 5, 2024

Thanks for the PR. One question I have is whether we retain support for Iron + Jazzy users who may continue to specify ROS_LOCALHOST_ONLY. If so, we would need to create iron and jazzy branches in rmw_zenoh before we merge this in. @clalancette what do you think?

@Timple
Copy link
Contributor

Timple commented Aug 5, 2024

Just an interested bystander here: I am following this repository out of curiosity.

For such a small delta, I would advise some guard around the statement instead of branching off. Otherwise a huge backport effort will probably be undertaken. With tooling like mergify bots etc. Making a lot of PR creation noise. Making repositories often un-watchable.

Of course this should not be your main concern but also take into account the maintenance effort this brings in. I think a (rather ugly) guard in the code is less of an effort.

@MichaelOrlov
Copy link

@Yadunund Friendly ping here to follow up.

@clalancette
Copy link
Collaborator

@Yadunund , @ahcorde , and I discussed this change.

In the long run (end of this year), we are going to have to branch off, due to releasing into distributions. But until we get there, it is convenient to use the same branch for everything. Thus, for now, I think should strive in this PR to maintain compatibility with Iron, Jazzy, and Rolling.

The simple way to do that would be to just remove all references to the localhost_only structure member. That works because we actually don't support the localhost_only parameter here in rmw_zenoh_cpp anyway. However, that is only half the story. If we just remove all references, then on Iron and Jazzy, localhost_only might be in an undefined state.

Thus, in addition to removing those references, we should also make sure that the structure is initialized. For rmw_init_options_init, we can accomplish that by calling memset on the structure before doing any work. For rmw_init_options_copy, we can accomplish that by calling memcpy from the src to the tmp structure before doing any additional work there.

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@CihatAltiparmak
Copy link
Contributor Author

CihatAltiparmak commented Aug 22, 2024

Friendly feedback @clalancette @Yadunund @ahcorde , this problem has started in main repo as well for rolling.

Github action log : https://github.com/CihatAltiparmak/moveit_middleware_benchmark/actions/runs/10504255092/job/29099303697#step:3:1285

@clalancette clalancette merged commit ea41516 into ros2:rolling Aug 22, 2024
8 checks passed
imstevenpmwork pushed a commit to ZettaScaleLabs/rmw_zenoh that referenced this pull request Aug 27, 2024
* remove rmw_localhost_only_t.

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
imstevenpmwork pushed a commit to ZettaScaleLabs/rmw_zenoh that referenced this pull request Aug 27, 2024
* remove rmw_localhost_only_t.

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
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.

6 participants