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

Jump to bottom

cmake: fix `HAVE_LDAP_SSL`, `HAVE_LDAP_URL_PARSE` on non-Windows #12006

Closed

vszakats wants to merge 8 commits into curl:master from vszakats:cmake-HAVE_LDAP_SSL-fix

Member

vszakats commented Oct 2, 2023 •

edited

set HAVE_LDAP_URL_PARSE if ldap_url_parse function exists.
Before this patch we set it based it on the presence of stricmp,
which correctly enabled it on e.g. Windows, but was inaccurate for
other platforms.
always set HAVE_LDAP_SSL if an LDAP backend is detected and
LDAPS is not explicitly disabled. This mimics autotools behaviour.
Previously we set it only for Windows LDAP. After this fix, LDAPS is
correctly enabled e.g. in default macOS builds.
enable LDAP[S] for a CMake macOS CI job. Target OS X 10.9 (Mavericks)
to avoid deprecation warnings for LDAP API.
always detect HAVE_LDAP_SSL_H, even with LDAPS explicitly disabled,
to sync behaviour with autotools.
fix benign typo in internal variable name.
eliminate an internal variable.

Ref: #11964 (effort to sync cmake detections with autotools)

Closes #12006


          cmake: try fixing HAVE_LDAP_SSL for OpenLDAP

b842698

Blind fix, let's see the CI results.

Ref: curl#11964 (effort to sync cmake detections with autotools)

Closes #xxxxx

vszakats added cmake LDAP labels

github-actions bot added the build label

vszakats mentioned this pull request

workflow to compare configure vs cmake outputs #11964

Closed

16 tasks

vszakats added 3 commits

October 2, 2023 11:46


          streamline ldap_ssl check

fb0988a


          add HAVE_LDAP_URL_PARSE check

a495d73


          fix benign typo

ba78cfd

vszakats changed the title ~~cmake: try fixing HAVE_LDAP_SSL for OpenLDAP~~ cmake: try fixing HAVE_LDAP_SSL, HAVE_LDAP_URL_PARSE for OpenLDAP

vszakats added 3 commits

October 2, 2023 14:38


          sync ldap_ssl.h and HAVE_LDAP_SSL with autotools

8db3372

do what autotools does:

- always check for ldap_ssl.h, even with LDAPS force-disabled

- set HAVE_LDAP_SSL if LDAPS enabled (default), regardless of
  the result of the ldap_ssl.h check. This header is missing
  from OpenLDAP packages, e.g. from the one bundled with macOS
  or the latest OpenLDAP installed via Homebrew on macOS.


          use check_include_file (not concat)

bf9376f

concat seems unnecessary.


          enable LDAP/LDAPS for a macOS CI job

8bb1acd

also make sure to target a macOS version when LDAP was not yet
deprecated to avoid compiler warnings.

github-actions bot added the CI label

vszakats changed the title ~~cmake: try fixing HAVE_LDAP_SSL, HAVE_LDAP_URL_PARSE for OpenLDAP~~ cmake: fix HAVE_LDAP_SSL, HAVE_LDAP_URL_PARSE on non-Windows


          eliminate a variable

5a52d95

vszakats closed this in

772f0d8

github-actions bot added the hacktoberfest-accepted label

vszakats added a commit that referenced this pull request


          cmake: tidy-up NOT_NEED_LBER_H detection

4e8a3a1

Follow-up to 772f0d8 #12006

Member Author

vszakats commented Oct 2, 2023

Merged this one minor cleanup too early. The follow-up is in 4e8a3a1.

vszakats deleted the cmake-HAVE_LDAP_SSL-fix branch

October 2, 2023 23:02

vszakats removed the hacktoberfest-accepted label

vszakats added a commit to vszakats/curl that referenced this pull request


          cmake: delete old HAVE_LDAP_URL_PARSE logic

910bf13

Left there by accident after adding proper detection for this.

Follow-up to 772f0d8 curl#12006

Closes #xxxxx

