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

build in windows with zlib should add ZLIB_WINAPI in MakefileBuild.vc #3133

Closed
wkwspirit opened this issue Oct 13, 2018 · 24 comments
Closed
Assignees

Comments

@wkwspirit
Copy link

curl version:7.61.1
If you want to use nmake to build curl with zlib on windows ,you should modify the file MakefileBuild.vc.
Change the ZLIB_CFLAGS value like this:
ZLIB_CFLAGS = /DHAVE_ZLIB_H /DHAVE_ZLIB /DHAVE_LIBZ /DZLIB_WINAPI /I"$(ZLIB_INC_DIR)"

@bagder bagder added build Windows Windows-specific labels Oct 13, 2018
@bagder
Copy link
Member

bagder commented Oct 13, 2018

Thanks, can you provide your suggested fix as a full patch or even a PR?

@rodwiddowson
Copy link
Contributor

I'd also be interested as to the "why do we need ZLIB_WINAPI ?",, and what version of ZLIB this applies to? I have been building ZLIB as a dependency of CURL for several years and not yet hit on this need.

   /* If building or using zlib with the WINAPI/WINAPIV calling convention,
    * define ZLIB_WINAPI.
    * Caution: the standard ZLIB1.DLL is NOT compiled using ZLIB_WINAPI.
    */

30 second googling appears to indicate that it changes the expectations on the caller (it turns on __stdcall) which makes me nervous, but no more than that.

@wkwspirit
Copy link
Author

Thanks, can you provide your suggested fix as a full patch or even a PR?

I have pulled a request.

@wkwspirit
Copy link
Author

I'd also be interested as to the "why do we need ZLIB_WINAPI ?",, and what version of ZLIB this applies to? I have been building ZLIB as a dependency of CURL for several years and not yet hit on this need.

   /* If building or using zlib with the WINAPI/WINAPIV calling convention,
    * define ZLIB_WINAPI.
    * Caution: the standard ZLIB1.DLL is NOT compiled using ZLIB_WINAPI.
    */

30 second googling appears to indicate that it changes the expectations on the caller (it turns on __stdcall) which makes me nervous, but no more than that.

The zlib version I used is 1.2.11 . I use nmake to build the curl. If I don't add ZLIB_WINAPI in MakefileBuild.vc. It'll have link error _zlibVersion .

@rodwiddowson
Copy link
Contributor

Odd. I build 7.62 yesterday against that with no problems, however I built my own zlib, which might be the difference. Where did you get yours from?

Thanks/

@bagder
Copy link
Member

bagder commented Oct 13, 2018

I also rather suspect the zlib build to be the weird party here and not the curl build. People have built curl with zlib using this for quite some time. It also seems unlikely that zlib would suddenly have changed to require this define just to build with a specific version.

@wkwspirit
Copy link
Author

Odd. I build 7.62 yesterday against that with no problems, however I built my own zlib, which might be the difference. Where did you get yours from?

Thanks/

I download the zlib from https://github.com/madler/zlib .The lastest release version is 1.2.11.

@rodwiddowson
Copy link
Contributor

Build using Make or VC? I don't see that being defined in the makefile, but it does appear to be defined in the (contributed) VC projects....

@wkwspirit
Copy link
Author

Build using Make or VC? I don't see that being defined in the makefile, but it does appear to be defined in the (contributed) VC projects....

In the winbuild directory,use NMAKE.

@rodwiddowson
Copy link
Contributor

In that case I'm lost.

I'm about to do a full dependency build (openSSL 1.1.1, Curl 7.62.latest, Zlib 1.2.11) (using VC15). If this doesn't fail I don't know what to say.

And as an aside to @bagder I don't actually care about this change making it or not since I can chose to build without that call. I just worry when there is something I don't understand.

thanks to @wkwspirit for bearing with my questions.

@wkwspirit
Copy link
Author

In that case I'm lost.

I'm about to do a full dependency build (openSSL 1.1.1, Curl 7.62.latest, Zlib 1.2.11) (using VC15). If this doesn't fail I don't know what to say.

