[
https://issues.apache.org/jira/browse/ZOOKEEPER-2841?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16090704#comment-16090704
]
ASF GitHub Bot commented on ZOOKEEPER-2841:
-------------------------------------------
GitHub user andschwa opened a pull request:
https://github.com/apache/zookeeper/pull/311
ZOOKEEPER-2841: ZooKeeper public include files leak porting change
This patch primarily adds a cross-platform CMake build system. This is in
addition to the Autotools system on Linux, until it can be deprecated, and
this
replaces the existing committed Visual Studio Solutions for Windows.
As this commit deprecates (and breaks) the previously existing Visual Studio
solutions and projects, they've been removed. Building on Windows now
requires
CMake, but this enables the support of any code-supported Visual Studio
version.
Support for Visual Studio 2017 was explicitly added by this patch, and
support
for future versions, barring software bugs, should be available
automatically
through CMake.
The notable features lacking in comparison to Autotools are Solaris support,
shared library builds, whitelisted symbol exports, libtool integration, and
doxygen document generation. Almost everything else from Autotools has been
ported, including header/function/library checks, and all targets
(zookeeper,
hashtable, cli, load_gen, and tests).
The primary work involved for CMake (other than the writing of
`CMakeLists.txt`)
was transitioning the hand-written `winconfig.h` to an auto-generated
`config.h`
file, and guarding code with `#ifdef HAVE_FEATURE`. The `cmake_config.h.in`
template was modeled after the Autotools config file so that the feature
guards
share the same names.
This patch also refactors the Windows port of the C client. Notably, it
moves as
much porting code as possible out of the publicly included `winconfig.h`
header
and into the relevant source files, or the private `winport.h` header.
While `load_gen.c` looks at first glance as if it were ported to Windows, it
never actually was, so the erroneous `#include "win32port.h"` was removed,
and
the target is not built on Windows.
The `include/winstdint.h` header was removed as it has been replaced by
`<stdint.h>`. This might break very old versions of Visual Studio; but in
those
cases, previous versions of the client are available.
A bug for upstream libraries including the ZooKeeper headers was fixed by
removing `#undef AF_INET6` entirely, and removing `#include <windows.h>`,
as it
"pulls in the world" and should not be included publicly.
A guard was placed around `#define snprintf` in order to enable compiling
with
modern versions of MSVC, including Visual Studio 2015.
A problem with `DLL_EXPORT` and `USE_STATIC_LIB` being redefined was fixed.
There are existent warnings which this patch did not attempt to fix.
Building tests on Windows is not supported, but was not previously
supported.
The tests need to be ported.
Future work should resolve the `ACL` / `ZKACL` conflict, and potentially
remove
`include/winconfig.h` altogether.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andschwa/zookeeper cmake-backport-3.5
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/311.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #311
----
commit 081833e9899afd4937075038f78bb4faff446ef4
Author: Andrew Schwartzmeyer <[email protected]>
Date: 2017-04-10T23:12:40Z
ZOOKEEPER-2756: Add CMake build system for better cross-platform support
This notably lacks Solaris and libtool support.
Almost everything else from Autotools has been ported,
including header/function/library checks, and all targets
(zookeeper, hashtable, cli, load_gen, and tests).
Both Linux and Windows are supported.
The primary work involved (other than the writing of `CMakeLists.txt`)
was transitioning the hand-written `winconfig.h` to an
auto-generated `config.h` file, and guarding code with `#ifdef
HAVE_FEATURE`. The `cmake_config.h.in` template was modeled after
the Autotools config file so that the feature guards share the same
names.
While `load_gen.c` looks at first glance as if it were ported to Windows,
it never actually was, so the erroneous `#include "win32port.h"` was
removed, and the target is not built on Windows.
There are existent warnings which this patch did not attempt to fix.
A guard was placed around `#define snprintf` in order to enable
compiling with modern versions of MSVC.
Fixed DLL_EXPORT and USE_STATIC_LIB redefinition.
As this commit deprecates (and breaks) the previously existing Visual
Studio solutions and projects, they've been removed.
Building tests on Windows is still not supported.
commit 49f9558dea6504e5f082edd0d4d5191503c54235
Author: Andrew Schwartzmeyer <[email protected]>
Date: 2017-07-07T22:35:37Z
ZOOKEEPER-2841: ZooKeeper public include files leak porting changes
This patch refactors the Windows port of the C client. Notably, it moves
as much porting code as possible out of the publicly included
`winconfig.h` header and into the relevant source files. This also
removes `winstdint.h` as it has been replaced by `<stdint.h>`. It fixes
problems for upstream libraries by removing `#undef AF_INET6` and
`#include <windows.h>`.
Future work should resolve the `ACL` / `ZKACL` conflict, and potentially
remove `include/winconfig.h` altogether.
----
> 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)
