search

zeromq / zmqpp

0mq 'highlevel' C++ bindings
http://zeromq.github.io/zmqpp
Mozilla Public License 2.0
438 stars 195 forks source link

`-Wextra` causes build failure on Windows/MSVC #171

Closed danielunderwood closed 7 years ago

danielunderwood commented 7 years ago

The root CmakeLists.txt has the line set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -pedantic -Wextra") which breaks builds on Windows/MSVC since -Wextra isn't defined.

I don't mind submitting a PR, but I'm solving some other issues with builds on Windows at the moment. Are builds on Windows even supposed to be supported? I'm seeing some issues related to Windows, so I was assuming Windows is supported.

benjamg commented 7 years ago

Ideally it builds on Windows, but I'm not sure how many of the maintainers use it so it doesn't get tested as often as it should. I don't see any reason we can't remove -Wextra it's nice to have the warnings but won't break anything.

danielunderwood commented 7 years ago

We don't necessarily have to completely remove it. We could have that flag when not on Windows. I think libzmq does that and then provides it as a CMake option as well.

Regarding submitting the changes, does zmqpp just do the normal fork, change, PR way of changes? Do you want me to consolidate all the fixes for Windows in one PR or do a separate PR for each?

danielunderwood commented 7 years ago

I just noticed your commit to fix the issue and that will also work. I've found a handful of other errors on Windows that I can either submit issues for or submit changes. Unfortunately, I'm not sure how far the errors go since I haven't reached an actual working build for Windows yet.

benjamg commented 7 years ago

As long as each PR leaves it in a working condition happy to do separate ones or a single one which ever makes life easier for contribution. We could really do with having the travis, or similar, test a windows and a mac build at some point.

danielunderwood commented 7 years ago

I was actually looking into testing for Windows. It seems that Travis has been working on Windows support for a while, but still doesn't have it. It looks like most people are using Appveyor for Windows testing and Travis for Linux and Mac. This does mean that you have to keep track of two configuration files, but it probably wouldn't be too bad.

Mac support will just require a couple lines in the config for Travis, which I can go ahead and submit a PR for. Unfortunately, Mac builds are failing the last time I checked due to #164. I'm planning to fix that issue since it's also causing an issue on Windows, so I can either do the support for Mac on Travis before that fix and have a broken CI build or after that fix and (hopefully) have a working CI build.

EDIT: I actually ended up fixing the htobe32 and am also working on testing Mac on Travis.