-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add missing libcrypto for undefined references #2436
Conversation
Can one of the admins verify this patch? |
@phsft-bot build |
Starting build on |
@@ -4,6 +4,7 @@ | |||
############################################################################ | |||
|
|||
ROOT_EXECUTABLE(rootd *.cxx ${CMAKE_SOURCE_DIR}/core/clib/src/strlcpy.c | |||
LIBRARIES rpdutil rsa ${GLOBUS_LIBRARIES} ${OPENSSL_LIBRARIES}) | |||
LIBRARIES rpdutil rsa ${GLOBUS_LIBRARIES} ${OPENSSL_LIBRARIES} | |||
BUILTINS OPENSSL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand, how does this add libcrypto?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only place where it said LIBRARIES ${OPENSSL_LIBRARIES} without BUILTINS OPENSSL. You should always have both. ${OPENSSL_LIBRARIES} provides libssl and libcrypto when the system libraries are used, BUILTINS OPENSSL provides libssl and libcrypto when the builtin openssl is used. Having {OPENSSL_LIBRARIES} without BUILTINS OPENSSL is inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the builtin for OpenSSL, so that having BUILTINS OPENSSL
would not be necessary anymore. Is there a way I can reproduce the error you see? See, for example, commits 65bbb76 and 3787d16, and take a look at the log for builtins/openssl
in ROOT for more details. If some configuration doesn't work for you, I'd like to fix the CMakeLists.txt
for builtin OpenSSL. Eventually, I want the BUILTINS
option to disappear from the build system. The dependencies should be there by default regardless if a package is builtin or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change here was just for consistency.
The change needed to fix the link error was the change in the rule for compiling libSrvAuth.so in net/rpdutils/CMakeLists.txt. After the change that made the dependency on openssl in libNet private, this dependency is no longer inherited by SrvAuth by declaring DEPENDENCIES Net. And since SrvAuth uses openssl directly the linking started to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The undefined references come from globus.cxx.o, so in order to reproduce you need to configure using -Dglobus:BOOL=ON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I will try with this configuration and fix the problem. The BUILTINS
option will gradually go away, so I don't want to add it back if it's not needed.
@ellert Thanks for the fix and sorry for the late merging! |
Missing linking to the libcrypto library causes undefined symbols: