[
https://issues.apache.org/jira/browse/ZOOKEEPER-2841?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16081634#comment-16081634
]
ASF GitHub Bot commented on ZOOKEEPER-2841:
-------------------------------------------
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
> Can you please describe what kinds of test / integration test you did on
windows?
Unfortunately, we don't have the unit tests building on Windows (under any
tools). So I ensured I could reliably build the project with CMake and get the
expected outputs. Moreover, I have a [zk-test
branch](https://github.com/andschwa/mesos/tree/zk-test) in Mesos where I
updated our system to apply these patches to the 3.5.2 release, and build with
them (verifying that the second patch resolved
[MESOS-7541](https://issues.apache.org/jira/browse/MESOS-7541)). I verified I
could link to the libraries as expected, and ran some tests (though I'd like to
give a full `mesos-test` pass of our entire suite).
Furthermore, we have been building and using the first patch (the addition
of the CMake build, but not the re-port changes) since [April
25](https://github.com/apache/mesos/commit/6e64ffaca365ed1e28256d7cb87bf9e1af626a75)
with no problems; so I'd say the CMake system is thoroughly tested for Windows
(at least in its previous form, but only some cosmetics, e.g.
TODOs/comments/default option for building unit tests, were changed when I
updated this PR).
> Can you please update the pull request description by adding a brief
description on what this patch is for and how it did it. The pull request
description will be part of commit message, and it's good to have a informative
commit message.
Wouldn't you rather use the two commit messages,
a58a5a95aef9cafc267a1b4a2bdb37f9e9e26363 and
c550a3e3babcaa3b3891280ac2d61b89fd294d06 as-is (those should be two links to
the commits with their respective messages, but the ASF bot might not copy them
since they're automatic from GitHub)? These are the patches as I'd commit them
(as in, I wouldn't squash them into one, since they're resolving two bugs).
> This pull request is targeting master, which is not going to be released
soon. branch-3.5 and branch-3.4 are branches for next releases, are you going
to send separate pull requests to those branches, or you are fine just with
this merged into master?
I would be happy to provide backports of these two patches, especially for
branch-3.5, so that I can remove the manual patch step in Mesos's build process
as soon as possible :D. But first, let's figure out the patch messages. I could
copy them into the PR description, but then I'm afraid it's going to be
squashed as one patch instead of the two logical patches. I could replace this
PR with two separate PRs, one for each patch, and then repeat that for the two
branches; but I'm not sure you want that either.
> ZooKeeper public include files leak porting changes
> ---------------------------------------------------
>
> Key: ZOOKEEPER-2841
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2841
> Project: ZooKeeper
> Issue Type: Bug
> Components: c client
> Environment: Windows 10 with Visual Studio 2017
> Reporter: Andrew Schwartzmeyer
> Assignee: Andrew Schwartzmeyer
> Labels: windows
>
> The fundamental problem is that the port of the C client to Windows is now
> close to six years old, with very few updates. This port leaks a lot of
> changes that should be internal to ZooKeeper, and many of those changes are
> simply no longer relevant. The correct thing to do is attempt to refactor the
> Windows port for new versions of ZooKeeper, removing dead/unneeded porting
> code, and moving dangerous porting code to C files instead of public headers.
> Two primary examples of this problem are
> [ZOOKEEPER-2491|https://issues.apache.org/jira/browse/ZOOKEEPER-2491] and
> [MESOS-7541|https://issues.apache.org/jira/browse/MESOS-7541].
> The first issue stems from this ancient porting code:
> {noformat}
> #define snprintf _snprintf
> {noformat}
> in
> [winconfig.h|https://github.com/apache/zookeeper/blob/ddf0364903bf7ac7cd25b2e1927f0d9d3c7203c4/src/c/include/winconfig.h#L179].
> Newer versions of Windows C libraries define {{snprintf}} as a function, and
> so it cannot be redefined.
> The second issue comes from this undocumented change:
> {noformat}
> #undef AF_INET6
> {noformat}
> again in
> [winconfig.h|https://github.com/apache/zookeeper/blob/ddf0364903bf7ac7cd25b2e1927f0d9d3c7203c4/src/c/include/winconfig.h#L169]
> which breaks any library that uses IPv6 and {{winsock2.h}}.
> Furthermore, the inclusion of the following defines and headers causes
> terrible problems for consuming libraries, as they leak into ZooKeeper's
> public headers:
> {noformat}
> #define _CRT_SECURE_NO_WARNINGS
> #define WIN32_LEAN_AND_MEAN
> #include <Windows.h>
> #include <Winsock2.h>
> #include <winstdint.h>
> #include <process.h>
> #include <ws2tcpip.h>
> {noformat}
> Depending on the order that a project includes or compiles files, this may or
> may not cause {{WIN32_LEAN_AND_MEAN}} to become unexpectedly defined, and
> {{windows.h}} to be unexpectedly included. This problem is exacberated by the
> fact that the {{winsock2.h}} and {{windows.h}} headers are order-dependent
> (if you read up on this, you'll see that defining {{WIN32_LEAN_AND_MEAN}} was
> meant to work-around this).
> Going forward, porting changes should live next to where they are used,
> preferably in source files, not header files, so they remain contained.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
