-
Notifications
You must be signed in to change notification settings - Fork 126
Some detection whether we need to use -lldap_r to get threadsafe libldap #458
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
Conversation
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 recommend against using ctypes. The find_library
function does not take distutils / setuptools options and flags into account. OpenLDAP 2.6 finally ships pkg-config files, so you could use pkg-config on Unix-like systems and figure out a fallback for OpenLDAP 2.4 and for Windows.
Or you could hook into build_ext command and use the compilers find_library_file
helper to check for presence of ldap or ldap_r shared libs.
from setuptools.command.build_ext import build_ext
class ldap_build_ext(build_ext):
def build_extension(self, ext):
library_dirs = self.compiler.library_dirs + ext.library_dirs
if self.compiler.find_library_file(library_dirs, "ldap_r"):
...
super().build_extension(ext)
setup(
...,
cmdclass = {'build_ext': ldap_build_ext},
...
)
I like the OPENLDAP_THREAD_SAFE
check. Should we check the flags in the module initializer of the C extension instead?
Thanks for pointing me to setuptools API, didn't know these things were available, still nothing there I can find for examining pkg-config files nor tools to examine I could tweak the patch as you suggest below (using
Alternatively, we could just say we assume libldap is thread safe (switching to setting |
setuptools has no code to run pkg-config. You need to role your own code, e.g. run You could use
Or keep it really simple:
|
Just been testing |
As shown in my initial example, you have to pass a combination of compiler and extension library dirs to the function.
|
Which is what I tested with. Anyway, I did away with all that and just moved to the runtime check. |
@tiran any feedback on the comments? |
627fa9a
to
bfb61b2
Compare
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.
Besides the minor issue - looks good!
bfb61b2
to
f121751
Compare
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.
LGTM!
Thanks!
P.S. I've also tested on Fedora 36 and it works with 389-ds-base tests
Closes #432