Fix TBB transitive dependencies for libblake3#460
Fix TBB transitive dependencies for libblake3#460BurningEnlightenment merged 3 commits intoBLAKE3-team:masterfrom
Conversation
7ea6e31 to
6e6e835
Compare
BurningEnlightenment
left a comment
There was a problem hiding this comment.
I'm a little pressed for time at the moment., so don't consider this a review.
| endmacro() | ||
|
|
||
| if(BLAKE3_USE_TBB) | ||
| find_package(TBB QUIET) |
There was a problem hiding this comment.
AFAIK find_dependency should be used in package configs.
There was a problem hiding this comment.
@BurningEnlightenment I considered using find_dependency but it doesn't seem to be appropriate here. find_dependency is meant for required dependencies but here TBB is a conditional dependency.
See this comment from the CMake devs discussing a similar situation:
find_dependencyis meant for required dependencies and is supposed to handle theQUIETandREQUIREDoptions from the caller automatically. Don't pass them explicitly.In a given build of
polo, CURL is either a dependency or it is not. Thepolo-config.cmakefile should be generated in a way that requires CURL or does not even search for CURL depending on how the project was built.
The way we simulate forwarding REQUIRED for the find_package(TBB) call is by specifying TBB via the COMPONENTS option (as opposed to OPTIONAL_COMPONENTS):
find_package(blake3 COMPONENTS TBB)
There was a problem hiding this comment.
@silvanshade maybe I'm wrong, but if blake3 has been statically built with TBB enabled you should get link errors if you don't also link against TBB, because blake3.c refers to symbols from blake3_tbb.cpp therefore linker shouldn't be able to omit the objects.
There was a problem hiding this comment.
@BurningEnlightenment You are right that the downstream project must link against TBB (if BLAKE3 was built with BLAKE3_USE_TBB), but the downstream project should not need to manually do so, it should be handled automatically through the appropriate CMake transitive target mechanisms. That is what this PR fixes.
This is achieved in the code snippets highlighted below:
add_library(BLAKE3::tbb INTERFACE IMPORTED)creates a logical target for theTBBthat BLAKE3 links.target_link_libraries(BLAKE3::tbb INTERFACE TBB::tbb)addsTBB::tbbto the link interface forBLAKE3::tbb.
The second line above ensures any target linking BLAKE3::tbb also links TBB::tbb.
target_link_libraries(BLAKE3::blake3 INTERFACE BLAKE3::tbb)addsBLAKE3::tbbto the link interface forBLAKE3::blake3.
Now any target linking BLAKE3::blake3 also links TBB::tbb because of how the link interface is configured.
I've tested and this does work as expected. You can try the example here: https://gist.github.com/silvanshade/1e1ae78cc0ad576d5f5060ceeafde11f
There was a problem hiding this comment.
We shouldn't need to target_link_libraries(BLAKE3::tbb INTERFACE TBB::tbb) in the config.cmake because we target_link_libraries(blake3 PUBLIC TBB::tbb) at CMakeLists.txt:232 which should propagate into the exported targets. I.e. the following should be necessary and sufficient:
if(@BLAKE3_USE_TBB@)
find_dependency(TBB)
endif()There was a problem hiding this comment.
I.e. the following should be necessary and sufficient:
if(@BLAKE3_USE_TBB@) find_dependency(TBB) endif()
You are right -- that is enough given the earlier use of the PUBLIC target link.
I've done things more elaborately because I was considering how usage of the CMake config would look from the perspective of a downstream project needing libblake3 with TBB support.
With the way I've designed it now, you can specify:
find_package(blake3 COMPONENTS TBB)If TBB isn't found, or isn't found with a compatible version, you get an error and an informative message.
With the minimal approach, extra checking and error logic is required:
find_package(blake3)
if(NOT BLAKE3_USE_TBB)
message(FATAL_ERROR "Expected TBB")
endif()I think the COMPONENTS version is more direct and the additional code I've implemented around that is designed to be reusable for other features in the future.
But I think it's important to settle on a design quickly in order to get a fix released so I'll simplify it as you suggest.
6e6e835 to
b67759a
Compare
b67759a to
510ae9d
Compare
The `join_pkg_config_requires` function would misbehave in a few edge cases. Though these edge cases aren't triggered by our current usage.
BurningEnlightenment
left a comment
There was a problem hiding this comment.
LGTM as soon as CI greenlights this. Thanks!
|
@oconnor663 can you cut a patch release soon? (As usual) I totally forgot the |
|
I added one additional fix for pkg-config at #461 |
This PR fixes the transitive dependencies for TBB when
libblake3is built withBLAKE3_USE_TBB=1which I unfortunately overlooked in the original PR.This PR introduces two changes:
BLAKE3_USE_TBB=1thentbbis added to theRequires:field forpkg-configBLAKE3_USE_TBB=1then CMakefind_package(blake3)automatically finds and configures theTBB::tbbtargetAdditionally, TBB functionality can be specifically requested through the CMake config with the following (which will fail with an informative error if not available):
An alternative design to this fix would be the approach suggested by @Ericson2314 in this comment.