condition for using Version_script and the win32 def file
Closed this issue · 24 comments
Our current configure.ac checks gcc (and only gcc) being the compiler
when deciding whether to use Version_script or the win32 def file:
https://github.com/libsndfile/libsamplerate/blob/master/configure.ac#L265
However, the linker should be one checked. Do you think the following
is correct: @SoapGentoo, @evpobr? If it is I can make a PR for it.
diff --git a/configure.ac b/configure.ac
index 0e2117c..4691e09 100644
--- a/configure.ac
+++ b/configure.ac
@@ -260,14 +260,13 @@ AS_IF([test "x$enable_werror" = "xyes"], [
AX_APPEND_COMPILE_FLAGS([-W -Wstrict-prototypes -Wmissing-prototypes -Wall -Waggregate-return -Wcast-align -Wcast-qual -Wnested-externs -Wshadow -Wpointer-arith], [CFLAGS])
dnl ====================================================================================
-dnl GCC stuff.
+dnl Exported symbols control.
-AS_IF([test "x$ax_cv_c_compiler_vendor" = "xgnu"], [
- # OS specific tweaks.
+AS_IF([test "x$lt_cv_prog_gnu_ld" = "xyes"], [
AS_CASE([${host_os}],
[mingw*|cygwin*], [
SHLIB_VERSION_ARG="-export-symbols \$(top_srcdir)/Win32/libsamplerate-0.def"],
- [linux*|kfreebsd*-gnu*|gnu*], [
+ [*], [
SHLIB_VERSION_ARG="-Wl,--version-script=\$(top_builddir)/src/Version_script"
])
])
If Clang is not detected by this path it makes sense.
If Clang is not detected by this path
It certainly is not detected (had noticed that in #147)
This seems like the right idea. I'd consider going even further, dumping the whole AS_CASE around it and getting rid of the -export-symbols cruft in favour of the LIBSAMPLERATE_DLL_* approach. @sezero what do you think? You'd end up with just
AS_IF([test "x$lt_cv_prog_gnu_ld" = "xyes"], [
SHLIB_VERSION_ARG="-Wl,--version-script=\$(top_builddir)/src/Version_script"
])
which should be fine, because GNU ld doesn't run on macOS, so nothing should break.
This seems like the right idea. I'd consider going even further, dumping the whole
AS_CASEaround it and getting rid of the-export-symbolscruft in favour of theLIBSAMPLERATE_DLL_*approach. @sezero what do you think? You'd end up with justAS_IF([test "x$lt_cv_prog_gnu_ld" = "xyes"], [ SHLIB_VERSION_ARG="-Wl,--version-script=\$(top_builddir)/src/Version_script" ])which should be fine, because GNU ld doesn't run on macOS, so nothing should break.
Will test building for windows in an hour
Surprisingly, MinGW build using only Version_script seems to work:
Same symbols are exported, just with different order. Tested ld versions
are 2.20 and 2.21.90.20111031
I don't yet know of any possible setups that this would fail.
The def file will still be needed for e.g. MSVC builds, though.
The def file will still be needed for e.g. MSVC builds, though.
This was my suggestion. Ditch the .def file, an annotate all exported functions with LIBSAMPLERATE_DLL_PUBLIC decorators.
This was my suggestion. Ditch the
.deffile, an annotate all exported functions withLIBSAMPLERATE_DLL_PUBLICdecorators.
I have concerns for it:
- It might require library users to re-adjust themselves with export macros
if samplerate.h is touched for the public api prototypes. - IIRC, MSVC might not behave if samplerate.h is kept intact but only private
implementations are marked with export macros. - Or: All my concerns are bogus, don't know, haven't tried MSVC or MinGW
with such solution.
@evpobr doesn't MSVC already complain when linking a dll, given that all public functions actually lack a __declspec(dllimport) decorator?
So you're fine with the slight inefficiency of not getting the __declspec(dllimport) optimization?
Oops, i forgot out def export functions not only by name, but also index:
src_new @1
src_delete @2
src_get_name @10
src_get_description @11
src_get_version @12
src_process @20
src_reset @21
src_set_ratio @22
src_clone @23
src_error @30
src_strerror @31
src_simple @40
src_is_valid_ratio @50
src_callback_new @60
src_callback_read @61
src_short_to_float_array @70
src_float_to_short_array @71
src_int_to_float_array @80
src_float_to_int_array @81
src_get_channels @90
Reading Raymond's blog, I wonder whether exporting those ordinals is the right approach.
Reading Raymond's blog, I wonder whether exporting those ordinals is the right approach.
It is obsolete stuff from 16-bit times, but it is definitely part of Win32 ABI of libsamplerate.
so dumping the .def file and switching to LIBSAMPLERATE_DLL_PUBLIC decorators will be a likely ABI break to anyone relying on the ordinals for linking. Is this a genuine concern in Win32 days? Shouldn't people be consuming the import libraries instead of generating their own anyways?
Very unlikely. But this is definitely ABI break by definition.
My verdict is that in the foreseeable future we will release version 1.0 and there we will remove Autotools and switch to CMake with the GenerateExportHeader module.
If we put a big fat warning on the release page and tell people not to reverse-engineering import libs and always use our import libs because the ordinals might change, are you willing to entertain that idea? Getting rid of Win32/libsamplerate-0.def would simplify things a lot.
Getting rid of
Win32/libsamplerate-0.defwould simplify things a lot.
I agree, but that doesn't give users anything. Don't forget that Libsamplerate is a widely used library. With Homebrew alone, we have ~ 500K installations over the past year. Is it worth making life difficult for people just for the sake of what is so convenient for us?
Is this a genuine concern in Win32 days? Shouldn't people be consuming the import libraries instead of generating their own anyways?
Hyrum's Law.
Wait, does homebrew run on Windows? Do people really use libsamplerate through Hombrew on Windows?
Hyrum's law is everywhere, I'm just wondering whether this is something we promised. In this case, I think the tradeoff is worth it, because simplifying the codebase and build systems is worth more than someone being able to rely on stable ordinals in Win32 linking.
Wait, does homebrew run on Windows? Do people really use libsamplerate through Hombrew on Windows?
It was just an example. And for macOS, not the most widely used platform.
In this case, I think the tradeoff is worth it, because simplifying the codebase and build systems is worth more than someone being able to rely on stable ordinals in Win32 linking.
I don't see much simplification. We now use two methods for exporting symbols. And we also have to use define to denote non-static private functions.
In any case, we will not be able to switch to compiler pragmas for export, even if we also use it for Windows. The problem with Linux remains. Our linker script contains very specific stuff, besides listing public functions. This will be an unambiguous, one hundred percent ABI break.
So I will go with my original suggestion
@evpobr read again. I'm suggesting dropping Win32/libsamplerate-0.def, not src/Version_script, the latter which we can't, due to ABI breaks on Linux. Dropping Win32/libsamplerate-0.def and going with manual decorations on the exported functions will neither break the ABI on Linux, nor macOS. It will only break the ABI for people on Win32 using the ordinal numbers for linking, which I consider marginal, because introducing decorators on the declarations means people get improved calling performance due to __declspec(dllimport). Microsoft also recommends macro-based/GenerateExportHeader-based idioms over DEF files.
@sezero feel free to send in a PR.
@SoapGentoo, I took a break, thought a little and now it seems to me that you are right. Make a pull request, I'll try how the new method will work on Windows.