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.

Sign up for GitHub

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: add native pkg-config detection for remaining Find modules #15408

Closed
wants to merge 17 commits into from
Closed

cmake: add native pkg-config detection for remaining Find modules #15408

wants to merge 17 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 25, 2024 •
edited
Loading

brotli, c-ares, libpsl, libssh2, nghttp2, nghttp3, ntgcp2, zstd.

Also:

Add workaround for CMake reporting successful libssh2 detection, but
leaving the header directory empty, and causing libssh2.h not found
while compiling. It happens when pkgconf is not detecting libssh2
dependency libcrypto in Homebrew after brew unlink openssl (as in
GHA/macos). The workaround is to require a non-empty header directory
to consider the detection successful. This workaround may need to be
tweaked and/or applied to other Find modules.

Follow-up to 7bab201 #15193

w/o whitespace: https://github.com/curl/curl/pull/15408/files?w=1

@vszakats vszakats added cmake feature-window A merge of this requires an open feature window labels Oct 25, 2024
@github-actions github-actions bot added the build label Oct 25, 2024
@vszakats vszakats marked this pull request as draft October 25, 2024 00:37
@vszakats vszakats force-pushed the cm-find-pkgconfig-2 branch 4 times, most recently from b2b30b6 to 352f651 Compare October 25, 2024 01:28
@vszakats vszakats marked this pull request as ready for review October 25, 2024 01:28
@dfandrich
Copy link
Contributor

Analysis of PR #15408 at 352f651e:

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

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

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

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

Generated by Testclutch

@vszakats vszakats mentioned this pull request Oct 28, 2024
Closed
1 task
This was referenced Nov 12, 2024
Closed
Closed
@vszakats vszakats force-pushed the cm-find-pkgconfig-2 branch from 681ee77 to 690f075 Compare December 16, 2024 19:09
@vszakats
Copy link
Member Author

vszakats commented Dec 17, 2024 •
edited
Loading

What seems to be happening on macOS, is that CMake is reporting
libssh2 to be found via LIBSSH2_FOUND, but, since the libssh2
dependency libcrypto isn't found by pkgconf (because we do
brew unlink openssl), CMake leaves the LIBSSH2_INCLUDE_DIRS
variable empty. Resulting in libssh2.h not found and a broken build.
(It replicates locally on a recent Arm machine.)

This wasn't happening earlier when working on this PR. It might be
a fallout of Homebrew switching from pkg-config to pkgconf
since then.

I see 3 ways to fix it:

  • stop doing brew unlink openssl. But it's there for a reason and
    this may not be trivial to work around. It would also not fix this for
    others doing this trick (required to use OpenSSL forks).
  • require LIBSSH2_INCLUDE_DIRS for a successful detection
    via pkg-config. Leaving detection to CMake-native.
    → WENT WITH THIS
  • use LIBSSH2_INCLUDEDIR if LIBSSH2_INCLUDE_DIRS is
    empty. The former holds the correct header path. A bit of shaky,
    may of may not have side-effects.

Then the question remains if this is a systemic issue that may
happen with other dependencies too? and should we apply the
fix to all Find modules?

@vszakats vszakats force-pushed the cm-find-pkgconfig-2 branch from 690f075 to 2528e11 Compare December 17, 2024 01:05
@vszakats vszakats closed this in 39c741b Dec 17, 2024
vszakats added a commit that referenced this pull request Dec 17, 2024
@vszakats
Revert: cmake: enable CURL_USE_PKGCONFIG for cross-MINGW
290607f 
This reverts commit 39c06f7 #15005.

