Skip to content

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

Merged
merged 1 commit into from
Apr 20, 2022

Conversation

mistotebe
Copy link
Contributor

Closes #432

Copy link
Member

@tiran tiran left a 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?

@mistotebe
Copy link
Contributor Author

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.

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 ldap.h to get the library version/feature flags so that doesn't get us all the way.

I could tweak the patch as you suggest below (using cmdclass) to find the right library path, but will have to dlopen it with ctypes anyway. Feel free to tweak the PR or suggest an alternative approach.

I like the OPENLDAP_THREAD_SAFE check. Should we check the flags in the module initializer of the C extension instead?

Alternatively, we could just say we assume libldap is thread safe (switching to setting LIBLDAP_R at load time) and require that people building on distros where libldap and libldap_r do not point to the same library (if there are any left) choose to link to libldap_r by hand.

@tiran
Copy link
Member

tiran commented Feb 1, 2022

setuptools has no code to run pkg-config. You need to role your own code, e.g. run pkg-config --libs, shlex the output, and extend extra_linker_args. Do the same for cflags and extra compiler args.

You could use self.compiler.preprocess to run the pre-processor with a feature check file like this and check the exit code:

#include <ldap.h>

#ifndef LDAP_API_FEATURE_X_OPENLDAP_THREAD_SAFE
#errror "no thread safe"
#endif

Or keep it really simple:

  • check for ldap_r, if present use ldap_r
  • otherwise use ldap
  • add a check for thread safe libldap to _ldap extension module and fail to compile / load when a non-reentrant LDAP lib is present.

@mistotebe
Copy link
Contributor Author

Just been testing find_library_file() and if passed empty library_dirs (the default in our case), it will always return None anyway, so that's no use whatsoever for us.

@tiran
Copy link
Member

tiran commented Feb 1, 2022

Just been testing find_library_file() and if passed empty library_dirs (the default in our case), it will always return None anyway, so that's no use whatsoever for us.

As shown in my initial example, you have to pass a combination of compiler and extension library dirs to the function.

library_dirs = self.compiler.library_dirs + ext.library_dirs

@mistotebe
Copy link
Contributor Author

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.

@mistotebe
Copy link
Contributor Author

@tiran any feedback on the comments?

Copy link
Contributor

@droideck droideck left a 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!

droideck
droideck previously approved these changes Feb 25, 2022
Copy link
Contributor

@droideck droideck left a 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

@mistotebe mistotebe requested a review from tiran March 1, 2022 16:38
@mistotebe mistotebe added this to the 3.4.1 milestone Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for OpenLDAP 2.5+
3 participants