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: sync GSS config code with other deps #15545

Closed
wants to merge 9 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Nov 11, 2024

  • stop passing explicit libpaths via CMAKE_SHARED_LINKER_FLAGS and
    CMAKE_EXE_LINKER_FLAGS. link_directories() is doing that already.
  • use curl_required_libpaths() to pass libpaths to the feature test.
    Reported-by: Daniel Engberg
    Fixes cmake: don't use quotes for GSS-API paths #15536
    Also fixes GSS feature detection with non-gcc/clang compilers,
    such as MSVC.
  • add libpaths to CURL_LIBPATHS.
  • move GSS_CFLAGS, GSS_LDFLAGS stringifications to FindGSS.
    To match the CFLAGS format returned by the rest of Find modules.
  • reorder calls to match other dependencies.
  • don't extend system LDFLAGS when FindGSS did not return any.
  • ignore LDFLAGS when detecting GSS via pkg-config. LDFLAGS holds
    a copy of libpaths and libs in this case. Ignore those to avoid these
    duplicates making into libcurl.pc and curl-config. Also syncing
    behavior with other Find modules which also ignore raw LDFLAGS.
  • ignore raw LDFLAGS coming from krb5-config --libs. FindGSS
    no longer returns dependency-specific LDFLAGS after this. Syncing
    behavior with other Find modules.
  • reduce scope of checker state push/pop/set.

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

- drop `CMAKE_SHARED_LINKER_FLAGS` and `CMAKE_EXE_LINKER_FLAGS` that
  seem redundant over `link_directories()`
- add libpaths to `CURL_LIBPATHS`.
- use `curl_required_libpaths()` to pass the libpaths to the
  feature test.

Fixes curl#15536
Syncing this with other Find modules.

With pkg-config this contains a copy of the libpath and the lib list.

This causes them to appear as duplicates in `libcurl.pc` `Libs.private`
and `curl-config`.
Say where these values are coming from. LDFLAGS is --libs.
E.g. on macOS these were: -dynamic -Wl,-search_paths_first

Let's readd these if there is anything critical that's reported missing,
otherwise sync with other Find modules that never bothered with misc
dependency-specific LDFLAGS.
@dfandrich
Copy link
Contributor

Analysis of PR #15545 at 97a8fcd5:

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

Generated by Testclutch

@diizzyy
Copy link
Contributor

diizzyy commented Nov 14, 2024

Using GSS in base, path is now missing is pc file

-- FindGSS krb5-config --cflags: -I/usr/include
-- FindGSS krb5-config --libs: -L/usr/lib -lgssapi -lgssapi_krb5 -lheimntlm -lkrb5 -lhx509 -lcom_err -lcrypto -lasn1 -lwind -lheimbase -lroken -lcrypt -pthread
-- Found GSS: Heimdal (found version "FreeBSD heimdal 1.1.0")

@vszakats
Copy link
Member Author

vszakats commented Nov 14, 2024

Using GSS in base, path is now missing is pc file

-- FindGSS krb5-config --cflags: -I/usr/include
-- FindGSS krb5-config --libs: -L/usr/lib -lgssapi -lgssapi_krb5 -lheimntlm -lkrb5 -lhx509 -lcom_err -lcrypto -lasn1 -lwind -lheimbase -lroken -lcrypt -pthread
-- Found GSS: Heimdal (found version "FreeBSD heimdal 1.1.0")

curl's CMake do not add system libpaths to the .pc file. As detected via CMAKE_SYSTEM_PREFIX_PATH with some extra logic applied.

Before this patch this logic was accidentally circumvented for libpaths detected in this single GSS detection codepath, that's why it was there before.

Is this causing any fallouts in your case?

@diizzyy
Copy link
Contributor

diizzyy commented Nov 14, 2024

Works for me (tm) but it's inconsistent with GNU Autotools?

@vszakats
Copy link
Member Author

Works for me (tm) but it's inconsistent with GNU Autotools?

Thanks for your tests!

Yes, it seems inconsistent indeed.

Assuming the CMake way is accurate, it would be nice in autotools too, though so far no one complained. Figuring out the system paths in autotools is also kind of difficult and manual. Something for a future PR.

@vszakats vszakats closed this in e0e93d4 Nov 14, 2024
@vszakats vszakats deleted the cm-gssapi-config-tidy branch November 14, 2024 20:53
vszakats pushed a commit to vszakats/curl that referenced this pull request Nov 15, 2024
Add gssapi to openssl job

waiting for:
- [x] curl#15564
- [x] curl#15545

Closes curl#15549
vszakats pushed a commit to vszakats/curl that referenced this pull request Nov 15, 2024
Add gssapi to openssl job

waiting for:
- [x] curl#15564
- [x] curl#15545

Closes curl#15549
vszakats pushed a commit to vszakats/curl that referenced this pull request Nov 15, 2024
Add gssapi to openssl job

waiting for:
- [x] curl#15564
- [x] curl#15545

Closes curl#15549
vszakats pushed a commit to vszakats/curl that referenced this pull request Nov 15, 2024
Add gssapi to openssl job

waiting for:
- [x] curl#15564
- [x] curl#15545

Closes curl#15549
vszakats pushed a commit to vszakats/curl that referenced this pull request Nov 15, 2024
Add gssapi to openssl job

waiting for:
- [x] curl#15564
- [x] curl#15545

Closes curl#15549
vszakats pushed a commit that referenced this pull request Nov 15, 2024
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.

3 participants