Use prebuilt MesaLink package in travis CI #878
Conversation
|
All CI tasks passed except this one: https://travis-ci.org/apache/incubator-brpc/jobs/568078842 |
.travis.yml
Outdated
| - PURPOSE=compile USE_MESALINK=yes | ||
| - PURPOSE=unittest USE_MESALINK=yes | ||
| - PURPOSE=compile-with-cmake USE_MESALINK=yes | ||
| - PURPOSE=compile-with-bazel USE_MESALINK=yes |
There was a problem hiding this comment.
USE_MESALINK option seems not working in cmake and bazel. It is still using openssl.
There was a problem hiding this comment.
I haven't updated the build scripts for cmake and bazel. Let me implement the changes today and push to this PR. Let me know if you think a seperate PR for build scripts would be better.
There was a problem hiding this comment.
There are many compilation options, such as with-glog, with-thrift. Just compiling with mesalink in travis seems not a general way to test all options. I think it may be a better way to add one specific PURPOSE called compile-with-all-options and we can turn on all the options here, not just mesalink.
There was a problem hiding this comment.
I have the same feeling; orthogonal build options bloat CI tasks.
I'm not familiar with the options other than USE_MESALINK; and it seems irrelevant to this PR which only adddresses a previous MesaLink build issue.
So maybe we merge this PR first; and create a new issue/PR for the proposed compile-with-all-options PURPOSE.
CMakeLists.txt
Outdated
| @@ -167,6 +182,7 @@ set(DYNAMIC_LIB | |||
| ${THRIFTNB_LIB} | |||
| ${OPENSSL_LIBRARIES} | |||
| ${OPENSSL_CRYPTO_LIBRARY} | |||
| ${MESALINK_LIB} | |||
There was a problem hiding this comment.
What if WITH_MESALINK is defined but openssl is in system and still be found? In the current cmake, brpc may still use openssl?
There was a problem hiding this comment.
MesaLink provides SSL/TLS but not crypto; so OpenSSL's libcrypto is still linked.
Actually, either ${OPENSSL_LIBRARIES} or ${OPENSSL_SSL_LIBRARY} is just redundant in the cases of MesaLink builds. No code uses libssl; therefore the generated libbrpc.so does not link libssl.
ldd output/lib/libbrpc.soyields the following result:
https://paste.ubuntu.com/p/bCMkRJyFK9/
So you can see there's no libssl linked.
There was a problem hiding this comment.
If you think this is not preferred for readability, I have a quickfix using CMake's list(APPEND). Looking forward to your reply.
There was a problem hiding this comment.
Thanks for your detailed explanation. In any case either ${OPENSSL_LIBRARIES} or ${MESALINK_LIB} is used, it's enough to link just one of them. So it may be more clear if we remove ${OPENSSL_LIBRARIES} when mesalink is used.
|
Any comments on the new commits pushed 5 days ago? |
@zyearn
This PR fixes a build issue in https://travis-ci.org/apache/incubator-brpc/jobs/536646641 due to Rust toolchain upgrades.