explain why detail::View symbols are visible

[adlarkin]

Signed-off-by: Ashton Larkin [email protected]

🦟 Bug fix

Fixes #759

Summary

This hides the symbols of the detail::View class so that we can make changes to it as needed without breaking ABI. Since this is an ABI-breaking change, this has to be done in fortress.

Edit: the symbol visibility for detail::View cannot be hidden. See the comments below for an explanation.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

[scpeters]

linking error in Ubuntu:

undefined reference to `ignition::gazebo::v6::detail::View::AddComponent(unsigned long, unsigned long, int)'

https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/6028/

maybe the symbols need to be visible?

[adlarkin]

maybe the symbols need to be visible?

So, after looking into the linking errors and chatting with @mjcarroll offline, I believe that the symbols for detail::View need to be visible. The reason for this is because when users call EntityComponentManager::Each, View::Component is called, which then calls View::ComponentImplementation. Since we are using templates for things like Each, this means that the templates are expanded where it's being used, so the detail::View class needs to be visible in order for downstream users to be able to call ecm.Each. There's also at least one other method that has this same issue (template method that uses a view in the definition): EntityComponentManager::AddComponentsToView

So, unless we can hide the templates somehow, then I believe we will need to keep detail::View symbols as visible, which is probably why they were never hidden in the first place.

@chapulina what do you think? I think we should probably close this PR and also close #759, with a note explaining why we can't hide the symbol visibility. Perhaps it would also be worth adding some documentation to the detail::View class, explaining why it's visible and not hidden.

[chapulina]

@chapulina what do you think?

Ouch, ok, it makes sense. I thought that we shouldn't need to make the symbols visible because they were headers. Gotta learn more about symbol generation.

I guess then that we can't ever make ABI breaking changes to detail. So even though users shouldn't be using them directly, we'll be keeping them stable. This is something good to know, and it means that our message to users can be simpler: "Anything that's installed will be kept ABI / API stable, regardless of whether they're in a detail folder."

[adlarkin]

I guess then that we can't ever make ABI breaking changes to detail

I don't think this statement is 100% true. If you look at ComponentStorageBase.hh, both the ComponentStorageBase and ComponentStorage classes are hidden, and work just fine since these classes aren't used in template methods elsewhere (i.e., these classes are completely self-contained in their respective library). So, I think it's more accurate to say that we can't make ABI breaking changes to some things in detail, which means that there's still a chance of API/ABI breaking changes for some things in detail.

I went ahead and reverted back to visible symbols in detail::View and added a note that explains why detail::View needs to be visible in f6606d7. I also updated the PR title. If this looks good to you, I think we can merge this PR and then close #759.

[codecov]

Codecov Report

Merging #788 (f6606d7) into main (f4a3ea4) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head f6606d7 differs from pull request most recent head 6143d3e. Consider uploading reports for the commit 6143d3e to get more accurate results

@@           Coverage Diff           @@
##             main     #788   +/-   ##
=======================================
  Coverage   65.34%   65.35%           
=======================================
  Files         240      240           
  Lines       17602    17602           
=======================================
+ Hits        11502    11503    +1     
+ Misses       6100     6099    -1     

Impacted Files	Coverage Δ
include/ignition/gazebo/detail/View.hh	100.00% <ø> (ø)
src/Server.cc	83.43% <0.00%> (+0.63%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4a3ea4...6143d3e. Read the comment docs.

[chapulina]

these classes aren't used in template methods elsewhere

Interesting, I think I'm starting to understand. It's very nuanced, something to be careful about when using these classes on our own headers too.