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

cmake: replace check_include_file_concat() for LDAP and GSS detection #15157

Closed
wants to merge 3 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 4, 2024

Replace check_include_file_concat() with check_include_file() in
GSS/LDAP detection to avoid these headers spilling into subsequent
feature checks.

  • For LDAP, reverse detection order to match with ./configure.
    Though, in current LDAP packages ldap.h does include lber.h.

  • For GSS, align header detection logic with ./configure, where
    gssapi/gssapi_generic.h might require gssapi/gssapi.h, and
    gssapi/gssapi_krb5.h might require both.

Ref: #436


@vszakats
Copy link
Member Author

vszakats commented Oct 5, 2024

That's a deep rabbit-hole of issues...

  • Optional dependencies should not use check_include_file_concat() for header detection and polluting the global header list with dependency headers. (also happens with GSS)
  • There are curl-specific customization variables that are namespaced CMAKE_*: CMAKE_LDAP_LIB, CMAKE_LBER_LIB, CURL_LDAP_INCLUDE_DIR.
  • They use non-standard naming scheme, and probably should not be namespaced at all (or to CURL_*).
  • These variables do not accept input via command-line, only by updating them in cache? or were they meant for internal use only?
  • Core OpenLDAP detection should probably be in its own Find module.
  • ldap is missed from the pkg-config dependency list (also in autotools): LIBCURL_PC_REQUIRES_PRIVATE.
  • custom curl CMake code should probably never (or only with a strong reason with comments) depend on CMake built-in globals: CMAKE_REQUIRED_FLAGS, CMAKE_REQUIRED_DEFINITIONS, CMAKE_REQUIRED_LINK_OPTIONS, CMAKE_REQUIRED_LIBRARIES, CMAKE_REQUIRED_INCLUDES, CMAKE_EXTRA_INCLUDE_FILES. This will have to untangled and see what sets depends on which uses.
  • OpenLDAP (detection) is very lightly tested in CI.

Related commit/PR(s): #436

https://cmake.org/cmake/help/latest/module/CMakePushCheckState.html

edit: Most of these were addressed via separate PRs.

@vszakats vszakats marked this pull request as draft October 5, 2024 10:43
@vszakats vszakats changed the title cmake: use cmake_{push,pop}_check_state() in LDAP detection cmake: use cmake_{push,pop}_check_state() more, and other tidy-ups Oct 5, 2024
@dfandrich
Copy link
Contributor

Analysis of PR #15157 at bc0992c7:

Test 593 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

Generated by Testclutch

@github-actions github-actions bot added the CI Continuous Integration label Oct 5, 2024
@vszakats vszakats marked this pull request as ready for review October 5, 2024 15:04
@github-actions github-actions bot added the tests label Oct 5, 2024
@vszakats vszakats removed the tests label Oct 5, 2024
@vszakats vszakats changed the title cmake: use cmake_{push,pop}_check_state() more, and other tidy-ups cmake: use cmake_push_check_state() more, other tidy-ups Oct 6, 2024
@vszakats vszakats marked this pull request as draft October 7, 2024 11:06
vszakats added a commit to vszakats/curl that referenced this pull request Oct 10, 2024
vszakats added a commit that referenced this pull request Oct 10, 2024
Enclose
`CMAKE_EXTRA_INCLUDE_FILES`,
`CMAKE_REQUIRED_DEFINITIONS`,
`CMAKE_REQUIRED_FLAGS`,
`CMAKE_REQUIRED_INCLUDES`,
`CMAKE_REQUIRED_LIBRARIES`,
`CMAKE_REQUIRED_LINK_OPTIONS`,
settings within `cmake_push_check_state()`/`cmake_pop_check_state()`
calls. It prevents spilling them into other feature checks. It also
replaces manual resets found in some places (which can have
the undesired side-effect of destroying values meant for global use.)

Cherry-picked from #15157
Closes #15251
vszakats added a commit that referenced this pull request Oct 10, 2024
…DES`

It was done for `zlib`, `brotli`, `libpsl`, `libssh2`, `wolfssh`
(a copy-paste case for `wolfssh`).

Feature detections should not rely by default on dependency headers.
There is no evidence they do now. If it becomes necessary, headers
should added for the duration of the feature check.

Ref: 118977f
Cherry-picked from #15157
Closes #15252
vszakats added a commit that referenced this pull request Oct 10, 2024
Add comments saying when we want values set in feature check option
variables to apply to all feature checks, globally. These are currently:
`ws2_32` and `socket` libraries, and `-D_WIN32_WINNT=` macro.

Also use `list(APPEND ...)` for the libraries to avoid overwriting
potentially existing values.

Cherry-picked from #15157
Closes #15253
ldap.h includes lber.h. If you know of a system where this is not
the case, let us know.
include gssapi/gssapi.h when checking for gssapi_generic.h/gssapi_krb5.h

To continue doing what ./configure does.
@vszakats vszakats changed the title cmake: use cmake_push_check_state() more, other tidy-ups cmake: replace check_include_file_concat() for LDAP and GSS detection Oct 10, 2024
@vszakats vszakats marked this pull request as ready for review October 10, 2024 18:29
vszakats added a commit that referenced this pull request Oct 10, 2024
Via these configuration values:
- `LDAP_LIBRARY`
- `LDAP_LBER_LIBRARY`
- `LDAP_INCLUDE_DIR`

Following the naming scheme used in `Find` modules.

Cherry-picked from #15157
Closes #15255
@vszakats vszakats closed this in 91d451b Oct 10, 2024
@vszakats vszakats deleted the cm-use-pushpop branch October 10, 2024 20:52
vszakats added a commit to vszakats/curl that referenced this pull request Oct 12, 2024
Regression from rebase mistake in
91d451b curl#15157
vszakats added a commit to vszakats/curl that referenced this pull request Oct 12, 2024
Regression from rebase mistake in
91d451b curl#15157
vszakats added a commit that referenced this pull request Oct 12, 2024
- limit `SIZEOF_SA_FAMILY_T` detection to non-Windows.
- make sure `sys/socket.h` exists before detecting `SIZEOF_SA_FAMILY_T`.
- limit `mach_absolute_time()` detection to `APPLE`. Drop from Windows
  pre-cache.
- skip `HAVE_LIBSOCKET` detection for Windows, drop pre-cached value.
- drop redundant pre-cached `HAVE_LIBZ` for Windows.
- `curl_required_libpaths()`: stop accepting multiple arguments.
  To prepare for `CMAKE_REQUIRED_LINK_DIRECTORIES` support.
  Follow-up to 7bab201 #15193
- GSS: fix recent rebase mistakes:
  - fix variable name.
  - do not add a header twice.
  Follow-up to 91d451b #15157
- GSS: quote a variable.

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

Successfully merging this pull request may close these issues.

2 participants