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: migrate dependency detections to Find modules #14555

Closed
wants to merge 47 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Aug 15, 2024

For: libgsasl, libidn2, libssh, libuv.

The new Find modules retain using pkg-config natively, not as a "hint"
for the CMake-native detection. Of the pre-existing Find modules, only
FindNettle, and FindGSS (with customized code) work this way. Align
detection code for the new modules and add version detection for the
CMake-native paths.

Also, add CMake-native detection for libgsasl.

The remaining outlier in CMakeLists.txt is GnuTLS, which has
a CMake built-in Find module, but which lacks pkg-config support,
required for vcpkg. It remains unchanged.

Another part-outlier is libssh, which keeps requiring the trick
find_package(libssh CONFIG QUIET) for reasons I could not yet figure
out.


  • decide on Find module naming scheme.
  • disable pkg-config if custom config is set via <NAME>_INCLUDE_DIR / <NAME>_LIBRARY? (Separate PR?)
  • decide if double-checking of idn2 libs and header is really necessary. [→ PROBABLY NOT. DELETED]
  • robustify version detection with if(EXISTS), more flexible regexp and synced var names. → cmake: sync up version detection in Find modules #14572
  • once there is more experience with this method and it turns out superior, consider changing the other Find modules. If this turns out worse, consider moving back to the "hint" way with these.

@vszakats vszakats added cmake feature-window A merge of this requires an open feature window labels Aug 15, 2024
@github-actions github-actions bot added the build label Aug 15, 2024
@vszakats vszakats changed the title cmake: move dependency detections to Find modules [WIP] cmake: move dependency detections to Find modules Aug 15, 2024
@vszakats vszakats marked this pull request as draft August 15, 2024 10:56
@vszakats vszakats force-pushed the cm-move-to-find branch 2 times, most recently from 1397916 to 7be485f Compare August 15, 2024 13:24
@talregev
Copy link
Contributor

Maybe in other PR we can have krb5 and it version?

@vszakats
Copy link
Member Author

Version detection already works with krb5, or did you mean something else?:
https://github.com/curl/curl/actions/runs/10408884975/job/28827262755#step:6:62

@talregev
Copy link
Contributor

talregev commented Aug 15, 2024

Version detection already works with krb5, or did you mean something else?: https://github.com/curl/curl/actions/runs/10408884975/job/28827262755#step:6:62

I meant that it will be in here.
https://github.com/curl/curl/actions/runs/10408884975/job/28827262755#step:10:10

@talregev
Copy link
Contributor

Version detection already works with krb5, or did you mean something else?: https://github.com/curl/curl/actions/runs/10408884975/job/28827262755#step:6:62

That a good a nice step. I like it!

@vszakats
Copy link
Member Author

https://github.com/curl/curl/actions/runs/10408884975/job/28827262755#step:10:10

Ah, got it, to the -V output. These seem pretty old and steady components and their versions have little impact on functionality, perhaps that's why it's not there? It's out of scope for this PR, but you may open an Issue/Discussion about it.

@vszakats vszakats changed the title [WIP] cmake: move dependency detections to Find modules cmake: move dependency detections to Find modules Aug 16, 2024
@vszakats vszakats removed the feature-window A merge of this requires an open feature window label Aug 16, 2024
@vszakats vszakats marked this pull request as ready for review August 16, 2024 18:23
@talregev
Copy link
Contributor

https://github.com/curl/curl/actions/runs/10408884975/job/28827262755#step:10:10

Ah, got it, to the -V output. These seem pretty old and steady components and their versions have little impact on functionality, perhaps that's why it's not there? It's out of scope for this PR, but you may open an Issue/Discussion about it.

I think is very important, because now when I got curl from ubuntu, I don't know which library was compile to get gsasl-api feature, and what the version it there. If users know, they can easly report to ubuntu (or other distro) or get decision if to use that curl.
I already create an issue about it, and it on known issue here on curl.
I agree that we need to deal with that in separate PR.
Also I was thinking to add find_krb5, and find_hemidal and move the logic to that files from main cmake.
We can also do it in separate PR.

@vszakats
Copy link
Member Author

Also I was thinking to add find_krb5, and find_hemidal and move the logic to that files from main cmake.
We can also do it in separate PR.

Do you mean to split FindGSS into FindHeimdal and Findkrb5, or?

@talregev
Copy link
Contributor

talregev commented Aug 17, 2024

Also I was thinking to add find_krb5, and find_hemidal and move the logic to that files from main cmake.
We can also do it in separate PR.

Do you mean to split FindGSS into FindHeimdal and Findkrb5, or?

If it already happen in FindGSS that is good, and not need to change.

@talregev
Copy link
Contributor

I see that file now, and that is a good direction.

In the future, if we see that the logic is large in FindGSS, we can think to split. Maybe it will be more easy to handle?
What happen if curl detect both hemidal and krb5. What it will choose?

@vszakats
Copy link
Member Author

Also I was thinking to add find_krb5, and find_hemidal and move the logic to that files from main cmake.
We can also do it in separate PR.

Do you mean to split FindGSS into FindHeimdal and Findkrb5, or?

If it already happen in FindGSS that is good, and not need to change.

It does. pkg-config detection wasn't working up until recently.
They are now both tested in GHA for macos/linux and Heimdal on NetBSD.

They are certainly not over-tested, so more tests and reports are welcome.

@vszakats vszakats force-pushed the cm-move-to-find branch 4 times, most recently from b8cd6b9 to b3d0977 Compare August 19, 2024 12:28
Also re-sort Makefile.am list.
…nfig detections

Making the call doesn't give anything useful. The only difference
is that it will defined the `<original_case>_FOUND` variable over
the fully uppercase version. And also omits the standard 'Found' log
messages.

On the other hand it will show a misleading error message when the
dependency is not found, but saying that the `_INCLUDE_DIRS` and
`_LIBRARIES` vars are missing, while in fact we use `_INCLUDE_DIR`
and `_LIBRARY` for that.

Re-add the log message manually for pkg-config detections to smoothen
out that difference.
@vszakats vszakats changed the title cmake: move dependency detections to Find modules cmake: migrate dependency detections to Find modules Aug 20, 2024
@vszakats vszakats closed this in 422696f Aug 20, 2024
@vszakats vszakats deleted the cm-move-to-find branch August 20, 2024 09:42
vszakats added a commit that referenced this pull request Aug 21, 2024
Found in local tests.

Follow-up to 422696f #14555
which added CMake-native detection.
vszakats added a commit that referenced this pull request Aug 21, 2024
Drop `find_package(libssh CONFIG)` detection method in favour of
the Find module that supports both `pkg-config`, and CMake-native
(since #14555) detection.

This aligns `libssh` detection with other dependencies. It makes the
build honor custom configuration via `LIBSSH_INCLUDE_DIR`,
`LIBSSH_LIBRARY`.

Also enable libssh in a GHA/macos cmake job for build coverage.

Fixing:
- curl-for-win requiring a hack to configure libssh:
  https://github.com/curl/curl-for-win/blob/4f9acbed92fd4aac0e874c9a591bec7d621cd9f2/curl.sh#L255-L263
- after #14555, GHA/windows gnutls vcpkg job no longer auto-detected
  libssh, due to a regression missing to enable libssh when
  found via `find_package(libssh CONFIG)`.
  Ref: https://github.com/curl/curl/actions/runs/10470138955/job/28994650338

Follow-up to 422696f #14555

Closes #14614
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