#50334 Issue 49730 - MozLDAP bindings have been unsupported for a while
Closed by spichugi. Opened by mhonek.
mhonek/389-ds-base remove-use_openldap  into  master

Download 50334.patch

Bug Description:
We haven't been supporting MozLDAP for a long time. In fact, it is not possible
to build without OpenLDAP as MozLDAP specifics were not maintained properly.

Fix Description:
Remove all MozLDAP-only features from the code.

Fixes https://pagure.io/389-ds-base/issue/49730

Author: mhonek

Review by: ???

Note to self: We should also remove ldapsdk_ stuff as mentioned in https://pagure.io/389-ds-base/pull-request/50332#comment-83802

rebased onto 42b39c617a483524ddc9b82ef1afd525990a8c02

@hmc, @firstyear Could you please have a look at this patch? With the latest rebase I've removed one more if/else regarding the LDAPSDK, however the rest of the likely named variables is actually used to convey the OpenLDAP-related stuff and personally I think there's no strong reason to rename them. Anyway, if I missed something, please let me know.

Thanks!

@mhonek That patch looks like it took a lot of effort, so well done!

The patch itself looks fine to me, but I noticed you missed removing a few of the ldapsdk_* variables in configure.ac and Makefile.am, as well as ldap/admin/src/scripts/ldif2ldap.in and ldap/admin/src/scripts/monitor.in.

The following files also seem to have some relationship with mozldap, but I'm not familiar with the 389-ds-base codebase, so the connection could be tangible at best. Can you please check?
ldap/servers/slapd/libmakefile:
ldap/servers/slapd/test-plugins/Makefile.server
ldap/servers/plugins/distrib/Makefile

ldap/admin/src/scripts/cl-dump.pl
ldap/admin/src/scripts/ns-newpwpolicy.pl.in
ldap/admin/src/scripts/repl-monitor.pl.in

ldap/servers/slapd/connection.c
ldap/servers/slapd/ldaputil.c
ldap/servers/slapd/utf8.c
ldap/servers/slapd/tools/ldclt/ldapfct.c

@firstyear knows far more about this codebase than I do, so he probably has more to say.

@firstyear Any feedback on this patch?

I see nothing obviously wrong with this change, but it's hard to see if it's has indeed 'cleaned up everything' as we want.

So from a technical viewpoint, I think this is fine and won't break anything, and I think if we have missed anything we can clean those later.

I think some of those places, (like when I looked at doing this cleanup), the 389 codebase had duplicated some parts of mozldap into the codebase due to expectations of the api compared to openldap, so I think some of these locations actually are valid to stay to enable functionality in the server that openldap libs don't provide or just do differently.

So I think ack from me here :)

rebased onto b770ac72317e1bdd018b9e3c1a22a403facf2180

I checked the files you mentioned @hmc (thanks for review) and I have to agree with @firstyear the mentions of mozldap/ldapsdk are mostly expected (e.g. sometimes we call it ldapsdk, however it actually carries libldap information, etc.). Also, please do not confuse with perl-mozilla-ldap which we still use in legacy tools.

If no one objects, I would merge the PR.

Pull-Request has been merged by mhonek

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3393

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

Metadata