Conversation
mistotebe
force-pushed
the
ldap_connect
branch
2 times, most recently
from
83ac741 to
1a21801
Compare
droideck
left a comment
droideck
left a comment
There was a problem hiding this comment.
Sorry for the late review!
The code looks good!
One minor suggestion, though - I think we need to add an entry to the docs. Somewhere here maybe? https://github.com/python-ldap/python-ldap/blob/30fe146e6c8e881a2db2a3f6b60fb6201bf6a534/Doc/reference/ldap.rst
But it can be done later, I think. So ack from me.
|
Hmm, there's a failure in the added test though on some of the envs (but not all of them). Weird... |
Lib/ldap/ldapobject.py
Outdated
| def connect(self): | ||
| """ | ||
| connect() -> None | ||
| Establishes LDAP connection if needed. | ||
| """ | ||
| if _ldap.VENDOR_VERSION >= 20500: | ||
| return self._ldap_call(self._l.connect) | ||
| raise NotImplementedError |
There was a problem hiding this comment.
I suggest to conditionally define the method. That way it's is possible to introspect the connection object and detect whether libldap supports the call.
| def connect(self): | |
| """ | |
| connect() -> None | |
| Establishes LDAP connection if needed. | |
| """ | |
| if _ldap.VENDOR_VERSION >= 20500: | |
| return self._ldap_call(self._l.connect) | |
| raise NotImplementedError | |
| if _ldap.VENDOR_VERSION >= 20500: | |
| def connect(self): | |
| """ | |
| connect() -> None | |
| Establishes LDAP connection if needed. | |
| """ | |
| return self._ldap_call(self._l.connect) |
There was a problem hiding this comment.
Is that a preference thing? From my point of view, pyhon-ldap's external surface shouldn't morph this way and throwing an exception sounds more appropriate.
I was tempted to use the actual libldap version if we had access to runtime libldap version banging about already.
There was a problem hiding this comment.
Keep in mind that _ldap.VENDOR_VERSION is a compile time constant.
There was a problem hiding this comment.
Keep in mind that
_ldap.VENDOR_VERSIONis a compile time constant.
Yes, that is correct. The function won't be available if python-ldap is compiled with an older version of OpenLDAP. A newer runtime version won't enable the feature.
Is that a preference thing? From my point of view, pyhon-ldap's external surface shouldn't morph this way and throwing an exception sounds more appropriate.
Do you have a better proposal how users can detect the presence of the feature up front? I don't want users to hard-code a version check. You could add ldap.HAVE_CONNECT.
There was a problem hiding this comment.
If they can't handle a NotImplementedException, they can still read ldap.VENDOR_VERSION?
I agree we need to figure out a general way of exposing new libldap interfaces that always exist (unlike ldap_init_fd) without yet more HAVE_* flags that are compile-time, but not runtime anyway.
AFAIK that's usually done by upping a hard dependency but I take it you're against declaring a dependency on 2.5+ just yet (in python-ldap 3.5.0 that is)? That would be the easiest approach and one I'd prefer. We still support 3.4 anyway and that's fine for some (defined) time.
There was a problem hiding this comment.
To be clear, what I'm suggesting is "if you need ldapobject.connect(), you should depend on python-ldap 3.5" and "python-ldap 3.5 will depend on libldap 2.5+".
There was a problem hiding this comment.
This is the alternative... (see latest push)
mistotebe
force-pushed
the
ldap_connect
branch
6 times, most recently
from
059f0d7 to
3b32e9c
Compare
|
In my opinion and experience, we should be very careful and mindful with libldap version requirements. A Python package cannot declare a minimum version of an external dependency. If we bump the minimum version, then we may break people's setup. python-ldap should stay compatible with Alpine, CentOS 7, Debian old stable, openSUSE Leap, and oldest Ubuntu LTS release. A lot of them are still on OpenLDAP 2.4, see https://repology.org/project/openldap/versions . We may consider requiring OpenLDAP 2.5, when CentOS Stream 8 and Debian 11 have reached EOL (2026 for Debian). |
|
Amazon Linux 2023 (just released) still uses OpenLDAP 2.4, sadly. |
mistotebe
force-pushed
the
ldap_connect
branch
3 times, most recently
from
b2e20e5 to
51a8bd1
Compare
|
Not sure I want to do runtime libldap detection, but the new commits do just that. I still think it's better to decide at compile time and have only one ifdef for NotImplementedError/the actual implementation. |
4 participants
No description provided.