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

auth: try to find ldap libs in case pkconfig is missing (happens on debian11) #15086

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

omoerbeek
Copy link
Member

Short description

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

@omoerbeek omoerbeek added the auth label Jan 24, 2025
@Habbie
Copy link
Member

Habbie commented Jan 24, 2025

with this change, what happens if the libraries really are not there?

@omoerbeek
Copy link
Member Author

with this change, what happens if the libraries really are not there?

Good question, did not test that, will do.

@omoerbeek
Copy link
Member Author

omoerbeek commented Jan 24, 2025

Setup does succeed, but compile fails.

  LDAP
    LDAP                      : NO
    LBER                      : NO
    Krb5                      : YES
    Krb5 Name                 : krb5
    Krb5 Version              : 1.18.3

and then

FAILED: modules/libldapbackend.a.p/ldapbackend_ldapauthenticator.cc.o 
ccache c++ -Imodules/libldapbackend.a.p -Imodules -I../modules -I. -I.. -I../pdns -Iext/arc4random -I../ext/arc4random -Iext/ipcrypt -I../ext/ipcrypt -Iext/json11 -I../ext/json11 -I../ext/luawrapper/include -I../ext/protozero/include -Iext/yahttp -I../ext/yahttp -I/usr/include/luajit-2.1 -I/usr/include/x86_64-linux-gnu -I/usr/include -fdiagnostics-color=always -D_GLIBCXX_ASSERTIONS=1 -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=c++17 -O2 -g -DHAVE_CONFIG_H -Wshadow -Wmissing-declarations -Wredundant-decls -Wno-ignored-attributes -fstack-protector --param=ssp-buffer-size=4 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -DBOOST_CONTAINER_USE_STD_EXCEPTIONS -fPIC -isystem /usr/include/mit-krb5 -DBOOST_ALL_NO_LIB -pthread -MD -MQ modules/libldapbackend.a.p/ldapbackend_ldapauthenticator.cc.o -MF modules/libldapbackend.a.p/ldapbackend_ldapauthenticator.cc.o.d -o modules/libldapbackend.a.p/ldapbackend_ldapauthenticator.cc.o -c ../modules/ldapbackend/ldapauthenticator.cc
In file included from ../modules/ldapbackend/ldapauthenticator_p.hh:22,
                 from ../modules/ldapbackend/ldapauthenticator.cc:21:
../modules/ldapbackend/ldapauthenticator.hh:21:10: fatal error: ldap.h: No such file or directory
   21 | #include <ldap.h>
      |          ^~~~~~~~
compilation terminated.

@miodvallat
Copy link
Contributor

miodvallat commented Jan 24, 2025

This was to be expected. Without pkgconfig information, you have been able to find the library, but you also need to find the header files...

Edit: nevermind, I had not noticed you were building without the library.

@omoerbeek
Copy link
Member Author

This was to be expected. Without pkgconfig information, you have been able to find the library, but you also need to find the header files...

The setup run should have failed as I ran meson setup -Dmodule-ldap=static without having ldap.

@coveralls
Copy link

coveralls commented Jan 24, 2025

Pull Request Test Coverage Report for Build 12952588052

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 50 unchanged lines in 13 files lost coverage.
  • Overall coverage increased (+0.01%) to 64.71%

Files with Coverage Reduction New Missed Lines %
modules/gpgsqlbackend/gpgsqlbackend.cc 1 88.62%
pdns/validate.cc 1 68.09%
pdns/pollmplexer.cc 1 83.66%
pdns/misc.cc 2 62.82%
pdns/misc.hh 3 87.62%
pdns/packethandler.cc 3 72.4%
pdns/iputils.cc 3 56.07%
modules/gpgsqlbackend/spgsql.cc 3 67.94%
pdns/dnsdistdist/dnsdist-lbpolicies.cc 4 70.3%
pdns/recursordist/rec-tcp.cc 4 66.54%
Totals Coverage Status
Change from base Build 12944046345: 0.01%
Covered Lines: 127871
Relevant Lines: 166476

💛 - Coveralls

@omoerbeek omoerbeek requested a review from Habbie January 24, 2025 15:06
@@ -21,7 +21,10 @@ if get_option('module-ldap') != 'disabled'
endif
endif
endif


if not dep_ldap_internal.found()
Copy link
Member

Choose a reason for hiding this comment

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

nice. I wonder whether required: true on line 9 (and maybe 17?) would be simpler, or perhaps I am missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the execution than bails out early and the alternative method does not get run

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

Successfully merging this pull request may close these issues.

4 participants