Combined with most Find modules now supporting `pkg-config`
(39c741b #15408) this change made
mingw-cross builds fragile by picking up OS-native components. Also
adding `/usr/include` to the header path, confusing feature detection.
vszakats added a commit to vszakats/curl that referenced this pull request Dec 17, 2024
@vszakats
cmake: extend AUTO option to brotli, zstd
9300ed9 
Probably requires curl#15408
@vszakats vszakats deleted the cm-find-pkgconfig-2 branch December 18, 2024 23:54
vszakats added a commit that referenced this pull request Dec 22, 2024
@vszakats
cmake: delete accidental debug message [ci skip]
23f4b93 
Follow-up to 39c741b #15408
vszakats added a commit to vszakats/curl that referenced this pull request Dec 24, 2024
@vszakats
libpsl: protect against half-detection
e3707bb 
Same as seen before with libssh2.

Follow-up to 39c741b curl#15408

Might be due to broken local installation. This also doesn't help:
export PKG_CONFIG_PATH="$(brew --prefix icu4c)/lib/pkgconfig"
where the expression resolves to the non-existing:
/usr/local/opt/icu4c@76/lib/pkgconfig

Seen on macOS with Homebrew:
```
-- Checking for module 'libpsl'
--   Found libpsl, version 0.21.5
Package icu-uc was not found in the pkg-config search path.
Perhaps you should add the directory containing `icu-uc.pc'
to the PKG_CONFIG_PATH environment variable
Package 'icu-uc', required by 'libpsl', not found
Package icu-uc was not found in the pkg-config search path.
Perhaps you should add the directory containing `icu-uc.pc'
to the PKG_CONFIG_PATH environment variable
Package 'icu-uc', required by 'libpsl', not found
Package icu-uc was not found in the pkg-config search path.
Perhaps you should add the directory containing `icu-uc.pc'
to the PKG_CONFIG_PATH environment variable
Package 'icu-uc', required by 'libpsl', not found
Package icu-uc was not found in the pkg-config search path.
Perhaps you should add the directory containing `icu-uc.pc'
to the PKG_CONFIG_PATH environment variable
Package 'icu-uc', required by 'libpsl', not found
Package icu-uc was not found in the pkg-config search path.
Perhaps you should add the directory containing `icu-uc.pc'
to the PKG_CONFIG_PATH environment variable
Package 'icu-uc', required by 'libpsl', not found
Package icu-uc was not found in the pkg-config search path.
Perhaps you should add the directory containing `icu-uc.pc'
to the PKG_CONFIG_PATH environment variable
Package 'icu-uc', required by 'libpsl', not found
Package icu-uc was not found in the pkg-config search path.
Perhaps you should add the directory containing `icu-uc.pc'
to the PKG_CONFIG_PATH environment variable
Package 'icu-uc', required by 'libpsl', not found
Package icu-uc was not found in the pkg-config search path.
Perhaps you should add the directory containing `icu-uc.pc'
to the PKG_CONFIG_PATH environment variable
Package 'icu-uc', required by 'libpsl', not found
Package icu-uc was not found in the pkg-config search path.
Perhaps you should add the directory containing `icu-uc.pc'
to the PKG_CONFIG_PATH environment variable
Package 'icu-uc', required by 'libpsl', not found
Package icu-uc was not found in the pkg-config search path.
Perhaps you should add the directory containing `icu-uc.pc'
to the PKG_CONFIG_PATH environment variable
Package 'icu-uc', required by 'libpsl', not found
-- Found Libpsl (via pkg-config):  (found version "0.21.5")
[...]
In file included from curl/_bld/lib/CMakeFiles/libcurl_static.dir/Unity/unity_0_c.c:4:
In file included from curl/lib/altsvc.c:32:
In file included from curl/lib/urldata.h:145:
curl/lib/psl.h:28:10: fatal error: 'libpsl.h' file not found
         ^~~~~~~~~~
1 error generated.
```
@vszakats vszakats mentioned this pull request Dec 24, 2024
Closed
vszakats added a commit that referenced this pull request Dec 25, 2024
@vszakats
cmake/FindLibpsl: protect against pkg-config "half-detection"
98f4191 
Same issue as seen before with libssh2: `libpsl`'s pkg-config module
depends on another module, but that's not found. CMake ends up reporting
`LIBPSL_FOUND=YES`, while leaving `LIBPSL_INCLUDE_DIRS` empty. Then
the build fails to find `psl.h`.

The missing dependency in this case is `icu4c`, which is "keg-only",
meaning it's not exposed in the default Homebrew header, pkg-config,
lib, etc locations. It must be added to the `PKG_CONFIG_PATH` env, as
suggested by the warnings messages of `pkgconf`.

To avoid this fallout, let's ensure that `LIBPSL_INCLUDE_DIRS` is
non-empty when detecting via `pkg-config` and fall back to the CMake
detection method otherwise.

This was an issue till Homebrew libpsl 0.21.5_1, fixed in 0.21.5_2, that
no longer depends on `icu4c`.

Example log:
```
-- Checking for module 'libpsl'
--   Found libpsl, version 0.21.5
Package icu-uc was not found in the pkg-config search path.
Perhaps you should add the directory containing `icu-uc.pc'
to the PKG_CONFIG_PATH environment variable
Package 'icu-uc', required by 'libpsl', not found
[...]
-- Found Libpsl (via pkg-config):  (found version "0.21.5")
[...]
In file included from curl/_bld/lib/CMakeFiles/libcurl_static.dir/Unity/unity_0_c.c:4:
In file included from curl/lib/altsvc.c:32:
In file included from curl/lib/urldata.h:145:
curl/lib/psl.h:28:10: fatal error: 'libpsl.h' file not found
         ^~~~~~~~~~
1 error generated.
```

Follow-up to 39c741b #15408
Closes #15827
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build cmake feature-window A merge of this requires an open feature window
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vszakats @dfandrich