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

Windows: MinGW fixes and Windows Travis builders #663

Merged
merged 4 commits into from
Aug 9, 2019

Conversation

mbitsnbites
Copy link
Contributor

Unfortunately the MSVC fixes in #658 broke MinGW builds (mainly due to using _WIN32 instead of _MSC_VER when checking if the compiler is MS Visual Studio).

The ifdef-logic has been cleaned up slightly to support both MSVC and MinGW (and possibly Clang etc too).

In addition, a couple of Travis build steps are added that should catch future breakage of the MinGW and MSVC support.

@mbitsnbites
Copy link
Contributor Author

Apologies for being impatient, but any chance of this PR getting reviewed anytime soon? It would be good to get it into master to get better Travis checks for Windows PR:s. @mnunberg ?

@@ -50,7 +50,9 @@
#include <ws2tcpip.h>
#include <stddef.h>

#ifdef _MSC_VER
typedef signed long ssize_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about win64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so? _MSC_VER is defined for all versions of the Microsoft Visual C++ compiler (and _WIN32 is defined for both 32-bit and 64-bit Windows).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way: Both the new travis builders (MinGW and MSVC) do 64-bit builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, i forgot that long becomes 64 bits on 64 bit platforms.

@@ -34,7 +34,7 @@
#define __SDS_H
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we pull sds directly from redis upstream.. so this might get overwritten at a later point in time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT the source for sds has already diverged from the upstream version (due to previous hiredis patches). This patch merely fixes regressions due to earlier patches.

Going forward it would be good to sync with/upstream to redis.

@mbitsnbites
Copy link
Contributor Author

I'll try to rebase the branch and fix any conflicts...

Use _MSC_VER (instead of _WIN32) for things that are specific for
Visual Studio.

Also remove #include <winsock2.h> from hiredis.h, as it leaks too
many symbols and defines into the global namespace, which is
undesirable for a public interface header. Anyone who uses the
the affected parts of the hiredis API needs to include the
appropriate headers anyway in order to declare struct timeval
variables.
@mbitsnbites
Copy link
Contributor Author

@mnunberg I've rebased and all Travis-CI checks passed.

@mnunberg mnunberg merged commit ac49287 into redis:master Aug 9, 2019
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