Fix the transport target dependency for static linking#675
Conversation
|
FYI: I found this problem in facebookincubator/velox#14942 . |
kou
force-pushed
the
cmake-transport-dependency
branch
from
7fda34d to
f711145
Compare
|
Hmm... This works on local but doesn't work on Velox CI... |
kou
force-pushed
the
cmake-transport-dependency
branch
from
f711145 to
e2ef1fd
Compare
`libtransport` uses `apache::thrift::rocket::CompressionAlgorithmSelector::fromTTransform()` in `thrift/lib/cpp/transport/THeader.cpp`: https://github.com/facebook/fbthrift/blob/531d08e0f9af1c17e9cb7fd49ab037a779aee376/thrift/lib/cpp/transport/THeader.cpp#L569-L571 So the `transport` target must depends on the `thriftcpp2` target.
kou
force-pushed
the
cmake-transport-dependency
branch
from
e2ef1fd to
71e51fe
Compare
|
@kou is this ready for merge? |
|
Yes. The build failure problem on Velox CI was caused by Velox side configuration. (This patch wasn't applied correctly.) This PR fixes the link problem found by Velox CI. |
| # and libtransport.so are cyclic dependencies and libthriftcpp2.so | ||
| # has libtransport.so dependency. We can't link to | ||
| # libthriftcpp2.so from libtransport.so. It's not allowed. | ||
| $<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:$<INSTALL_INTERFACE:FBThrift::thriftcpp2>> |
There was a problem hiding this comment.
Why INSTALL_INTERFACE and not BUILD_INTERFACE?
There was a problem hiding this comment.
Because this is for FBThrift users who use find_package(FBThrift) such as Velox.
BUILD_INTERFACE is for FBThrift itself (and FBThrift users who bundle FBThrift by FetchContent https://cmake.org/cmake/help/latest/module/FetchContent.html ).
INSTALL_INTERFACE is for FBThrift users who use FBThrift by find_package(FBThrift).
Related CMake documentation: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#export-and-install-expressions
vitaut
left a comment
vitaut
left a comment
There was a problem hiding this comment.
Thanks for the PR! Overall looks good but I have one question inline.
Summary: `libtransport` uses `apache::thrift::rocket::CompressionAlgorithmSelector::fromTTransform()` in `thrift/lib/cpp/transport/THeader.cpp`: https://github.com/facebook/fbthrift/blob/531d08e0f9af1c17e9cb7fd49ab037a779aee376/thrift/lib/cpp/transport/THeader.cpp#L569-L571 So `libthriftcpp2.a` must be linked after `libtransport.a`. Note that we don't need it for shared linking. Because `libthriftcpp2.so` already has `libtransport.so` dependency. X-link: facebook/fbthrift#675 Reviewed By: yfeldblum Differential Revision: D84010685 Pulled By: vitaut fbshipit-source-id: 2d4a6698451abf335812a280861b20c904e895fe
4 participants
libtransportusesapache::thrift::rocket::CompressionAlgorithmSelector::fromTTransform()inthrift/lib/cpp/transport/THeader.cpp:fbthrift/thrift/lib/cpp/transport/THeader.cpp
Lines 569 to 571 in 531d08e
So
libthriftcpp2.amust be linked afterlibtransport.a.Note that we don't need it for shared linking. Because
libthriftcpp2.soalready haslibtransport.sodependency.