Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Some detection whether we need to use -lldap_r to get threadsafe libldap#458

Merged
mistotebe merged 1 commit into
python-ldap:masterpython-ldap/python-ldap:masterfrom
mistotebe:libldap2.5mistotebe/python-ldap:libldap2.5Copy head branch name to clipboard
Apr 20, 2022
Merged

Some detection whether we need to use -lldap_r to get threadsafe libldap#458
mistotebe merged 1 commit into
python-ldap:masterpython-ldap/python-ldap:masterfrom
mistotebe:libldap2.5mistotebe/python-ldap:libldap2.5Copy head branch name to clipboard

Conversation

@mistotebe

Copy link
Copy Markdown
Contributor

Closes #432

@tiran tiran left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

tiran commented Feb 1, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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

tiran commented Feb 1, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

Comment thread Modules/common.h
Comment thread Modules/constants_generated.h
Comment thread setup.cfg
@mistotebe

Copy link
Copy Markdown
Contributor Author

@tiran any feedback on the comments?

@droideck droideck left a comment

Copy link
Copy Markdown
Contributor

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!

Comment thread setup.cfg
droideck
droideck previously approved these changes Feb 25, 2022

@droideck droideck left a comment

Copy link
Copy Markdown
Contributor

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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.