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: lib CURL_STATICLIB fixes (Windows) #11914

Closed
wants to merge 8 commits into from
Closed

cmake: lib CURL_STATICLIB fixes (Windows) #11914

wants to merge 8 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Sep 22, 2023 •
edited

  • always define CURL_STATICLIB when building libcurl for Windows.

    This disables __declspec(dllexport) for exported libcurl symbols.
    In normal mode (hide symbols) these exported symbols are specified
    via libcurl.def. When not hiding symbols, all symbols are exported
    by default.

    Regression from 1199308

    Fixes When building a static library project for Visual Studio with CMake, the preprocessor variable CURL_STATICLIB does not get set #11844

  • fix to omit libcurl.def when not hiding private symbols.

    Regression from 2ebc74c

  • fix ENABLED_DEBUG=ON + shared curl tool Windows builds by also
    omitting libcurl.def in this case, and exporting all symbols
    instead. This ensures that a shared curl tool can access all debug
    functions which are not normally exported from libcurl DLL.

  • delete INTERFACE_COMPILE_DEFINITIONS "CURL_STATICLIB" for "objects"
    target.

    Follow-up to 2ebc74c

  • delete duplicate BUILDING_LIBCURL definitions.

  • fix HIDES_CURL_PRIVATE_SYMBOLS to not overwrite earlier build settings.

    Follow-up to 1199308

Closes #11914

@vszakats vszakats added the cmake label Sep 22, 2023
@github-actions github-actions bot added the build label Sep 22, 2023
@vszakats vszakats added the Windows Windows-specific label Sep 22, 2023
@vszakats
Copy link
Member Author

vszakats commented Sep 22, 2023 •
edited

A new issue came up: memdebug.c functions cannot be accessed in ENABLED_DEBUG=ON + shared curl.exe builds. Because these functions are missing from libcurl.def and after this PR CURL_STATICLIB leaves them unexported.

https://ci.appveyor.com/project/curlorg/curl/build/job/8olkqly0nb6yb2v9 (CMake, VS2008, Release x86, Schannel)
https://ci.appveyor.com/project/curlorg/curl/build/job/x61dq69gdtsq7fvx (CMake, VS2022, Release x64, OpenSSL, WebSockets, Unity)

Plus there is curl_easy_perform_ev() going on a different track than memdebug functions, and ending up missing too.

@github-actions github-actions bot added the CI Continuous Integration label Sep 22, 2023
@vszakats
Copy link
Member Author

vszakats commented Sep 22, 2023 •
edited

Solved the ENABLED_DEBUG=ON + shared tool case by introducing libcurl-debug.def.

