Skip to content
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

Assume that callbacks are not broken in OpenLDAP when cross-compiling #7743

Closed
wants to merge 1 commit into from

Conversation

krnowak
Copy link
Contributor

@krnowak krnowak commented Dec 4, 2024

This adds a LDAP_HAVE_CONNCB_CROSS variable that will be used instead of the runtime check for a broken connection callbacks in ldap library. Set it to yes to assume that connection callbacks are working.

This allows the project to be configured successfully when cross-compiling.

If we do cross-compiling against a known broken version of OpenLDAP, we can do export ac_cv_member_struct_ldap_conncb_lc_arg=no before running configure. This is rather unlikely now, as the test was done to detect a bug that was fixed 16 years ago.

This allows the project to be configured successfully when cross-compiling, without disabling connection callbacks.

Tested locally in Flatcar (flatcar/scripts#2501)

@chewi
Copy link

chewi commented Dec 4, 2024

Given that you can already override the result with ac_cv_member_struct_ldap_conncb_lc_arg, I don't think an additional variable is really needed, but let's see what the maintainers think.

@krnowak
Copy link
Contributor Author

krnowak commented Dec 4, 2024

Given that you can already override the result with ac_cv_member_struct_ldap_conncb_lc_arg, I don't think an additional variable is really needed, but let's see what the maintainers think.

This can override a result of AC_CHECK_MEMBERS([struct ldap_conncb.lc_arg], ...), but there is nothing to override a result of AC_RUN_IFELSE that is being run when ac_cv_member_struct_ldap_conncb_lc_arg is "yes".

@chewi
Copy link

chewi commented Dec 4, 2024

Okay, but setting it to no is what would help here. When cross-compiling, you can either:

  • Let it do the AC_CHECK_MEMBERS check, and if yes, call the cross action, which assumes your OpenLDAP is not broken.
  • Manually set ac_cv_member_struct_ldap_conncb_lc_arg=no, bypassing the whole thing, because you know your OpenLDAP is broken.

@krnowak
Copy link
Contributor Author

krnowak commented Dec 4, 2024

Okay, but setting it to no is what would help here. When cross-compiling, you can either:

  • Let it do the AC_CHECK_MEMBERS check, and if yes, call the cross action, which assumes your OpenLDAP is not broken.
  • Manually set ac_cv_member_struct_ldap_conncb_lc_arg=no, bypassing the whole thing, because you know your OpenLDAP is broken.

Setting it to no means that we do not use LDAP connection callbacks. It indeed means that runtime check for broken OpenLDAP does not happen, but it is a lazy solution for making cross-compilation to work, because it results in using a sub-par solution instead of connection callbacks (as far as I understand it, of course). But if I know that our OpenLDAP is not broken and I want to cross-compile the project, then without this PR this is not possible to have - AC_CHECK_MEMBERS will set the ac_cv_member_struct_ldap_conncb_lc_arg=yes, but AC_RUN_IFELSE will bail out, because of empty "action-if-cross-compiling".

Honestly, I'd prefer to drop the AC_RUN_IFELSE - it is a check for an OpenLDAP bug that was fixed 16 years ago, I think.

@chewi
Copy link

chewi commented Dec 4, 2024

Our wires are crossed. 😅

If you want the callbacks, then you wouldn't set it at all. I still think a change is needed, I just think it should be:

@@ -80,7 +81,8 @@ AC_CHECK_MEMBERS([struct ldap_conncb.lc_arg],
                    [AC_DEFINE([HAVE_LDAP_CONNCB], [1],
                      [Define if LDAP connection callbacks are available])],
                    [AC_MSG_WARN([Found broken callback implementation])],
-                   [])],
+                   [AC_DEFINE([HAVE_LDAP_CONNCB], [1],
+                     [Define if LDAP connection callbacks are available])])],
                  [], [[#include <ldap.h>]])
 
 AC_CHECK_TYPE([LDAPDerefRes],

@krnowak
Copy link
Contributor Author

krnowak commented Dec 4, 2024

Oh, I finally got what you meant. :)

Yeah, true, that'd be simpler. I'll make the change.

@krnowak krnowak force-pushed the krnowak/cross-compile-fix branch from 7aac435 to 5b27ca0 Compare December 4, 2024 14:59
@krnowak krnowak changed the title Add a variable to help with cross-compilation Assume that callbacks are not broken in OpenLDAP when cross-compiling Dec 4, 2024
@krnowak
Copy link
Contributor Author

krnowak commented Dec 4, 2024

PR updated. I'm wondering if it could be also backported to 2.9 and 2.10 branches.

src/external/ldap.m4 Outdated Show resolved Hide resolved
If we do cross-compiling against a known broken version of OpenLDAP,
we can do `export ac_cv_member_struct_ldap_conncb_lc_arg=no` before
running configure. This is rather unlikely now, as the test was done
to detect a bug that was fixed 16 years ago.

This allows the project to be configured successfully when
cross-compiling, without disabling connection callbacks.
@krnowak krnowak force-pushed the krnowak/cross-compile-fix branch from 5b27ca0 to 3c66b0b Compare December 11, 2024 17:46
@krnowak
Copy link
Contributor Author

krnowak commented Dec 11, 2024

@sumit-bose: Thanks for the review. PR updated.

@krnowak krnowak requested a review from sumit-bose December 11, 2024 17:47
Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thank you for the update, ACK.

bye,
Sumit

@alexey-tikhonov
Copy link
Member

Pushed PR: #7743

  • master
    • 39f9ff8 - Assume that callbacks are not broken in OpenLDAP when cross-compiling
  • sssd-2-10
    • 8bac60a - Assume that callbacks are not broken in OpenLDAP when cross-compiling
  • sssd-2-9
    • ee195e7 - Assume that callbacks are not broken in OpenLDAP when cross-compiling

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants