[
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:
https://github.com/apache/zookeeper/pull/306
@hanm the PR is rebased, retested, and has some extra notes in it now for
some weird CMake things.
I would not put the CMake files in a separate folder. There are couple of
reasons for this:
* 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 _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)