This relies on CMake merging the two .def files before passing to the linker. Linkers may not natively support multiple .def files (couldn't yet figure out if this needs a specific CMake version), e.g. lld honors only the last one passed. It means the Makefile.mk use needs further fixing.

UPDATE: Seems to require CMake 3.12.0, so this solution is doubly no good.

@vszakats vszakats force-pushed the cmake-lib-fixups branch 3 times, most recently from c0a1076 to 91a8ced Compare September 23, 2023 01:50
@github-actions github-actions bot added the tests label Sep 23, 2023
@vszakats
Copy link
Member Author

vszakats commented Sep 23, 2023 •
edited

Solved this one by extending libcurl-debug.def to include all standard libcurl API definitions as well. Plus a test for it. This fixes debug + shared tool builds with both older CMake and Makefile.mk builds.

This perhaps isn't the most elegant solution, but I cannot think of a better one. Other alternatives may be to:

  • Split off debug functions to a separate lib and link that statically always (this would be likely more complex.)
  • Find a CMake feature that is able to set list of exports with a regex (curl_*) for all supported compilers (like autotools does).
  • Either:
    • drop Windows support from Makefile.mk.
    • find an lld/ld option to limit exports to a regex (like libtool's "-export-symbols-regex '^curl_.*'") [There don't seem to be such option.]
    • dynamically generate .def via our existing Perl script (making Perl a hard requirement there) [This worked fine locally as an experiment].
  • Export all symbols for debug builds.

@vszakats
Copy link
Member Author

vszakats commented Sep 23, 2023 •
edited

Next issues:

  1. non-Windows symbol curl_dbg_socketpair is picked up by tests/extern-scan.pl.
  2. tests/extern-scan.pl misses to pick up curl_dbg_malloc, curl_dbg_calloc, curl_dbg_realloc.

UPDATE: fixed.

@vszakats
Copy link
Member Author

vszakats commented Sep 23, 2023 •
edited

Tending to prefer the option 'Export all symbols for debug builds' as a reasonable compromise. This makes it unnecessary to roll a libcurl-debug.def.

@vszakats
Copy link
Member Author

vszakats commented Sep 23, 2023 •
edited

Strange error from a single CI job for the test added in this PR:

=== Start of file stderr3203
 Unsuccessful open on filename containing newline at ../../tests/extern-scan.pl line 88.
 readline() on closed filehandle H at ../../tests/extern-scan.pl line 90.
 Unsuccessful open on filename containing newline at ../../tests/extern-scan.pl line 88.
 readline() on closed filehandle H at ../../tests/extern-scan.pl line 90.
=== End of file stderr3203

Ref: https://github.com/curl/curl/actions/runs/6281395226/job/17062947662?pr=11914#step:11:3161

@vszakats
cmake: always define CURL_STATICLIB when building libcurl for Windows
fd7bb90 
This disables `__declspec(dllexport)` for exported libcurl symbols.
In normal mode (hide symbols) these exported symbols are specified
via `libcurl.def`. When not hiding symbols, all symbols are exported
by default.

Regression from 1199308

Fixes curl#11844
@vszakats vszakats removed tests CI Continuous Integration build labels Sep 23, 2023
@vszakats vszakats mentioned this pull request Sep 23, 2023
Closed
@vszakats vszakats changed the title cmake: libcurl build fixups cmake: libcurl CURL_STATICLIB fixes Sep 23, 2023
@vszakats vszakats changed the title cmake: libcurl CURL_STATICLIB fixes cmake: libcurl CURL_STATICLIB fixes (Windows) Sep 23, 2023
@vszakats vszakats changed the title cmake: libcurl CURL_STATICLIB fixes (Windows) cmake: lib CURL_STATICLIB fixes (Windows) Sep 23, 2023
@github-actions github-actions bot added the build label Sep 23, 2023
jay
jay reviewed Sep 23, 2023
View reviewed changes
lib/CMakeLists.txt Outdated Show resolved Hide resolved
@vszakats
cmake: fix to export all symbols in debug builds
0b0bb76 
We already used `WINDOWS_EXPORT_ALL_SYMBOLS` in
`CMake/CurlSymbolHiding.cmake` for MSVC, but that was limited to cases
when hiding symbols explicitly. We need to do this also when disabling
symbol hiding automtically for debug builds.
vszakats added a commit that referenced this pull request Sep 23, 2023
@vszakats
Makefile.mk: always set CURL_STATICLIB for lib (Windows)
6a85659 
Also fix to export all symbols in Windows debug builds, making
`-debug-dyn` builds work with `-DCURL_STATICLIB` set.

Ref: #11914 (same for CMake)

Closes #11924
@github-actions github-actions bot added the CI Continuous Integration label Sep 24, 2023
@vszakats vszakats force-pushed the cmake-lib-fixups branch 2 times, most recently from 84d1072 to 75655f1 Compare September 24, 2023 00:53
@vszakats
cmake: disable symbol hiding options for Windows debug builds
177faa7 
Symbol hiding options are enabled by default and with clang/gcc
toolchains it adds the `-fvisibility=hidden` linker option. We
need to disable it to make debug builds export all symbols from
libcurl DLL with these compilers.

This case misses CI coverage. (mingw-w64, debug with shared lib only
(or static+shared and BUILD_STATIC_CURL=OFF) while linking
libcurl DLL statically with dependencies exporting their symbols.)
@vszakats
cmake: disable symbol hiding for debug builds the earliest
399d17b 
This ensures that any symbol-hiding compiler option is used
everywhere and it also makes it unnecessary to repeat the
MSVC-specific option `WINDOWS_EXPORT_ALL_SYMBOLS=ON`.
@vszakats vszakats force-pushed the cmake-lib-fixups branch 4 times, most recently from c81157e to 399d17b Compare September 24, 2023 17:10
@vszakats vszakats removed cmdline tool CI Continuous Integration labels Sep 24, 2023
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
@vszakats @ptitSeb
Makefile.mk: always set CURL_STATICLIB for lib (Windows)
8673be4 
Also fix to export all symbols in Windows debug builds, making
`-debug-dyn` builds work with `-DCURL_STATICLIB` set.

Ref: curl#11914 (same for CMake)

Closes curl#11924
@vszakats vszakats closed this in a8ebde9 Sep 25, 2023
@vszakats vszakats deleted the cmake-lib-fixups branch September 25, 2023 22:11
@dietssa dietssa mentioned this pull request Oct 13, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jay jay jay left review comments

Assignees
No one assigned
Labels
build cmake regression Windows Windows-specific
Milestone
No milestone
2 participants
@vszakats @jay