fcl: add missing transitive_headers=True, CMP0077 #25381
+24
−20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jcar87
previously requested changes
Sep 24, 2024
recipes/fcl/all/conanfile.py
Outdated
| if self.options.with_octomap: | ||
| self.requires("octomap/1.9.7") | ||
| # Used in fcl/geometry/octree/octree.h | ||
| self.requires("octomap/1.9.7", transitive_headers=True, transitive_libs=True) |
There was a problem hiding this comment.
none of these seem to require transitive_libs - eigen is a hader only library and for the other two headers mention, only types /macros seem to be required, but should cause no symbols to be directly required by the translation unit including this header - at least, not "indirectly".
There was a problem hiding this comment.
It's generally not worth the time and effort to dig into the finer points of transitive_libs=True/False for every minor library, IMO, although I don't mind setting it to False for clear header-only libs like Eigen or OctoMap.
It's very easy to miss uses of externally-defined symbols in the code and test_package usually tests a too narrow slice of the library to catch issues caused by transitive_libs=False.
Regarding Eigen specifically, though, it does have support for an optional SuiteSparse backend that could be well added in the future as an optional dependency, which is why I decided to keep it as True.
Anyway, I'll change it to False for Eigen and OctoMap.
There was a problem hiding this comment.
It's generally not worth the time and effort to dig into the finer points of transitive_libs=True/False for every minor library,
Thanks! We don't expect you to do it - we are very happy to do so as part of the review process so that we can orient this better to suit the recipe. transitive_libs=True can result in over-linking for shared libraries (consumers will unconditionally link against it). IIRC only the Linux distros where ld has --as-needed by default (or gcc is configured to pass it) are robust against this.
Situation:
-
foobardepends onfcl,fcldepends onoctomap(foobar->fcl->octomap) -
transitive_libs=True(fcl on octomap) will cause thefoobarbinary to record a dependency onoctomap, uncondtionally, even when shared (except for the--as-neededcase)
The two things we want to protect ourselves against:
When all are shared=True:
- octomap is updated to a version where the minor component was updated (minor component -> source compatible, ABI compatible but may have new symbols). Because it is source compatible and
foobardidnt change, there's no reason forfoobarto be rebuilt or be aware ofoctomapat all. Conan won't trigger a rebuild - but if anything in the update changes, such as changing the .soname (which has unfortunately happened before in Conan center), foobar will be broken, even iffclis successfully rebuilt.
When fcl is shared and octomap is static:
- octomap symbols may end up in both fcl and the consumer (may being key here, will depend on the linker behaviour)
- being able to mix/match and select which libraries are static and which arent - are a feature we know users do use, even if it's not the general case we expect.
We can probably lint/statically detect the correct cases here - I'll make sure we have hooks that can advise better.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 8f60fb7fcl/0.6.1@#888fcc68c9fe7dabb483acf0334498f2fcl/0.7.0@#814c14d3b69a30bdf79ead26bb5815fa |
This comment has been minimized.
This comment has been minimized.
jcar87
reviewed
Sep 24, 2024
recipes/fcl/all/conanfile.py
Outdated
| tc.variables["OCTOMAP_PATCH_VERSION"] = octomap_version.patch | ||
| tc.variables["BUILD_TESTING"] = False | ||
| tc.variables["FCL_NO_DEFAULT_RPATH"] = False | ||
| tc.cache_variables["OCTOMAP_MAJOR_VERSION"] = octomap_version.major |
There was a problem hiding this comment.
I suspect these may need str() - the requirement to be json serializable may come from the fact that cache_variables end up in the generated presets file.
Conan v1 pipeline ✔️All green in build 3 ( Conan v2 pipeline ✔️
All green in build 3 ( |
Hooks produced the following warnings for commit 3baf85dfcl/0.7.0@#28b233d6dab43a14515f616ba516a067fcl/0.6.1@#67b1beb5cfaecda71f9ada52dcbe13a1 |
jcar87
reviewed
Sep 25, 2024
| # Used in fcl/common/types.h public header | ||
| self.requires("eigen/3.4.0", transitive_headers=True) | ||
| # Used in fcl/narrowphase/detail/convexity_based_algorithm/support.h | ||
| self.requires("libccd/2.1", transitive_headers=True, transitive_libs=True) |
There was a problem hiding this comment.
As far as I can see - this still does not require transitive_libs to be unconditionally set to true, has this been verified?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Changes to recipe: fcl/[*]
Motivation
Details