fix installation with MSYS2+mingw-w64

[vtorri]

No description provided.

[Cyan4973]

The make install test failed with this patch on msys2+mingw64:
https://ci.appveyor.com/project/YannCollet/zstd-p0yf0/builds/29261481/job/8s4soe51220db3qo#L185

cp ../lib/dll/libzstd.dll .
cp: cannot stat '../lib/dll/libzstd.dll': No such file or directory

[vtorri]

i don't know what you did but the dll in my patch is :

dll/libzstd-$(LIBVER_MAJOR).dll

that is common usage to add major version for parallal installation after api or abi break (in that case, major version should (must) be increased)

[Cyan4973]

appveyorTest is a branch which just adds a make install test to the recipe :
0c3b8c4

The test fails at make install stage, which is expected without this patch.

The test result linked above is the result of merging this patch with appveyorTest.
It was expected to pass the make install test, but seems to fail before reaching that stage,
presumable during the tests of the dynamic library, which have been working well so far :
https://ci.appveyor.com/project/YannCollet/zstd-p0yf0/builds/29261293/job/ibefne01q07ur7g0#L183 .

I presume this PR is incompatible with the dynamic library test for Windows,
and is likely failing at this line :
https://github.com/facebook/zstd/blob/dev/tests/Makefile#L227

[vtorri]

i guess so. Just add -$(LIBVER_MAJOR)

[Cyan4973]

I don't think that's the best course of action.
What the test shows is that there is an expectation that a file named libzstd.dll exist and can be linked to. I have no idea how many users may depend on it, but this is something I'm not willing to discover by breaking their application.

On Unix, the convention is to have multiple links to take care of this situation.
The installer will create libzstd.so.1.4.4, then link it from libzstd.so.1 and from libzstd.so.
This way, it's possible to link with -lzstd, which basically means "pick the latest version available".
Then, in some long term future, presuming a breaking change happens in the API, it becomes possible to link to libzstd.so.1 instead, to ensure that it won't link against a potential libzstd.so.2.

I guess it makes sense to use the same convention for Windows, even with the lack of link. Therefore, both versions should exist.

[vtorri]

the name of the dll is not important, what is important is the name of the import library, which is libzstd.dll.a

see : https://sourceware.org/binutils/docs/ld/WIN32.html section "direct linking to a dll"

so when you pass -lzstd, ld will find libzstd.dll.a and everything will be fine

mimic what linux does on windows is not good because they are different beasts.

[vtorri]

about https://github.com/facebook/zstd/blob/dev/tests/Makefile#L228

you should do instead :

$(CC) $(FLAGS) $< -o $@$(EXT) -DZSTD_DLL_IMPORT=1 -L../lib/dll -lzstd

and you shoud export PATH with ../lib/dll so that libzstd-1.dll is found when the test binary is executed

[vtorri]

or you can both copy import lib and dll to ., and pass -L. -lzstd

btw, ZSTD_DLL_IMPORT is completely useless.

[Cyan4973]

I'm fine with changing the test for a "better" one, if "better" can be documented.
The provided link "ld an win32" is useful for this objective.

what is important is the name of the import library, which is libzstd.dll.a

This is very confusing. *.a is the accepted suffix for static libraries in Unix world,
but here, this "import" library seems to remain a dynamic one.

Why are there 2 differently named dll now ?
One for development, one for runtime ?

ZSTD_DLL_IMPORT is completely useless.

Technically, it does prefix all public symbols with __declspec(dllimport).

This was added a long time ago, and is documented as having a "positive impact on performance" :
https://github.com/facebook/zstd/blob/dev/lib/zstd.h#L33

But we have very little knowledge of this subtlety. All I know is that it adds a form of "complexity", and even if it's a small one, it better pays for its presence.

If this option is not useful, then maybe explain why (the code comment is wrong then ?), and then we should get rid of it, to simplify the source code and its maintenance.

[vtorri]

@Cyan4973 "This is very confusing. *.a is the accepted suffix for static libraries in Unix world," but Windows IS NOT Unix. You try to convince yourself that unix OS and Windows must have the same behavior and should do the same things. It is not the case. For example : do you think that LD_PRELOAD or LD_LIBRARY_PATH support exists on Windows. No, it does not.

But, for years, gcc developpers try to simplify the life of Windows coders so that using the same options to gcc (compiler and linker) works correctly on all systems. There is one differences on windows, though, you must use -Wl,--out-implib OR using a .def file, but imho, using a .def file is very error prone as you can easily forget a public function in it.

