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: picky-linker fixes for openssl, ZLIB, H3 and more #10857

Closed
wants to merge 3 commits into from
Closed

cmake: picky-linker fixes for openssl, ZLIB, H3 and more #10857

wants to merge 3 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Mar 28, 2023 •
edited

  • fix HTTP/3 support detection with OpenSSL/quictls built with ZLIB.
    (Requires curl be built with ZLIB option also.)

  • fix HTTP/3 support detection with OpenSSL/quictls/LibreSSL and ld
    linker on Windows.

  • fix HTTP/3 support detection with wolfSSL to automatically add
    ws2_32 to the lib list on Windows. For all linkers.

  • reposition ZLIB (and other compression) detection after TLS
    detection, but before calling HTTP/3-support detection via
    CheckQuicSupportInOpenSSL.

    May be a regression from ebef55a
    May fix undefined reference for zlib symbols when building with static dependencies and BUILD_SHARED_LIBS=OFF #10832 (Reported-by: Micah Snyder)

    This also seems to fix an odd case, where OpenSSL/quictls is correctly
    detected, but its header path is not set while compiling, breaking
    build at src/curl_ntlm_core.c. Reason for this remains undiscovered.

  • satisfy "picky" linkers such as ld with MinGW, that are highly
    sensitive to lib order, by also adding brotli to the beginning of the
    lib list.

  • satisfy "picky" linkers by adding certain Windows systems libs to
    the lib list for OpenSSL/LibreSSL. (Might need additional ones for
    other forks, such as pthread for BoringSSL.)

Note: It'd make sense to always add ws2_32, crypt32 (except
Windows App targets perhaps?), bcrypt (except old-mingw!) on Windows
at this point. They are almost always required, and if some aren't,
they are ignored by the linker with no effect on final binaries.

Closes #10857

With these, a CMake build with gcc/ld almost works ;) Now gsasl lib is in the wrong position ¯\(ツ)/¯. → Worked around using CMAKE_C_STANDARD_LIBRARIES to pass it.

@vszakats vszakats mentioned this pull request Mar 28, 2023
Closed
@vszakats vszakats changed the title cmake: picky linker fixes for openssl, ZLIB, H3 and others cmake: picky-linker fixes for openssl, ZLIB, H3 and others Mar 28, 2023
@jay
Copy link
Member

jay commented Mar 28, 2023

not against this but should we be making concessions for these users that are compiling static builds? things can always break as the static library changes dependencies. for example what if the reporter did not enable zlib for curl, then isn't the problem the same, his build will fail because his ssl still requires zlib but cmake does not know that?

shouldn't the burden be on him to specify the requirements if there's no PKG_CONFIG="pkg-config --static" or other autotools way that cmake can't use. even then something like nghttp2 at least on windows you have to specify CPPFLAGS="-DNGHTTP2_STATICLIB" as well. it seems static builds are a lot of trouble this way.

@vszakats
Copy link
Member Author

@jay: It'd definitely help to state that. Static builds are extremely fragile and labour-intensive, curl-for-win being the living proof for that. An infinite amount of time can be spent tracking the variations and their changes. E.g. BoringSSL still needs phtread and indeed, those extra macros here and there. Also, CMake and autotools are actively working against making static linking work.

On Windows, I think it'd also help if we added the base set of Windows system libs that are almost always required. They are a common source of problems there.

vszakats added a commit to curl/curl-for-win that referenced this pull request Mar 29, 2023
@vszakats
curl-cmake.sh: prepare for upstream patch [ci skip]
60006cd 
That makes it easier to enable certain components by fixing detection
logic.

Ref: curl/curl#10857
@bagder
Copy link
Member

bagder commented Mar 29, 2023

I am in favor of clearly documenting that doing working completely static curl builds is not the responsibility of the curl build scripts. To set the expectations right.

@vszakats
Copy link
Member Author

vszakats commented Mar 29, 2023 •
edited

Here's one reply (and some more below that), to a static linking report/question received earlier: curl/curl-for-win#39 (comment) (This was Windows-specific, which is a particularly problematic platform for this, but some of it is likely common to all.)