And as an aside to @bagder I don't actually care about this change making it or not since I can chose to build without that call. I just worry when there is something I don't understand.

thanks to @wkwspirit for bearing with my questions.

I use openSSL 1.1.1,Curl 7.61.1(I can't find the 7.62),zlib 1.2.11(using vc15).I succeed.If you paste the error info ,maybe I can find the reason.

@rodwiddowson
Copy link
Contributor

32 bit build fine, 64 bt under way. ZLIB_WINAPI not defined.

	Nmake /f Makefile.vc mode=dll WITH_DEVEL=$(ROOT_DIR)\$(OPENSSL_DIR)\$(VSCMD_ARG_TGT_ARCH)Debug	\
			WITH_SSL=dll WITH_ZLIB=dll ENABLE_WINSSL=no VC=15 DEBUG=yes MAKE="NMAKE /e"				\
			ZLIB_LFLAGS=/libpath:$(ROOT_DIR)$(ZLIB_DIR)\$(ZlibTargetDir)\DEBUG 						\
			ZLIB_CFLAGS="/DHAVE_ZLIB_H /DHAVE_ZLIB /DHAVE_LIBZ /I$(ROOT_DIR)$(ZLIB_DIR)" 			\
			ZLIB_LIBS=$(ZLIB_IMPLIB)D.lib SSL_LIBS="libcrypto.lib libssl.lib" 						\
			BASE_NAME=libcurl$(LIBCURL_FILE_VERSION) BASE_NAME_DEBUG=libcurl$(LIBCURL_FILE_VERSION)d

(Yes I know its an unorthodox build, but it does what we need which is using windows standard file names for debug DLLs)

@wkwspirit
Copy link
Author

32 bit build fine, 64 bt under way. ZLIB_WINAPI not defined.

	Nmake /f Makefile.vc mode=dll WITH_DEVEL=$(ROOT_DIR)\$(OPENSSL_DIR)\$(VSCMD_ARG_TGT_ARCH)Debug	\
			WITH_SSL=dll WITH_ZLIB=dll ENABLE_WINSSL=no VC=15 DEBUG=yes MAKE="NMAKE /e"				\
			ZLIB_LFLAGS=/libpath:$(ROOT_DIR)$(ZLIB_DIR)\$(ZlibTargetDir)\DEBUG 						\
			ZLIB_CFLAGS="/DHAVE_ZLIB_H /DHAVE_ZLIB /DHAVE_LIBZ /I$(ROOT_DIR)$(ZLIB_DIR)" 			\
			ZLIB_LIBS=$(ZLIB_IMPLIB)D.lib SSL_LIBS="libcrypto.lib libssl.lib" 						\
			BASE_NAME=libcurl$(LIBCURL_FILE_VERSION) BASE_NAME_DEBUG=libcurl$(LIBCURL_FILE_VERSION)d

(Yes I know its an unorthodox build, but it does what we need which is using windows standard file names for debug DLLs)

Sorry,I don't know how you can compile success without ZLIB_WINAPI.

@rodwiddowson
Copy link
Contributor

Me neither (only the other way around), I suggest we see if he other Windows guys come up with an idea. But that might be Monday.

@jay
Copy link
Member

jay commented Oct 13, 2018

zlib can be built using stdcall (ZLIB_WINAPI) or cdecl conventions in Windows. zlib DLLs using cdecl is common in Windows. The issue reported here stems from the reporter building zlib himself using contributed project files which define ZLIB_WINAPI and therefore are using stdcall. It appears given this issue that ZLIB_WINAPI must not be defined in zconf or whatever but why that is I don't know. I don't see this as a curl issue.

@wkwspirit
Copy link
Author

Me neither (only the other way around), I suggest we see if he other Windows guys come up with an idea. But that might be Monday.

I find the reason.The curl use several functions of zlib. So if you compile zlib with ZLIB_WINAPI,you should add it when you compile curl.

@bagder
Copy link
Member

bagder commented Oct 16, 2018

I suggest this is a documentation issue. If zlib was built with ZLIB_WINAPI then that should be defined when curl is using such a zlib. If not, that define is not a good idea to use. And I don't think we should change our default now unless we have a strong reason to believe that users have changed to this way of building more often than they're not.

@jzakrzewski
Copy link
Contributor

If that's of any help there's ZEXTERN uLong ZEXPORT zlibCompileFlags OF((void)); maybe it could be used to detect it?

@rodwiddowson
Copy link
Contributor

rodwiddowson commented Oct 17, 2018

I suggest this is a documentation issue.

I'd agree.

What I am unsure is whether the tools are there to allow is from the top level make file Makefile.vc. It seems that the subsidiary file MakefileBuild.vc just builds the ZLIB_CFLAGS if it needs them to a fixed value.

One could say "Curl doesn't work with ZLIB_WINAPI", which would certainly be a suggestion for 7.62.0, or one could document how to call the subsidiary file directly (I do this, but I would't want to document it).

@jay
Copy link
Member

jay commented Oct 19, 2018

zlibCompileFlags

We would need to catch it before then. Right now we have code like this:

!IF "$(WITH_ZLIB)"=="dll"
!IF EXISTS("$(ZLIB_LIB_DIR)\zlibwapi.lib")
ZLIB_LIBS = zlibwapi.lib
!ELSEIF EXISTS("$(ZLIB_LIB_DIR)\zdll.lib")
ZLIB_LIBS = zdll.lib
!ELSE
ZLIB_LIBS = zlib.lib
!ENDIF
USE_ZLIB = true
ZLIB = dll
!ELSEIF "$(WITH_ZLIB)"=="static"

Can we assume that if zlibwapi.lib exists that always means zlib was built with ZLIB_WINAPI? Then we could add ZLIB_WINAPI conditionally if zlibwapi is found.

@MarcelRaad
Copy link
Member

As I wrote in #3135 (comment), I think that would be best. And also for its static version zlibstat.lib.

@jay
Copy link
Member

jay commented Oct 19, 2018

I missed that or maybe it was stuck in my unconscious, apologies.

@bagder
Copy link
Member

bagder commented Jan 10, 2019

Is there anyone who wants to work on this? Otherwise this will go to the KNOWN_BUGS pile soon.

@MarcelRaad
Copy link
Member

I think I can do that!

@MarcelRaad MarcelRaad self-assigned this Jan 10, 2019
MarcelRaad added a commit to MarcelRaad/curl that referenced this issue Jan 11, 2019
zlibwapi.lib (dynamic library) and zlibstat.lib (static library) use
have the ZLIB_WINAPI define set by default. Using them requires that
define too.

Ref: https://zlib.net/DLL_FAQ.txt

Fixes curl#3133
Closes
MarcelRaad added a commit to MarcelRaad/curl that referenced this issue Jan 11, 2019
zlibwapi.lib (dynamic library) and zlibstat.lib (static library) use
have the ZLIB_WINAPI define set by default. Using them requires that
define too.

Ref: https://zlib.net/DLL_FAQ.txt

Fixes curl#3133
Closes
MarcelRaad added a commit to MarcelRaad/curl that referenced this issue Jan 11, 2019
zlibwapi.lib (dynamic library) and zlibstat.lib (static library) have
the ZLIB_WINAPI define set by default. Using them requires that define
too.

Ref: https://zlib.net/DLL_FAQ.txt

Fixes curl#3133
Closes
Some1NamedNate pushed a commit to Some1NamedNate/curl that referenced this issue Jan 16, 2019
zlibwapi.lib (dynamic library) and zlibstat.lib (static library) have
the ZLIB_WINAPI define set by default. Using them requires that define
too.

Ref: https://zlib.net/DLL_FAQ.txt

Fixes curl#3133
Closes curl#3460
@lock lock bot locked as resolved and limited conversation to collaborators Apr 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging a pull request may close this issue.

6 participants