vszakats added a commit to vszakats/curl that referenced this pull request


          cmake: delete old HAVE_LDAP_URL_PARSE logic

196f416

Left there by accident after adding proper detection for this.

Follow-up to 772f0d8 curl#12006

Closes #xxxxx

vszakats mentioned this pull request

cmake: delete old HAVE_LDAP_URL_PARSE logic #12015

Closed

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          curl: re-enable LDAP/LDAP on non-Windows

5e493a9

We disabled it partly because in macOS builds LDAPS was not
enable in CMake builds. Also wrongly assumed this also applies
to autotools builds, but it turned out to be an issue with CMake
builds and fixed here: curl/curl#12006

Try re-enabling to see how it goes.

The other reason for disabling it was that Apple deprecated the
LDAP API in 10.10. We must build for Mavericks to avoid these
warnings. First, let's try without changing the target.

vszakats added a commit that referenced this pull request


          cmake: delete old HAVE_LDAP_URL_PARSE logic

ff9f57b

Left there by accident after adding proper detection for this.

Follow-up to 772f0d8 #12006

Ref: #11964 (effort to sync cmake detections with autotools)

Closes #12015

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          curl: unblock LDAP/LDAP on non-Windows

e23ad41

We disabled it partly because in macOS builds LDAPS was not
enable in CMake builds. Also wrongly assumed this also applies
to autotools builds, but it turned out to be an issue with CMake
builds and fixed here: curl/curl#12006

Try unblocking to see how it goes.

We might still want to block it in the future.

The other reason for disabling it was that Apple deprecated the
LDAP API in 10.10. We must build for Mavericks to avoid these
warnings. First, let's try without changing the target.

UPDATE: It cases warnings, but there are some even without this
feature enabled.

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          disable system LDAP on non-Windows

75c158f

On macOS this auto-detects LDAP (tested with CMake), but not LDAPS.

CMake fix pending: curl/curl#12006

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          curl: re-enable system LDAP/LDAPS on macOS

We disabled it partly because in macOS builds LDAPS was not
enabled in CMake builds. Also wrongly assumed this also applies
to autotools builds, but it turned out this was an issue with
CMake builds and fixed here:
  curl/curl#12006 (in curl 8.4.0)

We this fix, we can enable it.

The other reason for disabling it was that Apple deprecated the
LDAP API in 10.10.

To avoid extra deprecation warnings coming with LDAP on macOS,
target OS X 10.9 Mavericks instead of High Sierra.

The build also runs fine with these warning present.

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          curl: unblock system LDAP/LDAP on macOS

2fc5a1d

We disabled it partly because in macOS builds LDAPS was not
enabled in CMake builds. Also wrongly assumed this also applies
to autotools builds, but it turned out this was an issue with
CMake builds and fixed here:
  curl/curl#12006 (in curl 8.4.0)

We this fix, we can enable it.

The other reason for disabling it was that Apple deprecated the
LDAP API in 10.10.

To avoid extra deprecation warnings coming with LDAP on macOS,
target OS X 10.9 Mavericks instead of High Sierra.

The build also runs fine with these warning present.

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          curl: re-enable system LDAP/LDAPS on macOS

305bc87

We disabled it partly because in macOS CMake builds, LDAPS did
not enable automatically. We wrongly assumed this also applies
to autotools builds, but it turned out this was an issue with
CMake builds only and fixed here:
  curl/curl#12006 (in curl 8.4.0)

With this fix landed, we can re-enable it in CMake and re-enable
with autotools for all curl versions. With 8.3.0 this will only
enable LDAP, without LDAPS.

The other reason for disabling it was that Apple deprecated the
LDAP API in 10.10.

To avoid extra deprecation warnings coming with LDAP on macOS,
we target OS X 10.9 Mavericks instead of High Sierra.

The build also runs fine with these warning present, but we only
enable LDAP/LDAPS on macOS when targeting 10.9.

Also fix `_OSVER` value for `10.[0-9]` inputs.

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          disable system LDAP on non-Windows

33763be

On macOS this auto-detects LDAP (tested with CMake), but not LDAPS.

CMake fix pending: curl/curl#12006

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          curl: re-enable system LDAP/LDAPS on macOS

cea963d

We disabled it partly because in macOS CMake builds, LDAPS did
not enable automatically. We wrongly assumed this also applies
to autotools builds, but it turned out this was an issue with
CMake builds only and fixed here:
  curl/curl#12006 (in curl 8.4.0)

With this fix landed, we can re-enable it in CMake and re-enable
with autotools for all curl versions. With 8.3.0 this will only
enable LDAP, without LDAPS.

The other reason for disabling it was that Apple deprecated the
LDAP API in 10.10.

To avoid extra deprecation warnings coming with LDAP on macOS,
we target OS X 10.9 Mavericks instead of High Sierra.

The build also runs fine with these warning present, but we only
enable LDAP/LDAPS on macOS when targeting 10.9.

Also fix `_OSVER` value for `10.[0-9]` inputs.

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          curl: re-enable system LDAP/LDAPS on macOS

6cbb181

We disabled it partly because in macOS CMake builds, LDAPS did
not enable automatically. We wrongly assumed this also applies
to autotools builds, but it turned out this was an issue with
CMake builds only and fixed here:
  curl/curl#12006 (in curl 8.4.0)

With this fix landed, we can re-enable it in CMake and re-enable
with autotools for all curl versions. With 8.3.0 this will only
enable LDAP, without LDAPS.

The other reason for disabling it was that Apple deprecated the
LDAP API in 10.10.

To avoid extra deprecation warnings coming with LDAP on macOS,
we target OS X 10.9 Mavericks instead of High Sierra.

The build also runs fine with these warning present, but we only
enable LDAP/LDAPS on macOS when targeting 10.9.

Also fix `_OSVER` value for `10.[0-9]` inputs.

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          disable system LDAP on non-Windows

01abf40

On macOS this auto-detects LDAP (tested with CMake), but not LDAPS.

CMake fix pending: curl/curl#12006

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          curl: re-enable system LDAP/LDAPS on macOS

8ad6330

We disabled it partly because in macOS CMake builds, LDAPS did
not enable automatically. We wrongly assumed this also applies
to autotools builds, but it turned out this was an issue with
CMake builds only and fixed here:
  curl/curl#12006 (in curl 8.4.0)

With this fix landed, we can re-enable it in CMake and re-enable
with autotools for all curl versions. With 8.3.0 this will only
enable LDAP, without LDAPS.

The other reason for disabling it was that Apple deprecated the
LDAP API in 10.10.

To avoid extra deprecation warnings coming with LDAP on macOS,
we target OS X 10.9 Mavericks instead of High Sierra.

The build also runs fine with these warning present, but we only
enable LDAP/LDAPS on macOS when targeting 10.9.

Also fix `_OSVER` value for `10.[0-9]` inputs.

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          disable system LDAP on non-Windows

791d3df

On macOS this auto-detects LDAP (tested with CMake), but not LDAPS.

CMake fix pending: curl/curl#12006

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          curl: re-enable system LDAP/LDAPS on macOS

66167af

We disabled it partly because in macOS CMake builds, LDAPS did
not enable automatically. We wrongly assumed this also applies
to autotools builds, but it turned out this was an issue with
CMake builds only and fixed here:
  curl/curl#12006 (in curl 8.4.0)

With this fix landed, we can re-enable it in CMake and re-enable
with autotools for all curl versions. With 8.3.0 this will only
enable LDAP, without LDAPS.

The other reason for disabling it was that Apple deprecated the
LDAP API in 10.10.

To avoid extra deprecation warnings coming with LDAP on macOS,
we target OS X 10.9 Mavericks instead of High Sierra.

The build also runs fine with these warning present, but we only
enable LDAP/LDAPS on macOS when targeting 10.9.

Also fix `_OSVER` value for `10.[0-9]` inputs.

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          disable system LDAP on non-Windows

70bde5a

On macOS this auto-detects LDAP (tested with CMake), but not LDAPS.

CMake fix pending: curl/curl#12006

vszakats added a commit to vszakats/curl-for-win that referenced this pull request


          curl: re-enable system LDAP/LDAPS on macOS

36a8a60

We disabled it partly because in macOS CMake builds, LDAPS did
not enable automatically. We wrongly assumed this also applies
to autotools builds, but it turned out this was an issue with
CMake builds only and fixed here:
  curl/curl#12006 (in curl 8.4.0)

With this fix landed, we can re-enable it in CMake and re-enable
with autotools for all curl versions. With 8.3.0 this will only
enable LDAP, without LDAPS.

The other reason for disabling it was that Apple deprecated the
LDAP API in 10.10.

To avoid extra deprecation warnings coming with LDAP on macOS,
we target OS X 10.9 Mavericks instead of High Sierra.

The build also runs fine with these warning present, but we only
enable LDAP/LDAPS on macOS when targeting 10.9.

Also fix `_OSVER` value for `10.[0-9]` inputs.

vszakats added a commit to curl/curl-for-win that referenced this pull request


          disable system LDAP on non-Windows

On macOS this auto-detects LDAP (tested with CMake), but not LDAPS.

CMake fix pending: curl/curl#12006

vszakats added a commit to curl/curl-for-win that referenced this pull request


          curl: re-enable system LDAP/LDAPS on macOS

058e395

We disabled it partly because in macOS CMake builds, LDAPS did
not enable automatically. We wrongly assumed this also applies
to autotools builds, but it turned out this was an issue with
CMake builds only and fixed here:
  curl/curl#12006 (in curl 8.4.0)

With this fix landed, we can re-enable it in CMake and re-enable
with autotools for all curl versions. With 8.3.0 this will only
enable LDAP, without LDAPS.

The other reason for disabling it was that Apple deprecated the
LDAP API in 10.10.

To avoid extra deprecation warnings coming with LDAP on macOS,
we target OS X 10.9 Mavericks instead of High Sierra.

The build also runs fine with these warning present, but we only
enable LDAP/LDAPS on macOS when targeting 10.9.

Also fix `_OSVER` value for `10.[0-9]` inputs.

zuoxiaofeng pushed a commit to zuoxiaofeng/curl that referenced this pull request


          cmake: fix HAVE_LDAP_SSL, HAVE_LDAP_URL_PARSE on non-Windows

2ce0618

- set `HAVE_LDAP_URL_PARSE` if `ldap_url_parse` function exists.
  Before this patch we set it based it on the presence of `stricmp`,
  which correctly enabled it on e.g. Windows, but was inaccurate for
  other platforms.

- always set `HAVE_LDAP_SSL` if an LDAP backend is detected and
  LDAPS is not explicitly disabled. This mimics autotools behaviour.
  Previously we set it only for Windows LDAP. After this fix, LDAPS is
  correctly enabled in default macOS builds.

- enable LDAP[S] for a CMake macOS CI job. Target OS X 10.9 (Mavericks)
  to avoid deprecation warnings for LDAP API.

- always detect `HAVE_LDAP_SSL_H`, even with LDAPS explicitly disabled.
  This doesn't make much sense, but let's do it to sync behaviour with
  autotools.

- fix benign typo in variable name.

Ref: curl#11964 (effort to sync cmake detections with autotools)

Closes curl#12006

zuoxiaofeng pushed a commit to zuoxiaofeng/curl that referenced this pull request


          cmake: tidy-up NOT_NEED_LBER_H detection

1ea624d

Follow-up to 772f0d8 curl#12006

zuoxiaofeng pushed a commit to zuoxiaofeng/curl that referenced this pull request


          cmake: delete old HAVE_LDAP_URL_PARSE logic

ee675ef

Left there by accident after adding proper detection for this.

Follow-up to 772f0d8 curl#12006

Ref: curl#11964 (effort to sync cmake detections with autotools)

Closes curl#12015

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