Technically, a .a on unix is an archive created with the ar archiver (hence the .a suffix). On windows the import lib is the list of stub (empty) functions that appear in the dll. The import library is NOT a dll. Visual Studio uses .lib for import lib. But it also uses .lib for static lib, which is very confusing. So, for around 20 years, the import lib has the .dll.a suffix on Windows when built with gcc (more precisely, it's libtool which decided to name the import lib like that) and the ld linker with correctly do things if you pass -lzstd, as mentioned in the link i have provided.

another reference :

http://www.mingw.org/wiki/sampleDLL

about my ZSTD_DLL_IMPORT comment : there are to cases :

1. when you CREATE the dll, so you must pass -DZSTD_DLL_EXPORT to the preprocessor so that __declspec(dllexport) is present in front of the public API
2. when you USE the dll. Then you must pass __declspec(dllimport) to the public API

So you could simplify the definition of ZSTDLIB_API like that :

#ifdef _WIN32
# ifdef ZSTD_DLL_EXPORT
#  define ZSTDLIB_API __declspec(dllexport) ZSTDLIB_VISIBILITY
# else
#  define ZSTDLIB_API __declspec(dllimport) ZSTDLIB_VISIBILITY /* It isn't required but allows to  generate better code, saving a function pointer load from the IAT and an indirect jump.*/
#else
# define ZSTDLIB_API ZSTDLIB_VISIBILITY
#endif

I've also removed the check of the value of ZSTD_DLL_EXPORT which is also useless. What is important is that -DZSTD_DLL_EXPORT must be passed to the preprocessor when building the dll. Then everything will be fine.

I think that ZSTDLIB_VISIBILITY is useless on Windows.

Final reference :

https://gcc.gnu.org/wiki/Visibility section "How to use the new C++ visibility support"

[Cyan4973]

I'm fine with most of the suggestions proposed.
Just, I note it leads to more changes than what the initial patch contains.
I also believe it's essential to document these choices properly.
Basically, the same work as you did in this thread, but made available in a more "permanent" way directly into the repository.

Maintainers can't be expected to know all details of all systems at all times, so it's normal to have questions and surprises.
In some future, any contribution can easily break something, and without the proper test acting as a guard, it will go undetected. And without documentation, it will be difficult to understand and explain what must be done differently.

[vtorri]

so, what's the status of this PR ?

[Cyan4973]

the name of the dll is not important,

Apparently, it is.

In this test, the *.dll produced with the new recipe, aka libzstd-1.dll, is copied as a version-less libzstd.dll, so that users already linking to this name continue to work accordingly.
Unfortunately, the test fails : even though it links to libzstd.dll, which exists, it's still looking for libzstd-1.dll nonetheless. The suspicion is that there is some kind of metadata within libzstd-1.dll which makes it impossible to simply copy the file as libzstd.dll afterwards.

A curious and annoying detail is that I can't reproduce it on my local config : using a VM with Windows 10 + msys2 installed, the very same test pass flawlessly with libzstd.dll.
That's a concern : without a capability to reproduce the issue, it's much harder to debug.

[vtorri]

it's more in libzstd.dll.a, i think

[Cyan4973]

it's more in libzstd.dll.a, i think

Well, if this test was compiled with -lzstd, then I think it would get into this category.
Reading the documentation in https://sourceware.org/binutils/docs/ld/WIN32.html, -lzstd will reach libzstd.dll.a, which will redirect torwards libzstd-1.dll.

But this test is different, since it lists libzstd.dll directly :
$(CC) $(FLAGS) $< -o $@$(EXT) -DZSTD_DLL_IMPORT=1 libzstd.dll

so it's unclear why it would need to reach libzstd-1.dll afterwards (and why it doesn't in my own msys2 VM).

It's also unclear to me why this test was redacted this way. So I'll try to get more information on what this test is trying to achieve.

[Cyan4973]

Not sure if it matters, but the name libzstd-1.dll is indeed present into libzstd.dll, itself just a copy of libzstd-1.dll . Scrubbed with xxd :
│000e9d80: 6c69 627a 7374 642d 312e 646c 6c00 5a42 libzstd-1.dll.ZB

[vtorri]

normally (and i never had problem with this ways since more than 10 years) :

1. create the dll with the following link flags : -shared -Wl,--out-implib,implib.dll.a dll_name.dll

so with zstd : -shared -Wl,--out-implib,libzstd.dll.a libzstd-$(LIBVER_MAJOR).dll

2. to link against zstd DLL : -L/path/to/libzstd.dll.a -lzstd

3. to execute a program linked against zstd DLL : the PATH must have the directory where the DLL is.

and that's all.

[vtorri]

btw you should create the dll with the version with step 1 above, then copy it to libzstd.dll (not the contrary)

[terrelln]

This PR hasn't made progress in over a year. This issue seems to be a bit complex, and each fix causes another problem. We're not against fixing this issue, if still present, but will need a different approach. So I will close this PR.