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

common: Fix symbol export using MSVC #49

Merged
merged 2 commits into from
Mar 27, 2024
Merged

common: Fix symbol export using MSVC #49

merged 2 commits into from
Mar 27, 2024

Conversation

asoulier
Copy link
Collaborator

No description provided.

Copy link

@amyspark amyspark left a comment

Choose a reason for hiding this comment

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

Just a small nit (for reasons of consistency under Windows), otherwise LGTM.

src/common.h Outdated Show resolved Hide resolved
@amyspark
Copy link

amyspark commented Mar 19, 2024

There's a further nit I had not noticed @asoulier, as regards importing shared v. static libraries.

  • For shared libraries, you need to mark all the declarations in lc3.h et al. as __declspec(dllimport) for downstream consumers, otherwise MSVC will complain about __imp_ pivot symbols not found.
  • For static libraries, you need to turn off the macro altogether.

Those could be tackled at once with a combination of defines for the shared build (-DDLL_EXPORTS, for instance?) and for the build process itself (-DSTATIC_BUILD in c_static_args with Meson >= 1.3.0, otherwise you may need to reject get_option('default_library') == 'both').

src/common.h Outdated
@@ -86,8 +86,11 @@
* Change the visibility of interface functions
*/

#define LC3_EXPORT \
__attribute__((visibility ("default")))
#ifdef _WIN32

Choose a reason for hiding this comment

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

The bit in #49 (comment) as regards static libraries on Windows still needs addressing.

@asoulier asoulier force-pushed the msvc branch 16 times, most recently from e1e578a to 38e0b88 Compare March 21, 2024 21:12
@asoulier
Copy link
Collaborator Author

@amyspark I have added a CI to check compilation with MSVC.
Do you confirms that runs now correctly with MSVC ?

Thanks.

@amyspark
Copy link

@asoulier I think the only bit missing is the ability to build liblc3 statically too. Currently we'd need to hardcode the --buildtype flag or use MinGW to sidestep this issue.

@asoulier
Copy link
Collaborator Author

asoulier commented Mar 21, 2024

I changed the buildtype to release (It looks like it was compiling in debug by default).
I would not change the behavior of compiling dynamic library by default.
If I set the both option, the static library becomes installed on linux systems (Looks not useful for me).
If you want to generate both, can you have a try with:

meson setup --wipe --default-library both build

This works for me on linux.

Copy link

@amyspark amyspark left a comment

Choose a reason for hiding this comment

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

I tested again and MSVC 17.10 Preview 2, GCC 13.2 Rev 5 (MSYS2 UCRT64) and Clang 17 (CLANG64) are all happy.

*/

#ifdef _WIN32
#define LC3_EXPORT __declspec(dllexport)

Choose a reason for hiding this comment

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

i wonder if LC3_EXPORT could be moved to the src functions rather than a header which is public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

MSVC needs a match between functions declaration and definitions.
So the header declarations needs to be decorated with __declspec(dllexport).

Choose a reason for hiding this comment

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

I see, thanks

@asoulier
Copy link
Collaborator Author

@amyspark Thanks for your checks!

@asoulier asoulier closed this Mar 27, 2024
@asoulier asoulier reopened this Mar 27, 2024
@asoulier asoulier merged commit bfea2c0 into main Mar 27, 2024
15 checks passed
@asoulier asoulier deleted the msvc branch March 27, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants