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

Do not export fcl dependency #256

Merged
merged 4 commits into from
Dec 6, 2024
Merged

Do not export fcl dependency #256

merged 4 commits into from
Dec 6, 2024

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Dec 5, 2024

Hopefully closes #255.

It seems we have erroneousely exported fcl as interface link library when it should only be private (it is only used in obb.cpp and in no header files).

@peci1
Copy link
Contributor Author

peci1 commented Dec 5, 2024

Hmm, it seems this PR has started a bit of a rabbit hole.

To fix the first build failure, I've added the PUBLIC keyword to the first ament_target_dependencies.

I've missed the second ament_target_dependencies, which should also receive a PUBLIC keyword, because that is the only one ament_target_dependencies accepts (it doesn't support INTERFACE or PRIVATE).

Now, I could add the PUBLIC keyword there, but assimp is not an exported dependency according to package.xml (the same way as fcl). On the other hand, qhull is declared as <depends>, so it should be exported.

However, the only usage of qhull I see is in bodies.cpp, so it could technically also be a non-transitive dependency.

Please, maintainers, guide me towards the wanted result in these decisions:

  1. Should I add PUBLIC keyword to the ament_target_dependencies(... assimp QHULL) ?
  2. Should I move assimp out of the ament_target_dependencies and put it next to fcl in target_link_libraries(PRIVATE) ? This would remove assimp library from the exported link libraries, probably breaking some downstream that depends on it (which would be wrong because assimp is not transitive according to package.xml).
  3. Or should I move both assimp and QHULL to the target_link_libraries(PRIVATE) and redeclare qhull as non-transitive dependency in package.xml? This would remove both assimp and qhull from exported link libraries, probably breaking some downstream that depends on them (which would be wrong in case of assimp, but correct in case of qhull, except the fact that relying on transitive dependencies is not a good habit).

@rhaschke
Copy link
Contributor

rhaschke commented Dec 5, 2024

Dependencies that are publicly exposed in headers should be declared public in cmake and transitive in package.xml.
As far as I see, neither assimp, fcl, or qhull are exposed in the headers. Thus, you should go for option 3 and declare all of them private and non-transitive.
Issues in downstream packages should be fixed later on.

@peci1
Copy link
Contributor Author

peci1 commented Dec 5, 2024

Okay, I've turned fcl, assimp and qhull into private dependencies. Let's see what happens downstream :)

@peci1
Copy link
Contributor Author

peci1 commented Dec 5, 2024

Feel free to squash-merge this as the commit history in this PR does not make much sense.

@rhaschke rhaschke merged commit 60405a3 into moveit:ros2 Dec 6, 2024
8 checks passed
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (cf9eb23) to head (2b37f95).
Report is 1 commits behind head on ros2.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@     Coverage Diff     @@
##   ros2   #256   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

3 participants