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

Comments

Projects
None yet
6 participants
@wkwspirit
Copy link

wkwspirit commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Member

bagder commented Oct 13, 2018

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

@rodwiddowson

This comment has been minimized.

Copy link
Contributor

rodwiddowson commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Author

wkwspirit commented Oct 13, 2018

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

I have pulled a request.

@wkwspirit

This comment has been minimized.

Copy link
Author

wkwspirit commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Contributor

rodwiddowson commented Oct 13, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Author

wkwspirit commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Contributor

rodwiddowson commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Author

wkwspirit commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Contributor

rodwiddowson commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Author

wkwspirit commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Contributor

rodwiddowson commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Author

wkwspirit commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Contributor

rodwiddowson commented Oct 13, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Author

wkwspirit commented Oct 15, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

jzakrzewski commented Oct 17, 2018

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

@rodwiddowson

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

MarcelRaad commented Oct 19, 2018

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

@jay

This comment has been minimized.

Copy link
Member

jay commented Oct 19, 2018

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

@bagder

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

MarcelRaad commented Jan 10, 2019

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

winbuild: conditionally use /DZLIB_WINAPI
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

winbuild: conditionally use /DZLIB_WINAPI
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

winbuild: conditionally use /DZLIB_WINAPI
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 added a commit to Some1NamedNate/curl that referenced this issue Jan 16, 2019

winbuild: conditionally use /DZLIB_WINAPI
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment