[master] visibility_control & portable installation.

[seanyen]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#1634
Primary OS tested on	Windows
Robotic platform tested on	turtlebot on Gazebo Simulation

---

Description of contribution in a few bullet points

• Added visibility control for properly building shared library on Windows. https://index.ros.org/doc/ros2/Tutorials/Ament-CMake-Documentation/#building-libraries-on-windows

• Corrected the executable and libraries installation location for better portability support. https://index.ros.org/doc/ros2/Tutorials/Ament-CMake-Documentation/#building-a-library

• The bottom line is that this change should not break other platform builds.

Description of documentation updates required from your changes

• None.

---

Future work that may be required in bullet points

• More coming changes are staged at this branch: https://github.com/ms-iot/navigation2/tree/seanyen/windows Let me know how I can break them down to review-able changes.

[seanyen]

@SteveMacenski Let me know logistically what branch I should target and anything I should know. This change is probably the one touching the most of the sources for Windows port, so I want to tackle this first. :)

[SteveMacenski]

@seanyen you should retarget this to master, we don't accept PRs directly to distribution branches. It'll trickle into Foxy and cherry picked to eloquent.

In terms of this PR, I'll admit I've never done software development in windows so I really don't understand much that's going on here other than "mhm, sure, looks like things I've seen before in other projects".

What's this visibility control thing? How should I plan on dealing with this in the future? There seems to be a bunch of redundancy in the visibility control files for the packages. Can this be templated / generalized and added to the nav2_common so that we don't need to make some special file for each package?

[SteveMacenski]

libraries should go in bin?

[seanyen]

Yes, for a shared library (.DLL files), in general it should go to the bin folder for the install space (which is specified in RUNTIME DESTINATION), and the corresponding imported library (.LIB files) goes to lib folder (which is specified in ARCHIVE DESTINATION). On Windows, the symbols information is stored in the imported lib and the MSVC linker is linking against it instead of the shared lib.

REF: https://cmake.org/cmake/help/latest/command/install.html

[seanyen]

What's this visibility control thing? How should I plan on dealing with this in the future?

Here is a comprehensive explanation for Linux developer :)
https://gcc.gnu.org/wiki/Visibility

From my perspective, the GCC and MSVC made a opposing default decision on how to decide what symbols to be visible of a shared library for the linker linkage. From the guide, for GCC, one can specify -fvisibility=hidden to the toolchain to enforce the similar policy that MSVC. Perhaps that is something we can do, so when someone adds an new interface, CI can catch the visibility control earlier.

There seems to be a bunch of redundancy in the visibility control files for the packages. Can this be templated / generalized and added to the nav2_common so that we don't need to make some special file for each package?

There are many discussions (ament/ament_cmake#201 and ament/ament_cmake#164) on how we can do about it. Some methods might make it less manual, but the one adopted in the upstream ROS 2 packages is this approach. However I think it is still up to the package maintainer to decide what approach to use.

[seanyen]

@SteveMacenski Just want to quick check, I saw some test failures on and off from the CI for The test did not generate a result file. and are they flaky tests? Just want to make sure it is not a regression from this change.

[SteveMacenski]

@seanyen, yes, those are just 2 flaky tests, #1585. I haven't had the cycles to fix them. No need to worry about it.

Those visibility files appear to have some generic structure, can we add something to nav2_common to generate that for us?

I'm slightly concerned about maintainability and consistency here. Mostly because I'm not a windows developer and adding this stuff here, right now, does enable it, but I'm in future updates / new packages, I'm not going to be well suited to ensuring consistent compatibility. So I'd like for us to come up with some strategy that this is automatically generated or some set of finite and easy to follow rules about what needs to be done to support this. Or if checking in on occasion and fixing up things to work on windows is something you and your team can do.

[SteveMacenski]

@seanyen just following up here

[seanyen]

@SteveMacenski Sorry I didn't get back to you earlier. I ack'ed your concern and I agreed that we should come up with some more sustainable way for the visibility control thing. I will find some time next week to iterate on what I can do by adding something in nav2_common.

[SteveMacenski]

Thanks. If that works out, it would be nice to actually just have a generic tools available to generate this stuff, or have CI generate this stuff and include it in pushes to master.

Since Windows is the only OS that seems to need this, its hard for me as a Linux user to really appreciate or debug this. There are (currently) so few windows power-users involved to really help maintain to keep this stuff up and running. I'm all for supporting windows, but I think there needs to be some tooling made available from Microsoft to help us out as maintainers, in the form of automated tooling and testing examples to flag these issues in CI. Since I don't own a Windows or Mac, both of those I rely on tooling or power-users of those OS's to make sure they keep compatible.

[SteveMacenski]

Hi, just following up on this quickly a couple of weeks later.

[seanyen]

@SteveMacenski Sorry for the overdue on the response. I decided to start a new pull request here: #1704. In the new pull request, it is using CMake built-in features to take care of the visibility control and it should largely minimize the maintainer effort keeping the visibility up-to-dated, and in the new pull request, I include all the initial required Windows changes.

[SteveMacenski]

Amazing. Thanks for the attention- I’ll review tomorrow.