And an unresolved discussion with a similar request: curl/curl-for-win#41

These were for final apps linking libcurl, which is kind of the same as linking curl itself.

@jay
Copy link
Member

jay commented Mar 29, 2023 •
edited

in mingw-w64 I used to do it like below for autotools. i don't know if something similar is possible with cmake.

./buildconf
CPPFLAGS="-DNGHTTP2_STATICLIB" LDFLAGS="-static" PKG_CONFIG="pkg-config --static" ./configure ...
make LDFLAGS="-static -all-static" V=1

then tests

make test LDFLAGS="-static -all-static" V=1

then i would actually run the tests from msys2 shell instead of mingw-w64 shell for some reason. it's been a while since i've built like that.

@vszakats
Copy link
Member Author

@jay: Thanks, I did not know about the -all-static option. Maybe it can help dropping some hacks from curl-for-win autotools builds. The MSYS2 env helps a lot indeed. Some believe that the resulting binary also requires MSYS2, but that is not the case (with -static-libgcc / -static-libstdc++) binaries.

@vszakats
cmake: HTTP/3 detection fixes with openssl and forks
91e5121 
- fix HTTP/3 support detection with openssl/quictls built with ZLIB

- fix HTTP/3 support detection with openssl/quictls/libressl and
  `ld` linker on Windows

- reposition ZLIB (and other compression) detection _after_ TLS
  detection, but before calling HTTP/3-support detection via
  `CheckQuicSupportInOpenSSL`. This seems to fix an odd case, where
  OpenSSL (quictls) is correctly detected, but its header path is
  not set while compiling, breaking it at `src/curl_ntlm_core.c`.

  Regression from ebef55a
  May fix curl#10832
@vszakats
fix linking openssl and brotli with "picky" linkers
fbf4fa9
@vszakats
add ws2_32 for wolfssl H3 detection
217cfda
@vszakats vszakats changed the title cmake: picky-linker fixes for openssl, ZLIB, H3 and others cmake: picky-linker fixes for openssl, ZLIB, H3 and more Mar 29, 2023
@vszakats vszakats closed this in 1e3319a Mar 30, 2023
@vszakats vszakats deleted the cmake-openssl-zlib branch March 30, 2023 08:56
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
@vszakats @bch
cmake: picky-linker fixes for openssl, ZLIB, H3 and more
1c00fbb 
- fix HTTP/3 support detection with OpenSSL/quictls built with ZLIB.
  (Requires curl be built with ZLIB option also.)

- fix HTTP/3 support detection with OpenSSL/quictls/LibreSSL and `ld`
  linker on Windows.

- fix HTTP/3 support detection with wolfSSL to automatically add
  `ws2_32` to the lib list on Windows. For all linkers.

- reposition ZLIB (and other compression) detection _after_ TLS
  detection, but before calling HTTP/3-support detection via
  `CheckQuicSupportInOpenSSL`.

  May be a regression from ebef55a
  May fix curl#10832 (Reported-by: Micah Snyder)

  This also seems to fix an odd case, where OpenSSL/quictls is correctly
  detected, but its header path is not set while compiling, breaking
  build at `src/curl_ntlm_core.c`. Reason for this remains undiscovered.

- satisfy "picky" linkers such as `ld` with MinGW, that are highly
  sensitive to lib order, by also adding brotli to the beginning of the
  lib list.

- satisfy "picky" linkers by adding certain Windows systems libs to
  the lib list for OpenSSL/LibreSSL. (Might need additional ones for
  other forks, such as `pthread` for BoringSSL.)

Note: It'd make sense to _always_ add `ws2_32`, `crypt32` (except
Windows App targets perhaps?), `bcrypt` (except old-mingw!) on Windows
at this point. They are almost always required, and if some aren't,
they are ignored by the linker with no effect on final binaries.

Closes curl#10857
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

undefined reference for zlib symbols when building with static dependencies and BUILD_SHARED_LIBS=OFF
3 participants
@vszakats @jay @bagder