Adding declspec export attribute to header functions

[Honeybunch]

Hi there! I'm trying to get libmysofa added to vcpkg so it will be available to more users. Also steam-audio uses libmysofa and I'd like to use steam-audio from vcpkg 😄

See: microsoft/vcpkg#38368

For my vcpkg port I ran into an interesting problem that this patch addresses. Without this any downstream project that depends on libmysofa will fail to properly link to the library on windows. Not sure if this is the way you'd like to fix this. We could also do a .def file for the linker to mark these functions as part of the export .lib.

[umlaeute]

or build the library with gcc (rather than msvc) which has an option to export all the symbols.
(the library should then be usable even in an MSVC project)

[umlaeute]

sidenote: i'm not at all opposed against adding a MYSOFA_EXPORT.
however, i think this should not be limited to Windows.

in general i'm a big fan of using hidden symbols by default for libraries (only symbols explicitly exported via MYSOFA_EXPORT are made public), which is well supported by CMake (which even has functions to generate a header file with a proper MYSOFA_EXPORT definition).

so yes, but please do not stop at MSVC.

[Honeybunch]

or build the library with gcc (rather than msvc) which has an option to export all the symbols. (the library should then be usable even in an MSVC project)

In vcpkg the default compiler triplet for windows uses msvc so getting it to compile with msvc would be preferred

[Honeybunch]

in general i'm a big fan of using hidden symbols by default for libraries (only symbols explicitly exported via MYSOFA_EXPORT are made public), which is well supported by CMake (which even has functions to generate a header file with a proper MYSOFA_EXPORT definition).

What the library currently does is generate a mysofa_export.h header from a config as you suggested. The problem is that this header is not installed so it can't be safely just included in mysofa.h. Which is only part of the problem, each function still needs to be marked up with MYSOFA_EXPORT define so that when a downstream project includes the header it knows that the functions have the declspec(import) property.

[umlaeute]

The problem is that this header is not installed

so the solution is to install the header, no?

this of course would complicate the includes from a simple single mysofa.h to two dependent headers - which might not be desired.

[Honeybunch]

I tried the vcpkg x64-mingw-dynamic and x64-mingw-static triplets and I can't find any issues with them. So it seems like this change is compatible with non-msvc compilers on Windows

[Honeybunch]

this of course would complicate the includes from a simple single mysofa.h to two dependent headers - which might not be desired.

This is why I didn't do that. I wasn't sure what the preference was so I did it the way that I thought would be the least invasive

[umlaeute]

So it seems like this change is compatible with non-msvc compilers on Windows

my comment was not about introducing incompatibilities, but about doing it properly rather than halfway: other systems (e.g. gcc on Linux) can benefit from the change as well.
but then MYSOFA_EXPORT needs to have a value on these systems.

also note the inconsistent behaviour mentioned in #220 (review)

[ColdenCullen]

It sounds like a solution to this is to define MYSOFA_EXPORT to __attribute__((visibility(default))) and compile with -fvisibility=hidden on non-Windows platforms (per this StackOverflow). @umlaeute I think that satisfies what you're asking for?

[Honeybunch]

So TIL about cmake's generate_export_header

Looks like the way cmake expects you to use this is, yeah, to make sure the generated header is installed so downstream projects can include it.

[Honeybunch]

I updated my port to use this and then made sure a downstream project could consume it and it looks to work

[umlaeute]

@ColdenCullen said

It sounds like a solution to this is to define MYSOFA_EXPORT to __attribute__((visibility(default))) and

Yes, that's what I've been trying to say.

[Honeybunch]

Any feedback for the latest version of this? Seems like I was wrong and the right answer was to include and install the header that Cmake generated

[Honeybunch]

Hate to necro an old PR but wanted to swing back to this. What's the current feedback for this change as it currently exists?

[hoene]

looks reasonable for me.