Skip to content

cmake: rename LDAP dependency config variables to match Find modules #15255

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

Closed
wants to merge 4 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 10, 2024

  • CMAKE_LDAP_LIBLDAP_LIBRARY
  • CMAKE_LBER_LIBLDAP_LBER_LIBRARY
  • CMAKE_LDAP_INCLUDE_DIRLDAP_INCLUDE_DIR

This entry was previously titled:
cmake: allow manual configuration for LDAP

It also intended to allow manual override for these variables, but
it turns out this was already possible without this change. This
leaves the renames as the notable updates.

Cherry-picked from #15157


@github-actions github-actions bot added the build label Oct 10, 2024
@vszakats vszakats changed the title cmake: allow manual configuration for LDAP dependency cmake: allow manual configuration for LDAP libraries Oct 10, 2024
@vszakats vszakats changed the title cmake: allow manual configuration for LDAP libraries cmake: allow manual configuration for LDAP Oct 10, 2024
@vszakats vszakats closed this in 2c90f7f Oct 10, 2024
@vszakats vszakats deleted the cm-ldap-config-fixes branch October 10, 2024 20:51
@dg0yt
Copy link
Contributor

dg0yt commented Nov 5, 2024

Consider an update of the Release Notes:

This change didn't enable manual configuration, but it changed the name of the configuration variables.

Noticed in preparing the update of the vcpkg port which made use of the old variables.
Reminder: CMake cache variables are input variables. Patterns like

    if(NOT DEFINED LDAP_LIBRARY)
      set(LDAP_LIBRARY "ldap" CACHE STRING "Name or full path to ldap library")
    endif()

don't provide added value unless you want to ensure plain CMake variables precedence over cache variables.

FTR vcpkg had

if(NOT CURL_DISABLE_LDAP AND NOT WIN32)
    find_package(PkgConfig REQUIRED)
    pkg_check_modules(LDAP REQUIRED ldap)
    set(HAVE_LIBLDAP 1)
    set(CMAKE_LDAP_INCLUDE_DIR "${LDAP_INCLUDE_DIRS}")
    set(CMAKE_LDAP_LIB "${LDAP_LINK_LIBRARIES}" CACHE STRING "")
    pkg_check_modules(LBER REQUIRED lber)
    set(HAVE_LIBLBER 1)
    set(CMAKE_LBER_LIB "${LBER_LINK_LIBRARIES}" CACHE STRING "")
endif()

not as a patch but as CMAKE_PROJECT_INCLUDE.

dg0yt added a commit to dg0yt/vcpkg that referenced this pull request Nov 5, 2024
@vszakats vszakats changed the title cmake: allow manual configuration for LDAP cmake: rename LDAP dependency config variables to match Find modules Nov 5, 2024
vszakats added a commit that referenced this pull request Nov 5, 2024
@vszakats
Copy link
Member Author

vszakats commented Nov 5, 2024

Thanks. That's unexpected. A Set(VAR "") does not actually set VAR to "".
Luckily this construct was seldom used in the code elsewhere.

I'd not have renamed knowing that, but a planned PR to convert to a Find
module also needs it for consistency: https://github.com/curl/curl/pull/15273/files?w=1
It would sync behaviour of these variables with all the others.

Updated this PR's message/title and the entry in RELEASE-NOTES. I hope
it's an improvement: 1b4897f

@dg0yt
Copy link
Contributor

dg0yt commented Nov 6, 2024

A Set(VAR "") does not actually set VAR to "".

It is important to realize the difference between cache variables (global, cached, input) and normal variables (scoped), and what takes precedence.

A set(VAR "") does set the normal variable VAR in the current scope to "". That's even the easy part, and it is good. But...

An unset(VAR) or set(VAR ) doesn't just unset the normal variable VAR in the current scope. It removes that idenifier's specialization from the current scope. Now ${VAR} may expand to another definition (cache or parent scope)!

That's why set(VAR ) is the wrong way to initialize variables. What is needed is set(VAR ""). (Unsetting is something which can be usetful at the end of a macro or find modules.) CURL examples which I consider wrong (noticed yesterday):

curl/CMakeLists.txt

Lines 2087 to 2090 in 380790b

unset(_implicit_libs)
if(NOT MINGW AND NOT UNIX)
set(_implicit_libs ${CMAKE_C_IMPLICIT_LINK_LIBRARIES})
endif()

@dg0yt
Copy link
Contributor

dg0yt commented Nov 6, 2024

Updated this PR's message/title and the entry in RELEASE-NOTES. I hope
it's an improvement: 1b4897f

LGTM.

vszakats added a commit that referenced this pull request Dec 16, 2024
Also add cleanup `unset()`s where missing.

Reported-by: Kai Pastor
Bug: #15255 (comment)
Follow-up to 8b09138 #14610

Closes #15497
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
Via these configuration values:
- `LDAP_LIBRARY`
- `LDAP_LBER_LIBRARY`
- `LDAP_INCLUDE_DIR`

Following the naming scheme used in `Find` modules.

Cherry-picked from curl#15157
Closes curl#15255
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
Also add cleanup `unset()`s where missing.

Reported-by: Kai Pastor
Bug: curl#15255 (comment)
Follow-up to 8b09138 curl#14610

Closes curl#15497
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants