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

Jump to bottom

autotools: allow --enable-symbol-hiding with windows #9586

Closed
wants to merge 1 commit into from
Closed

autotools: allow --enable-symbol-hiding with windows #9586

wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Sep 25, 2022 •
edited

This local autotools logic was put in place in 9e24b9c (in 2012) which disabled it for Windows unconditionally. Testing reveals that it actually works with tested toolchains (mingw-w64 and CI ones), so let's allow this build feature on that platform. Bringing this in sync with CMake, which already supported this.

Closes #9586

@vszakats vszakats marked this pull request as draft September 25, 2022 04:27
@vszakats
Copy link
Member Author

vszakats commented Sep 25, 2022 •
edited

Tested fine with mingw-w64. Let's test with the legacy mingw and the others.

UPDATE 1: They passed.
UPDATE 2: --enable-symbol-hiding is also the default setting and it's not explicitly disabled in these tests, meaning the option was actually active.

@vszakats vszakats marked this pull request as ready for review September 25, 2022 10:33
@vszakats
autotools: allow --enable-symbol-hiding with windows
4e2a131 
This local autotools logic was put in place in
9e24b9c (in 2012) which disabled it for
Windows unconditionally. Testing reveals that it actually works with
tested toolchains (mingw-w64 and CI ones), so let's allow this build
feature on that platform. Bringing this in sync with CMake, which already
supported this.

Closes curl#9586
@vszakats vszakats added build Windows Windows-specific labels Sep 25, 2022
vszakats added a commit to curl/curl-for-win that referenced this pull request Sep 25, 2022
@vszakats
curl-autotools.sh: replace libcurl.def patch with upstream fix [ci skip]
9b71378 
Fix the upstream autotools option `--enable-symbol-hiding` on Windows,
then use that instead of our hand rolled solution which also involves
patching the build system.

This brings down the number of local patch two just 2. (Still two more
than ideal though.)

Ref: curl/curl#9586
@jay
Copy link
Member

jay commented Sep 25, 2022

But does it actually do anything on windows?

curl/include/curl/curl.h

Lines 112 to 126 in 93d0928

#ifdef CURL_STATICLIB
# define CURL_EXTERN
#elif defined(CURL_WIN32) || defined(__SYMBIAN32__) || \
(__has_declspec_attribute(dllexport) && \
__has_declspec_attribute(dllimport))
# if defined(BUILDING_LIBCURL)
# define CURL_EXTERN __declspec(dllexport)
# else
# define CURL_EXTERN __declspec(dllimport)
# endif
#elif defined(BUILDING_LIBCURL) && defined(CURL_HIDDEN_SYMBOLS)
# define CURL_EXTERN CURL_EXTERN_SYMBOL
#else
# define CURL_EXTERN
#endif

@vszakats
Copy link
Member Author

vszakats commented Sep 25, 2022 •
edited

Sure, unless I'm fatally misunderstanding something. After the patch, libcurl.dll no longer exports every existing public function linked to it, only the curl API. This let me drop the manual solution using a pre-rolled .def file (as used in Makefile.m32 builds). Ref this curl-for-win patch: curl/curl-for-win@9b71378

The compiler macro is a no-op on Windows, but there is extra logic run around the linking phase, and it may also trigger some linker options. I didn't investigate the internals further.

@vszakats
Copy link
Member Author

Example build here: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/44876993

jay
jay approved these changes Sep 25, 2022
View reviewed changes
Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is extra logic run around the linking phase

ah ok

@vszakats vszakats closed this in 61c7cca Sep 25, 2022
@vszakats vszakats deleted the autotools-windows-hidesymbols branch September 25, 2022 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jay jay jay approved these changes

Assignees
No one assigned
Labels
build Windows Windows-specific
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vszakats @jay