zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ZOOKEEPER-2841) ZooKeeper public include files leak porting changes
Date Sat, 08 Jul 2017 23:17:00 GMT

    [ https://issues.apache.org/jira/browse/ZOOKEEPER-2841?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16079363#comment-16079363

ASF GitHub Bot commented on ZOOKEEPER-2841:

Github user andschwa commented on the issue:

    @hanm the PR is rebased, retested, and has some extra notes in it now for some weird CMake
    I would not put the CMake files in a separate folder. There are couple of reasons for
    * It is a very strong convention with CMake that the top level `CMakeLists.txt` file exists
at the top level of the source directory (thus making variables like `CMAKE_SOURCE_DIR` make
sense). This is also necessary for the macro `ExternalProject_Add` to be used with older (pre
3.8) versions of CMake, when it didn't have the `SOURCE_SUBDIR` option. (This macro is often
used by other projects, e.g. Mesos, to download and build a dependency.)
    * The CMake system can also be used on Linux platforms (in fact, it's even setup to build
and run the tests with `ctest`). If `cmake` has not been invoked, or if `autoconf`/`configure`
have not been invoked, they will not interfere with each other (so the source files themselves
can co-exist). It's only the generated files, and only on systems that use `make`, that may
become confusing. But this confusion is easily cleared up by asking: which configure system
did you invoke? So I don't think it'll be much of a problem. (We have both Autotools and CMake
concurrently in Mesos, with the eventual plan of deprecating the former with the latter. Developers
still using Autotools have been just fine ignoring CMake.)
    * CMake can easily build out-of-tree. I've tested this, as I usually build with `mkdir
build; cd build; cmake ..; cmake --build .`. So you can use the Autotools and CMake systems
concurrently, if you're, say, testing both systems.
    > Ultimately we might drop existing makefiles in favor of the CMake
    That would be fantastic :smile:
    The only annoying thing is that, until that day, there are now two systems to maintain.
I'd posit that this is better than the previous situation where there was the Linux system,
and then at least, what, three? Windows systems being (not) maintained. If (when) CMake does
get broken on Linux because a change was made to Autotools and not replicated to CMake, it
won't be the end of the world. You'll have one working system, and someone (like me) will
end up fixing the other system. Not the greatest situation, but not the worst.
    So all that said, leaving CMake available for both operating systems I think is the right
thing to do. The scope of impact is still only on Windows, as we're deprecating (deleting)
the previous build files, and we're adding a choice for Linux developers. (When ZooKeeper
3.5.3 is released, I'll replace Mesos's if/else code to build ZK on Linux and Windows with
a _single_ piece of code, using CMake. It'll be awesome.)

> 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 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

View raw message