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

add module definition file, let vs2019 build correct binary via cmake #760

Closed
wants to merge 2 commits into from

Conversation

qiongzhu
Copy link

@qiongzhu qiongzhu commented Feb 5, 2020

As title. This patch set enables build on Windows 10 with Visual Studio 2019 community version. The build process & output files are verified.

Without the 'hiredis.def' module definition file, VS 2019 will generate an dll without any exports, which makes the output file 'hiredis.dll' useless.

Besides, the header file 'alloc.h' should be copied for other apps to compile correctly against hiredis library.

Detailed build instructions:

Configure for Windows 32 bit:
1. open 'x86 Native Tools Command Prompt for VS 2019', navigate to your source directory
2. configure with cmake:
  cmake -G "Visual Studio 16 2019" -A Win32 -DCMAKE_CONFIGURATION_TYPES=RelWithDebInfo -DCMAKE_INSTALL_PREFIX=/out32/hiredis/ .

Configure for Windows 64 bit:
1. open 'x64 Native Tools Command Prompt for VS 2019', navigate to your source directory
2. configure with cmake:
  cmake -G "Visual Studio 16 2019" -DCMAKE_CONFIGURATION_TYPES=RelWithDebInfo -DCMAKE_INSTALL_PREFIX=/out64/hiredis/ .

Finally, build & install:
  msbuild.exe /p:Configuration=RelWithDebInfo ALL_BUILD.vcxproj
  msbuild.exe /p:Configuration=RelWithDebInfo INSTALL.vcxproj

@michael-grunder
Copy link
Collaborator

Hi @qiongzhu thanks for the PR.

I haven't developed on Windows in more than a decade, and I have no Windows machines so I'm not able to be very useful here.

That said, it appears that the .def file isn't recognized in MinGW and is causing a failed Appveyor build.

Travis is also failing on Windows but that appears unrelated to this PR

Configure for Windows 32 bit:
1. open 'x86 Native Tools Command Prompt for VS 2019', navigate to your source directory
2. configure with cmake:
  cmake -G "Visual Studio 16 2019" -A Win32 -DCMAKE_CONFIGURATION_TYPES=RelWithDebInfo -DCMAKE_INSTALL_PREFIX=/out32/hiredis/ .

Configure for Windows 64 bit:
1. open 'x64 Native Tools Command Prompt for VS 2019', navigate to your source directory
2. configure with cmake:
  cmake -G "Visual Studio 16 2019" -DCMAKE_CONFIGURATION_TYPES=RelWithDebInfo -DCMAKE_INSTALL_PREFIX=/out64/hiredis/ .

Finally, build & install:
  msbuild.exe /p:Configuration=RelWithDebInfo ALL_BUILD.vcxproj
  msbuild.exe /p:Configuration=RelWithDebInfo INSTALL.vcxproj
@qiongzhu
Copy link
Author

qiongzhu commented Feb 6, 2020

Gcc 7.4 from cygwin 32/64 uses different syntax in .def file. The problem is fixed, and can be verified via above AppVeyor build. The output hiredis.dll exports all internal symbols without the .def file. Currently, only the APIs in the .def file are exported.

The other CI 'Travis CI build' got a failure. I've checked the build procedure and find out it uses following command to build with Visual Studio 2017 on Windows:

cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release && ninja -v

I've verified the above command in VS2017 & VS2019, it works perfectly. Please check the build server again.

Besides, the VS2017 + cmake + ninja build also needs the .def file, or you will get a hiredis.dll without any exported symbols.

@michael-grunder
Copy link
Collaborator

michael-grunder commented Feb 6, 2020

@mbitsnbites Sorry for tagging you but can you weigh in on this PR? From what I read it appears right but I'm not familiar enough with Windows.

I'm specifically interested in the .def file. Adding alloc.h isn't only needed on Windows.

@mbitsnbites
Copy link
Contributor

Hi! It's been a while since I messed with def files (it was back in 2000 IIRC). I only have two comments:

  1. Should the def file really be included in the source list for ADD_LIBRARY(hiredis SHARED ... for all platforms (or just for Windows)? It seems to work, but I'm not sure why.
  2. I'm getting the failing Travis build in another project, and I suspect that it's due to a change in the build environment (see https://github.com/mbitsnbites/buildcache/issues/78 ).

@michael-grunder
Copy link
Collaborator

Should the def file really be included in the source list for ADD_LIBRARY(hiredis SHARED ... for all platforms (or just for Windows)? It seems to work, but I'm not sure why.

Good point, probably not given it has no use on Linux/OSX.

I'm getting the failing Travis build in another project, and I suspect that it's due to a change in the build environment

It looks like the 1803 -> 1809 windows build container upgrade caused an issue here, although I'm not sure exactly what issue.

Travis-CI forum post

I think it's related to vcvarsall.bat as I can get hiredis to build by using cmake directly.

michael-grunder added a commit to michael-grunder/hiredis that referenced this pull request Feb 8, 2020
@michael-grunder
Copy link
Collaborator

Hi @qiongzhu, I took a crack at limiting the .def file logic to Windows as it's not needed in Linux/OSX.

Link to your PR with the Windows specific logic

Let me know if that logic actually works on a Windows machine.

@qiongzhu
Copy link
Author

qiongzhu commented Feb 9, 2020

@michael-grunder I've verified your commit 'd1d2bd73c47cd109a1e9aa0adcd06f6b49f42c04', and it works as expected.

.def file is one of msvc way of controlling exported symbols. It seems that gcc also recognize the file, but take no action outside of cygwin. Anyway, it is better to limit the logic as windows specific.

@michael-grunder
Copy link
Collaborator

Closing in favor of #764

michael-grunder added a commit that referenced this pull request Feb 28, 2020
Housekeeping

* Check for C++ (#758, #750) 
* Include `alloc.h` in `make install` and `cmake`
* Add a `.def` file for Windows (#760)
* Include allocation wrappers referenced in adapter headers
* Fix minor syntax errors and typos in README
* Fix CI in Windows by properly escaping arguments (#761)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants