Add visibility control to all packages

[Levi-Armstrong]

This makes all symbols hidden as default and uses visibility control to expose symbols to have the same behavior building on Linux as it is on Windows.

[codecov]

Codecov Report

Merging #405 into feature/CommandLanguage will decrease coverage by 0.03%.
The diff coverage is 84.68%.

@@                     Coverage Diff                     @@
##           feature/CommandLanguage     #405      +/-   ##
===========================================================
- Coverage                    75.46%   75.43%   -0.04%     
===========================================================
  Files                          204      204              
  Lines                        12204    12204              
===========================================================
- Hits                          9210     9206       -4     
- Misses                        2994     2998       +4     

Impacted Files	Coverage Δ
...sseract_collision/bullet/bullet_cast_bvh_manager.h	100.00% <ø> (ø)
...ract_collision/bullet/bullet_cast_simple_manager.h	100.00% <ø> (ø)
...act_collision/bullet/bullet_discrete_bvh_manager.h	100.00% <ø> (ø)
..._collision/bullet/bullet_discrete_simple_manager.h	100.00% <ø> (ø)
...seract_collision/core/continuous_contact_manager.h	100.00% <ø> (ø)
...esseract_collision/core/discrete_contact_manager.h	100.00% <ø> (ø)
...de/tesseract_collision/fcl/fcl_discrete_managers.h	100.00% <ø> (ø)
...sseract_common/include/tesseract_common/resource.h	0.00% <0.00%> (ø)
...t/include/tesseract_environment/core/environment.h	76.78% <ø> (ø)
.../include/tesseract_environment/core/state_solver.h	100.00% <ø> (ø)
... and 108 more

[Levi-Armstrong]

@colin-lewis-19 and @marrts Do either of you have time to review this? This does not contain changes to code just setting symbol visibility.

[mpowelson]

@Levi-Armstrong Is this how packages like PCL handle it?

[Levi-Armstrong]

I am not sure how PCL does it but it should be similar. There is a global way to expose all symbols which may be what they are doing, but this approach is the preferred way. This was copied form rclcpp, which i copied from GCC website.

[mpowelson]

I am not sure how PCL does it but it should be similar. There is a global way to expose all symbols which may be what they are doing, but this approach is the preferred way. This was copied form rclcpp, which i copied from GCC website.

Do IDEs get confused? This isn't going to make code development painful is it?

Edit: Still reviewing. I guess it took this comment as meaning I was done with the review.

[mpowelson]

Is this a new macro? I don't remember it.

[Levi-Armstrong]

I am not sure how PCL does it but it should be similar. There is a global way to expose all symbols which may be what they are doing, but this approach is the preferred way. This was copied form rclcpp, which i copied from GCC website.

Do IDEs get confused? This isn't going to make code development painful is it?

Edit: Still reviewing. I guess it took this comment as meaning I was done with the review.

I don't believe so. If you want a class or global function to visible you just need to add TESSERACT_PUBLIC and if it is not supposed to be visible you add TESSERACT_LOCAL and that is pretty much it.

[Levi-Armstrong]

Is this a new macro? I don't remember it.

No it was added as part of the switch to cmake_common_scripts.

[mpowelson]

So if visibility is turned off, then a target that links it will not be able to see it? Could we just default them to visible and set the ones we want private (since there are only a few). I assume not since that is basically what we had before just with none set not visible

[mpowelson]

Looks fine to me assuming this is the overall approach you want to go with. I did not check for missing objects. I just checked the ones you changed.

[mpowelson]

Is there documentation about when to use these? Perhaps we should start a contribution guide like github keeps bugging us about.

[Levi-Armstrong]

Just at the bottom of the GCC link provided. Basically it is the three cases below. Inline, typedefs, templates, enum, const, static variable,functions or classes do not require anything.

class TESSERACT_PUBLIC|TESSERACT_LOCAL ....
struct TESSERACT_PUBLIC|TESSERACT_LOCAL ....

TESSERACT_PUBLIC|TESSERACT_LOCAL void foo();

[mpowelson]

Should these just go in tesseract_variables()? Or is that too general?

[Levi-Armstrong]

I think tat should be fine. I will add it there and remove it from the cmake files.

[mpowelson]

That's an interesting bug.

[Levi-Armstrong]

Yea I am not sure how this was working but when I disabled visibility it just would not work and took a while to figure out what was going on.

[traversaro]

This was copied form rclcpp, which i copied from GCC website.

Small note on this. Most of the ros2 code copies the code from https://gcc.gnu.org/wiki/Visibility that is described as "Some examples of the syntax:", but that code unfortunately does not work well when compiling static libraries under Windows/MSVC. The more complete version is indeed still in https://gcc.gnu.org/wiki/Visibility, but at the bottom, under the "Step-by-step guide", that contains the complete example that works correctly also for static libraries on Windows/MSVC. See ament/ament_cmake#201 (comment) .

Note that this is not a big problem if you are not interesting in supporting compilation as static library under Windows/MSVC.

[Levi-Armstrong]

This was copied form rclcpp, which i copied from GCC website.

Small note on this. Most of the ros2 code copies the code from https://gcc.gnu.org/wiki/Visibility that is described as "Some examples of the syntax:", but that code unfortunately does not work well when compiling static libraries under Windows/MSVC. The more complete version is indeed still in https://gcc.gnu.org/wiki/Visibility, but at the bottom, under the "Step-by-step guide", that contains the complete example that works correctly also for static libraries on Windows/MSVC. See ament/ament_cmake#201 (comment) .

Note that this is not a big problem if you are not interesting in supporting compilation as static library under Windows/MSVC.

Yea I am using the full one that you recommended supporting creating static libraries.

[Levi-Armstrong]

@traversaro In your example it has RCLCPP_BUILD_STATIC_LIBRARY and RCLCPP_BUILD_SHARED_LIBRARY which I just added to mine, but curious why this does not just leverage BUILD_SHARED_LIBS to check if static or shared?

[traversaro]

@traversaro In your example what is the between RCLCPP_BUILD_STATIC_LIBRARY RCLCPP_BUILD_SHARED_LIBRARY which I just added to mine, but curious why this does not just leverage BUILD_SHARED_LIBS to check if static or shared?

If you refer to ament/ament_cmake#201 (comment), it is because the DEFINE_SYMBOL property would be set in any case (see https://cmake.org/cmake/help/v3.18/prop_tgt/DEFINE_SYMBOL.html), so if you don't set it you would get a ${PROJECT_NAME}_EXPORTS macro defined in your code.

[Levi-Armstrong]

@traversaro Sorry one more question. Does RCLCPP_BUILD_STATIC_LIBRARY or RCLCPP_BUILD_SHARED_LIBRARY need to be provided with the target for those linking against your library or can they be private?

target_compile_definition(target PUBLIC RCLCPP_BUILD_SHARED_LIBRARY=ON)

or should I just do this setting default behavior?

if (NOT DEFINED RCLCPP_BUILD_SHARED_LIBRARY)
  set(RCLCPP_BUILD_SHARED_LIBRARY ON)
endif()

[traversaro]

@traversaro Sorry one more question. Does RCLCPP_BUILD_STATIC_LIBRARY or RCLCPP_BUILD_SHARED_LIBRARY need to be provided with the target for those linking against your library or can they be private?

I guess from my example you refer to RCLCPP_BUILDING_LIBRARY_SHARED as RCLCPP_BUILD_SHARED_LIBRARY and RCLCPP_STATIC_LIBRARY as RCLCPP_BUILD_STATIC_LIBRARY ?
Note that the names used were different as the meaning of the variables were indeed different: RCLCPP_STATIC_LIBRARY needs to be defined for both the compilation unit of the static libraries and for the compilation units that consume the static library (i.e. it needs to have PUBLIC visibility, while RCLCPP_BUILDING_LIBRARY_SHARED must be defined only for the compilation unit of the shared library, and must not be defined for the compilation units that consume the shared library, so it should be defined with PRIVATE, or use the DEFINE_SYMBOL property. Note that the advantage of the generate_export_header method is that all of this is handled automatically and the header generation time, and there is no need to have a RCLCPP_STATIC_LIBRARY macro that needs to be defined by downstream projects.

[traversaro]

or should I just do this setting default behavior?

I am not sure I understand what you mean here.

[Levi-Armstrong]

@traversaro I believe the the changes in the latest commit should now enable the support of static libraries?

[traversaro]

@traversaro I believe the the changes in the latest commit should now enable the support of static libraries?

I am probably missing something, but you should have a different visibility header for each library, as the symbol that is exported in a library is the same symbol that is imported in another library, so you can't use the same visibility header (or the same visibility macros) for all libraries.

[Levi-Armstrong]

I am probably missing something, but you should have a different visibility header for each library, as the symbol that is exported in a library is the same symbol that is imported in another library, so you can't use the same visibility header (or the same visibility macros) for all libraries.

Oh, I was not aware.

[Levi-Armstrong]

@traversaro I went through and made the switch such that every library has its own export header but I did have one question.

Because my headers now have "<target_name>_export.h", I had to change the target include directories "$<INSTALL_INTERFACE:include>" to "$<INSTALL_INTERFACE:include;include/${PROJECT_NAME}>" for other packages that use this package to fine the export header. Is this correct or am I doing something else wrong?

[Levi-Armstrong]

I found a way around changing the install include paths setting the export file name.

generate_export_header(${target} EXPORT_FILE_NAME ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}/${target}_export.h)

[Levi-Armstrong]

@traversaro I switch everything to using the generate_export_headers, thanks for the help.

[Levi-Armstrong]

@traversaro I switched to using the generate_export_headers, but it only appears to work on Focal, the Bionic and Xenial seem to fail for odd reasons. Have you ever experienced this?

[Levi-Armstrong]

Well I could not figure out what was going on when using the generate_export_header so I went back an manually added the visibility control file for every target and everything is building and the tests are passing.