Bug 5513: Fix OpenLDAP detection #2267
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
squid-anubis
added
M-failed-description
|
@FireBurn, do you have an ldap package installed? AFAICT, since 2024 commit 2a849db, our logic for LDAP detection1 is "If pkg-config fails, rely on the user to specify correct libraries/flags/etc. for the features they want to enable (instead of poorly duplicating pgk-config logic/job)".
@FireBurn, what output do you get with these changes? Same as for v6.14? Besides SQUID_CHECK_LDAP_API quoted above, is there output related to the PKG_CHECK_MODULES() call that fails to find Footnotes
|
rousskov
added
the
S-waiting-for-author
| SQUID_AUTO_LIB(ldap,[LDAP],[LIBLDAP]) | ||
| SQUID_CHECK_LIB_WORKS(ldap,[ | ||
| PKG_CHECK_MODULES([LIBLDAP],[ldap],[:],[:]) | ||
| SQUID_STATE_SAVE(squid_ldap_state) |
There was a problem hiding this comment.
State preservation is done by the *_LIB_WORKS macro.
| SQUID_STATE_SAVE(squid_ldap_state) |
| AC_CHECK_HEADERS(ldap.h lber.h) | ||
| AC_CHECK_HEADERS(mozldap/ldap.h) | ||
| SQUID_CHECK_LDAP_API | ||
| SQUID_STATE_ROLLBACK(squid_ldap_state) |
There was a problem hiding this comment.
State preservation is done by the *_LIB_WORKS macro.
| SQUID_STATE_ROLLBACK(squid_ldap_state) |
| SQUID_STATE_SAVE(squid_ldap_state) | ||
| PKG_CHECK_MODULES([LIBLDAP],[ldap],[:],[ | ||
| AC_CHECK_LIB(lber, ber_init, [LIBLBER="-llber"]) | ||
| AC_CHECK_LIB(ldap, ldap_init, [LIBLDAP_LIBS="-lldap $LIBLBER"]) |
There was a problem hiding this comment.
At #2267 (comment): Are you wanting me to just close this and use --with-ldap=/usr/lib64 or something similar?
I am opening a new thread for this important question to avoid hijacking an unrelated change request where that question was asked. My earlier questions were not answered directly, but I assume that #2267 (comment) implies that the following statements are true:
- In your build environment, there are no LDAP pkg-config files installed.
- In your build environment, using
--with-ldap=/usr/lib64(or similar) works.
If the above statements are true, and there are no known problems with existing pkg-config-based code in Squid, then I believe that the correct answer to your question is "Yes, please close this PR: We do not want to restore or add detection code for libraries that provide working pkg-config files (in some environments) and can be correctly handled using custom ./configure options (in other environments). We need to avoid long-term maintenance overheads associated with such detection code. Wiki pull requests documenting workarounds for older environments are welcome."
There was a problem hiding this comment.
- For Dists <EL9, there is no LDAP pkg-config (openldap < 2.6)
--with-ldap=/usr/lib64didn't change anything
There was a problem hiding this comment.
- For Dists <EL9, there is no LDAP pkg-config (openldap < 2.6)
Noted.
--with-ldap=/usr/lib64didn't change anything
IMO, we need to focus on addressing that problem (instead of re-affirming that it is possible to hard-code exceptional code that fixes build in a specific environment like EL7 or EL8). "Addressing that problem" may result in fixing --with-ldap=X support (for all/many environments!) or adjusting those workaround instructions for a specific environment like EL7 or EL8. More at #2267 (comment)
There was a problem hiding this comment.
I will admit that it was premature to drop the AC_CHECK_LIB earlier. Adding them back in a way that works with the new design is fine.
There was a problem hiding this comment.
- For Dists <EL9, there is no LDAP pkg-config (openldap < 2.6)
Noted.
--with-ldap=/usr/lib64didn't change anythingIMO, we need to focus on addressing that problem
The reason for that is that is the placement of the fallback logic within the PKG_CHECK_MODULES macro parameters. Precisely the failure/bug case my change request was about.
With the current PR patch:
--with-ldapshould work--with-ldap=/usr/lib64should work
|
@FireBurn as there is a solution to the issue you highlighted, are you okay with closing this PR? |
|
Sorry, I'll build 7.2 on Monday. If I have issues I'll report back |
|
So with 7.2 with my patch: checking for LIBLDAP... no With 7.3 with passing "--with-ldap=/usr/lib64" I get: checking for LIBLDAP... no |
|
Here is a diff-like summary of the previous comment: -So with 7.2 with my patch:
+With 7.3 with passing "--with-ldap=/usr/lib64" I get:
-checking for ber_init in -llber... yes
-checking for ldap_init in -lldap... yes
-checking for library containing ldap_url_desc2str... -lldap
+checking for library containing ldap_url_desc2str... no
-checking for library containing ldap_url_parse... none required
+checking for library containing ldap_url_parse... no
-checking for library containing ldap_start_tls_s... none required
+checking for library containing ldap_start_tls_s... no
-configure: Library 'ldap' support: yes -lldap -llber
+configure: error: Required library 'ldap' not found |
|
I also tried --with-openldap=/usr/lib64 |
AFAICT, @FireBurn comment is meant to imply that the alternative "solution" discussed earlier does not work and, hence, this PR should be merged instead. My change request remains unaddressed, but I would like to clarify one aspect to avoid a possible misunderstanding:
The question is not whether
Most likely, the correct solution is
Justification for the above reasoning has been provided in my earlier change request. I hope the diff in the earlier comment shows what command line options/variables are missing or what needs to be fixed in configure.ac. I have not studied the issue enough to suggest what is missing or needs to be fixed. |
merging after third-party confirmation that it works.
6 participants
Authored-by: Mike Lothian mike@fireburn.co.uk
This fixes https://bugs.squid-cache.org/show_bug.cgi?id=5513
And if you're happy with it, get it backported to 7.x too