Skip to content

test installation with MSYS2+mingw-w64#1916

Merged
Cyan4973 merged 1 commit intofacebook:appveyorTestfrom
vtorri:dev
Mar 3, 2020
Merged

test installation with MSYS2+mingw-w64#1916
Cyan4973 merged 1 commit intofacebook:appveyorTestfrom
vtorri:dev

Conversation

@Cyan4973
Copy link
Copy Markdown
Contributor

@Cyan4973 Cyan4973 commented Dec 2, 2019

appveyorCI has been updated to add an installation test
which fails without this patch.
It should pass with it.

Comment thread lib/Makefile
@$(INSTALL) -d -m 755 $(DESTDIR)$(LIBDIR)/
ifneq (,$(filter MINGW%,$(shell uname)))
@$(INSTALL) -d -m 755 $(DESTDIR)$(BINDIR)/
@$(INSTALL_PROGRAM) $(LIBZSTD) $(DESTDIR)$(BINDIR)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why does $(LIBZSTD) go in $(BINDIR)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a good point.
No idea what is the "proper" directory layout for msys2.
This part comes from @vtorri patch.

@Cyan4973
Copy link
Copy Markdown
Contributor Author

Cyan4973 commented Dec 4, 2019

Note : there is still an incompatibility with our test suite and @vtorri 's msys2 patch,
related to dll naming.
So this can't be merged yet.

@vtorri
Copy link
Copy Markdown
Contributor

vtorri commented Dec 4, 2019

dll should go in bin/ subdir
import lib (.dll.a) should go in lib/

@terrelln
Copy link
Copy Markdown
Contributor

@Cyan4973 what is the status of this PR?

@Cyan4973
Copy link
Copy Markdown
Contributor Author

It seems this is a case of a patch breaking a test, and no further update to fix the situation.

I'll look into it again at my next oncall session, starting later this week.

@Cyan4973
Copy link
Copy Markdown
Contributor Author

Cyan4973 commented Jan 17, 2020

The source repository comes from @vtorri , so I can't make changes to this PR directly, as changes must come from source repository.

What I could do instead is duplicate @vtorri commit into some local branch, and resume from there, trying to fix what still doesn't work.

@Cyan4973
Copy link
Copy Markdown
Contributor Author

Cyan4973 commented Jan 18, 2020

OK, so the problematic case is test-symbols.

And the reason is, the test can't even start, because it expects to find libzstd.dll in /lib/dll, but the patch has changed that.

In both cases, the change is done unilaterally and without documentation "because it's better". On the topic of library name change :

is common usage to add major version for parallal installation after api or abi break

but a quick look at any Windows system or program directory show that most *.dll files do not follow such practice, including Microsoft ones, including msys and mingw ones.
When the number is present (that is, very rarely), generally it's directly attached, and sometimes it's preceded by an _ underscore, I have yet to find a single instance where it's preceded by a - dash .

edit: found them ! In the msys2 installation directly, there are a bunch of *.dll following the - dash convention in /usr/bin. Actually, all instances of - dash convention are present in this directory.

The suggestion to fix the test is :

Just add -$(LIBVER_MAJOR)

which is not that trivial, since the place where the test is defined doesn't know what $(LIBVER_MAJOR) is. It may presume that it should likely be 1. But it also underlines that any existing user of the *.dll file will have to be aware of the name change, otherwise linking or runtime will fail.

So I think I guess what to do next :

  • provide both version of the *.dll file, with and without the number, so that existing applications expecting libzstd.dll still work.
  • change the library name with number, so that the version number sits right next to the name, which seems a more common convention.
  • run tests, see if they pass
  • document choices

@vtorri
Copy link
Copy Markdown
Contributor

vtorri commented Jan 18, 2020

When the number is present (that is, very rarely)

projects built with Visual Studio indeed do not add this version number

but on MSYS2, libtool and meson build system are quite common. Here is the list of DLL built for the EFL (a toolkit like gtkk or qt) with an installer i've written (see the list here https://github.com/vtorri/ewpi). The packages are built from source, with almost no modifications (which is the purpose of of my patch for zstd) and you will see that the major version is quite common (and btw you have accepted my patch for lz4 which adds this major version...) :

/opt/ewpi_64/bin/avcodec-58.dll
/opt/ewpi_64/bin/avdevice-58.dll
/opt/ewpi_64/bin/avfilter-7.dll
/opt/ewpi_64/bin/avformat-58.dll
/opt/ewpi_64/bin/avutil-56.dll
/opt/ewpi_64/bin/icudt.dll
/opt/ewpi_64/bin/icudt65.dll
/opt/ewpi_64/bin/icuin.dll
/opt/ewpi_64/bin/icuin65.dll
/opt/ewpi_64/bin/icuio.dll
/opt/ewpi_64/bin/icuio65.dll
/opt/ewpi_64/bin/icutest.dll
/opt/ewpi_64/bin/icutest65.dll
/opt/ewpi_64/bin/icutu.dll
/opt/ewpi_64/bin/icutu65.dll
/opt/ewpi_64/bin/icuuc.dll
/opt/ewpi_64/bin/icuuc65.dll
/opt/ewpi_64/bin/libaacs-0.dll
/opt/ewpi_64/bin/libarchive-13.dll
/opt/ewpi_64/bin/libass-9.dll
/opt/ewpi_64/bin/libbdplus-0.dll
/opt/ewpi_64/bin/libbluray-2.dll
/opt/ewpi_64/bin/libbs2b-0.dll
/opt/ewpi_64/bin/libBullet3Common.dll
/opt/ewpi_64/bin/libBulletCollision.dll
/opt/ewpi_64/bin/libBulletDynamics.dll
/opt/ewpi_64/bin/libBulletInverseDynamics.dll
/opt/ewpi_64/bin/libBulletSoftBody.dll
/opt/ewpi_64/bin/libbz2-1.dll
/opt/ewpi_64/bin/libcairo-2.dll
/opt/ewpi_64/bin/libcairo-script-interpreter-2.dll
/opt/ewpi_64/bin/libcares-2.dll
/opt/ewpi_64/bin/libcheck-0.dll
/opt/ewpi_64/bin/libcrypto-44.dll
/opt/ewpi_64/bin/libcurl-4.dll
/opt/ewpi_64/bin/libdav1d.dll
/opt/ewpi_64/bin/libdbus-1-3.dll
/opt/ewpi_64/bin/libdjvulibre-21.dll
/opt/ewpi_64/bin/libexpat-1.dll
/opt/ewpi_64/bin/libffi-7.dll
/opt/ewpi_64/bin/libFLAC-8.dll
/opt/ewpi_64/bin/libfontconfig-1.dll
/opt/ewpi_64/bin/libfreetype-6.dll
/opt/ewpi_64/bin/libfribidi-0.dll
/opt/ewpi_64/bin/libgcrypt-20.dll
/opt/ewpi_64/bin/libgif-6.dll
/opt/ewpi_64/bin/libgio-2.0-0.dll
/opt/ewpi_64/bin/libglib-2.0-0.dll
/opt/ewpi_64/bin/libgme.dll
/opt/ewpi_64/bin/libgmodule-2.0-0.dll
/opt/ewpi_64/bin/libgobject-2.0-0.dll
/opt/ewpi_64/bin/libgpg-error6-0.dll
/opt/ewpi_64/bin/libgraphene-1.0-0.dll
/opt/ewpi_64/bin/libgraphite2.dll
/opt/ewpi_64/bin/libgsm-1.dll
/opt/ewpi_64/bin/libgstallocators-1.0-0.dll
/opt/ewpi_64/bin/libgstapp-1.0-0.dll
/opt/ewpi_64/bin/libgstaudio-1.0-0.dll
/opt/ewpi_64/bin/libgstbase-1.0-0.dll
/opt/ewpi_64/bin/libgstcheck-1.0-0.dll
/opt/ewpi_64/bin/libgstcontroller-1.0-0.dll
/opt/ewpi_64/bin/libgstfft-1.0-0.dll
/opt/ewpi_64/bin/libgstgl-1.0-0.dll
/opt/ewpi_64/bin/libgstnet-1.0-0.dll
/opt/ewpi_64/bin/libgstpbutils-1.0-0.dll
/opt/ewpi_64/bin/libgstreamer-1.0-0.dll
/opt/ewpi_64/bin/libgstriff-1.0-0.dll
/opt/ewpi_64/bin/libgstrtp-1.0-0.dll
/opt/ewpi_64/bin/libgstrtsp-1.0-0.dll
/opt/ewpi_64/bin/libgstsdp-1.0-0.dll
/opt/ewpi_64/bin/libgsttag-1.0-0.dll
/opt/ewpi_64/bin/libgstvideo-1.0-0.dll
/opt/ewpi_64/bin/libgthread-2.0-0.dll
/opt/ewpi_64/bin/libharfbuzz-0.dll
/opt/ewpi_64/bin/libharfbuzz-icu-0.dll
/opt/ewpi_64/bin/libharfbuzz-subset-0.dll
/opt/ewpi_64/bin/libiconv.dll
/opt/ewpi_64/bin/libilbc-2.dll
/opt/ewpi_64/bin/libintl-9.dll
/opt/ewpi_64/bin/libjbig2dec-0.dll
/opt/ewpi_64/bin/libjpeg-8.dll
/opt/ewpi_64/bin/libkvazaar-4.dll
/opt/ewpi_64/bin/liblcms2-2.dll
/opt/ewpi_64/bin/libLinearMath.dll
/opt/ewpi_64/bin/liblz4-1.dll
/opt/ewpi_64/bin/liblzma-5.dll
/opt/ewpi_64/bin/libmodplug-1.dll
/opt/ewpi_64/bin/libmp3lame-0.dll
/opt/ewpi_64/bin/libmpg123-0.dll
/opt/ewpi_64/bin/libmysofa.dll
/opt/ewpi_64/bin/libnghttp2-14.dll
/opt/ewpi_64/bin/libogg-0.dll
/opt/ewpi_64/bin/libopenh264-4.dll
/opt/ewpi_64/bin/libopenjp2.dll
/opt/ewpi_64/bin/libopenmpt-0.dll
/opt/ewpi_64/bin/libopus-0.dll
/opt/ewpi_64/bin/liborc-0.4-0.dll
/opt/ewpi_64/bin/libout123-0.dll
/opt/ewpi_64/bin/libpixman-1-0.dll
/opt/ewpi_64/bin/libpkgconf-3.dll
/opt/ewpi_64/bin/libpng16-16.dll
/opt/ewpi_64/bin/libpsl-5.dll
/opt/ewpi_64/bin/libregex-1.dll
/opt/ewpi_64/bin/librtmp.dll
/opt/ewpi_64/bin/librtmp-1.dll
/opt/ewpi_64/bin/libsnappy.dll
/opt/ewpi_64/bin/libsndfile-1.dll
/opt/ewpi_64/bin/libsoxr.dll
/opt/ewpi_64/bin/libsoxr-lsr.dll
/opt/ewpi_64/bin/libspeex-1.dll
/opt/ewpi_64/bin/libssh2-1.dll
/opt/ewpi_64/bin/libssl-46.dll
/opt/ewpi_64/bin/libtag.dll
/opt/ewpi_64/bin/libtheora-0.dll
/opt/ewpi_64/bin/libtheoradec-1.dll
/opt/ewpi_64/bin/libtheoraenc-1.dll
/opt/ewpi_64/bin/libtiff-5.dll
/opt/ewpi_64/bin/libtls-18.dll
/opt/ewpi_64/bin/libvorbis-0.dll
/opt/ewpi_64/bin/libvorbisenc-2.dll
/opt/ewpi_64/bin/libvorbisfile-3.dll
/opt/ewpi_64/bin/libwavpack-1.dll
/opt/ewpi_64/bin/libwebp-7.dll
/opt/ewpi_64/bin/libwebpdecoder-3.dll
/opt/ewpi_64/bin/libwebpdemux-2.dll
/opt/ewpi_64/bin/libwebpmux-3.dll
/opt/ewpi_64/bin/libxml2-2.dll
/opt/ewpi_64/bin/libzstd.dll
/opt/ewpi_64/bin/lua51.dll
/opt/ewpi_64/bin/SDL2.dll
/opt/ewpi_64/bin/swresample-3.dll
/opt/ewpi_64/bin/swscale-5.dll
/opt/ewpi_64/bin/zlib-1.dll

@Cyan4973
Copy link
Copy Markdown
Contributor Author

Cyan4973 commented Jan 21, 2020

Indeed, looking at *.dll names at large on Windows doesn't show many version numbers, but zooming into /usr/bin within msys2 environment specifically shows a different picture, with many *.dll featuring a version number.
So let's use that convention since the install patch is for msys2.

A link to a page explaining naming convention on msys2 would have been useful for documentation purpose.

@vtorri
Copy link
Copy Markdown
Contributor

vtorri commented Jan 27, 2020

any news about this ?

@Cyan4973
Copy link
Copy Markdown
Contributor Author

revisiting the failing test.
We'll sort this out, and plan to merge this patch afterwards.

@Cyan4973
Copy link
Copy Markdown
Contributor Author

Cyan4973 commented Mar 3, 2020

Let's merge it locally. There may be additional details to work out before a merge into dev.

@Cyan4973 Cyan4973 merged commit 8ff3ed5 into facebook:appveyorTest Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants