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

Export symbols for all compilers on Windows #2630

Merged
merged 1 commit into from Jan 21, 2023
Merged

Export symbols for all compilers on Windows #2630

merged 1 commit into from Jan 21, 2023

Conversation

ChrisThrasher
Copy link
Collaborator

@ChrisThrasher ChrisThrasher commented Jan 20, 2023

Description

Linking Catch2 v3.2.1 as a shared library when using Clang on Windows fails because symbols only get exported for MSVC (and MSVC-like compilers) but not for Clang. Removing this conditional means that Clang on Windows now gets this too. This should have no effect on compilers on non-Windows platforms. I believe we could move this out of the if(BUILD_SHARED_LIBS) block as well since it will just do nothing if you're building static libraries.

Here are two CI pipeline runs from SFML illustrating before and after applying this patch.

Before:
https://github.com/ChrisThrasher/SFML/actions/runs/3946193148

After
https://github.com/ChrisThrasher/SFML/actions/runs/3972002714

@codecov
Copy link

codecov bot commented Jan 21, 2023

Codecov Report

Merging #2630 (baab9e8) into devel (2d3c971) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            devel    #2630   +/-   ##
=======================================
  Coverage   91.15%   91.15%           
=======================================
  Files         187      187           
  Lines        7691     7691           
=======================================
  Hits         7010     7010           
  Misses        681      681           

@horenmar
Copy link
Member

Thanks.

I believe we could move this out of the if(BUILD_SHARED_LIBS) block as well since it will just do nothing if you're building static libraries.

After reading the docs, I agree. But since the conditional still has to be there (the behaviour of static libraries compiled with default visibility and hidden visibility is different when they are linked into dynamic library later), I think it is fine to leave it the way it is now.

@horenmar horenmar merged commit dd36f83 into catchorg:devel Jan 21, 2023
@horenmar horenmar added the Building and Packaging Issues affecting build/packaging scripts and utilities label Jan 21, 2023
@ChrisThrasher ChrisThrasher deleted the export_all_symbols branch January 21, 2023 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Building and Packaging Issues affecting build/packaging scripts and utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants