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

Prefer documented zlib library names #2354

Closed
wants to merge 2 commits into from

Conversation

richardthe3rd
Copy link
Contributor

As promised on mailing list "Building on Windows: zlib library names in winbuild/MakeBuild.vc" this PR means can build on Windows with zlib without needing to modify zlib library names.

Check for existence of import and static libraries with documented names and use them if they do. Fallback to previous names.

According to https://github.com/madler/zlib/blob/master/win32/README-WIN32.txt on Windows, the names of the import library is "zdll.lib" and static library is "zlib.lib".

Check for existence of import and static libraries with documented names and use them if they do. Fallback to previous names.

According to https://github.com/madler/zlib/blob/master/win32/README-WIN32.txt on Windows, the names of the import library is "zdll.lib" and static library is "zlib.lib".
@bagder bagder added the build label Mar 3, 2018
@bagder
Copy link
Member

bagder commented Mar 3, 2018

I'd appreciate a 👍 or otherwise positive comments from a windows dev or two first, but to me (as a non-windows dev) it looks fine!

Copy link
Contributor

@rodwiddowson rodwiddowson left a comment

Choose a reason for hiding this comment

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

I can make not comment on the static side but it seems OK to me if the names are OK. I note that in the static leg it looks for zlib.lib, but the default lib is zlib_a.lib

I have done a build using my usual build process and it shows no regressions. But that build, while using MakeFileBuiild.vc is nonstandard and so shouldn't be relied upon.

So in gerrit-speak "looks OJK to me, but some wlse must confirm"

@richardthe3rd
Copy link
Contributor Author

To be fair, I haven't checked the static branch either, but the names are consistent with my reading of https://github.com/madler/zlib/blob/master/win32/README-WIN32.txt - I don't see any mention of zlib_a.lib there. For builds that are using zlib_a.lib it should continue to work same as before.

@MarcelRaad
Copy link
Member

How did you build zlib to get these output file names? When using the Visual Studio 14 project files from versoon 1.2.11, they are zlibwapi.lib/dll for the dynamic library and zlibstat.lib for the static library.

@richardthe3rd
Copy link
Contributor Author

I used nmake on https://github.com/madler/zlib/blob/master/win32/Makefile.msc

I had seen reference to zlibwapi as well - a bit more reading suggests there are 2 "official" builds - according to https://github.com/madler/zlib/blob/master/contrib/vstudio/readme.txt

"There is also an official DLL build of zlib, named zlib1.dll"

What about adding another exists check on the library name?

@richardthe3rd
Copy link
Contributor Author

(sorry, too easy to hit close)

@MarcelRaad
Copy link
Member

MarcelRaad commented Mar 3, 2018

Ah, I had just gotten these names as well when using the makefile build in the win32 folder. Yes, I think checking for all three would be best.

zlib built from vstudio projects creates zlibwapi.lib/dll for the dynamic library and zlibstat.lib for the static library.
Copy link
Member

@MarcelRaad MarcelRaad left a comment

Choose a reason for hiding this comment

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

👍

@bagder
Copy link
Member

bagder commented Mar 4, 2018

Thanks!

@bagder bagder closed this in cc1d4c5 Mar 4, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants