You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by andschwa <gi...@git.apache.org> on 2017/07/18 17:52:38 UTC
[GitHub] zookeeper pull request #313: ZOOKEEPER-2841: ZooKeeper public include files ...
GitHub user andschwa opened a pull request:
https://github.com/apache/zookeeper/pull/313
ZOOKEEPER-2841: ZooKeeper public include files leak porting changes
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.4
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/313.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 #313
----
commit 8b903e3089a575bbead03275c7d1e8591245cca0
Author: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
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 bf53c535bcda4a92728140770482919055573ba0
Author: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
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.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/313
Regarding `testAsyncWatcherAutoResetterminate`:
I ran c tests on linux and don't observe consistent failure of with this patch.
Otherwise lgtm, I am merging this in 3.4.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/313
[This tree](https://github.com/andschwa/zookeeper/tree/cmake-backport-3.4.8) is the exact patch I tested on Windows with the 3.4.8 tarball.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/313
Actually, @andschwa Can you please update the PR to remove the port of ZOOKEEPER-1643 (https://github.com/apache/zookeeper/pull/313/commits/7776bd61c625c8b363655ab7244fa9f33e5f2198)? I will do a separate commit to pick that up into branch-3.4 after this PR being merged.
Doing it separately avoid messing up two different credentials associated with two separate commits - that is a limitation of current commit script we have. Plus back port of ZOOKEEPER-1643 deserves its separate commit for the record.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/313
@andschwa Thanks for update. I will review this later this week in detail. Regarding the "flaky" test, AFAIK, this one is not particularly flaky (and in general, there are a lot less flaky tests in C client tests comparing to Java tests). The pre-commit build is green though. Did you observe fairly consistent failures for this `Zookeeper_simpleSystem::testAsyncWatcherAutoResetterminate`? One thing we can try is to run this test with and without your patch and see if there are any difference. Re integration tests - if you have time yeah please go ahead and try it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/313
@hanm I figured you'd probably need that given your commit scripts. Fixing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/313
@hanm While the Windows version of Mesos had been using ZooKeeper 3.5.2, the Linux version was using ZooKeeper 3.4.8. I took this patch and cherry-picked back to 3.4.8, and changed the Windows version of Mesos to use 3.4.8 with this patch. However, I also needed 97e598b6c (ZOOKEEPER-1643) in order to build, which is not in `branch-3.4`. Would you be willing to backport ZOOKEEPER-1643 too? If so, then we can switch to the next 3.4 release without any patches at all.
As for `Zookeeper_simpleSystem::testAsyncWatcherAutoReset`, I observed it failing consistently even on `branch-3.4` built with Autotools (i.e. none of my changes at all). I'm inferring that it's a machine issue, and since it didn't fail on Jenkins, I don't think we need to worry about it.
Furthermore, with this patch _and_ ZOOKEEPER-1643 (currently in this branch), I was able to integration test with Mesos on Windows and Linux successfully.
CentOS 7:
```
[==========] Running 7 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 7 tests from ZooKeeperTest
[ RUN ] ZooKeeperTest.Auth
[ OK ] ZooKeeperTest.Auth (6898 ms)
[ RUN ] ZooKeeperTest.SessionTimeoutNegotiation
[ OK ] ZooKeeperTest.SessionTimeoutNegotiation (46 ms)
[ RUN ] ZooKeeperTest.Create
[ OK ] ZooKeeperTest.Create (6728 ms)
[ RUN ] ZooKeeperTest.LeaderDetector
[ OK ] ZooKeeperTest.LeaderDetector (70 ms)
[ RUN ] ZooKeeperTest.LeaderDetectorTimeoutHandling
2017-07-19 12:13:30,626:200313(0x7fd99cb08700):ZOO_ERROR@handle_socket_error_msg@1781: Socket [127.0.0.1:45292] zk retcode=-4, errno=112(Host is down): failed while receiving a server response
2017-07-19 12:13:30,642:200313(0x7fd96cfff700):ZOO_ERROR@handle_socket_error_msg@1757: Socket [127.0.0.1:45292] zk retcode=-4, errno=111(Connection refused): server refused to accept the client
[ OK ] ZooKeeperTest.LeaderDetectorTimeoutHandling (51 ms)
[ RUN ] ZooKeeperTest.LeaderDetectorCancellationHandling
[ OK ] ZooKeeperTest.LeaderDetectorCancellationHandling (74 ms)
[ RUN ] ZooKeeperTest.LeaderContender
2017-07-19 12:13:30,759:200313(0x7fd96cfff700):ZOO_ERROR@handle_socket_error_msg@1781: Socket [127.0.0.1:44849] zk retcode=-4, errno=112(Host is down): failed while receiving a server response
2017-07-19 12:13:34,096:200313(0x7fd96cfff700):ZOO_ERROR@handle_socket_error_msg@1799: Socket [127.0.0.1:44849] zk retcode=-112, errno=116(Stale file handle): sessionId=0x15d5c44fa610000 has expired.
2017-07-19 12:13:34,119:200313(0x7fd99cb08700):ZOO_ERROR@handle_socket_error_msg@1781: Socket [127.0.0.1:44849] zk retcode=-4, errno=112(Host is down): failed while receiving a server response
2017-07-19 12:13:34,121:200313(0x7fd99cb08700):ZOO_ERROR@handle_socket_error_msg@1799: Socket [127.0.0.1:44849] zk retcode=-112, errno=116(Stale file handle): sessionId=0x15d5c44fa610001 has expired.
2017-07-19 12:13:34,142:200313(0x7fd96cfff700):ZOO_ERROR@handle_socket_error_msg@1781: Socket [127.0.0.1:44849] zk retcode=-4, errno=112(Host is down): failed while receiving a server response
2017-07-19 12:13:34,153:200313(0x7fd99cb08700):ZOO_ERROR@handle_socket_error_msg@1757: Socket [127.0.0.1:44849] zk retcode=-4, errno=111(Connection refused): server refused to accept the client
[ OK ] ZooKeeperTest.LeaderContender (6787 ms)
[----------] 7 tests from ZooKeeperTest (20654 ms total)
[----------] Global test environment tear-down
[==========] 7 tests from 1 test case ran. (20795 ms total)
[ PASSED ] 7 tests.
```
Windows 10:
```
[==========] Running 7 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 7 tests from ZooKeeperTest
[ RUN ] ZooKeeperTest.Auth
[ OK ] ZooKeeperTest.Auth (7641 ms)
[ RUN ] ZooKeeperTest.SessionTimeoutNegotiation
[ OK ] ZooKeeperTest.SessionTimeoutNegotiation (71 ms)
[ RUN ] ZooKeeperTest.Create
[ OK ] ZooKeeperTest.Create (7018 ms)
[ RUN ] ZooKeeperTest.LeaderDetector
[ OK ] ZooKeeperTest.LeaderDetector (93 ms)
[ RUN ] ZooKeeperTest.LeaderDetectorTimeoutHandling
2017-07-19 12:13:55,269:18232(0x53d8):ZOO_ERROR@handle_socket_error_msg@1781: Socket [127.0.0.1:57541] zk retcode=-4, errno=32(Broken pipe): failed while receiving a server response
2017-07-19 12:13:55,271:18232(0x53d8):ZOO_ERROR@handle_socket_error_msg@1519: Socket [127.0.0.1:57541] zk retcode=-4, errno=140(Unknown error): failed to send a handshake packet: Unknown error
[ OK ] ZooKeeperTest.LeaderDetectorTimeoutHandling (76 ms)
[ RUN ] ZooKeeperTest.LeaderDetectorCancellationHandling
[ OK ] ZooKeeperTest.LeaderDetectorCancellationHandling (66 ms)
[ RUN ] ZooKeeperTest.LeaderContender
2017-07-19 12:13:55,470:18232(0xaf4):ZOO_ERROR@handle_socket_error_msg@1781: Socket [127.0.0.1:57557] zk retcode=-4, errno=32(Broken pipe): failed while receiving a server response
2017-07-19 12:13:55,473:18232(0xaf4):ZOO_ERROR@handle_socket_error_msg@1799: Socket [127.0.0.1:57557] zk retcode=-112, errno=19(No such device): sessionId=0x15d5c455ad70000 has expired.
2017-07-19 12:13:55,519:18232(0x4dfc):ZOO_ERROR@handle_socket_error_msg@1781: Socket [127.0.0.1:57557] zk retcode=-4, errno=32(Broken pipe): failed while receiving a server response
2017-07-19 12:13:55,523:18232(0x4dfc):ZOO_ERROR@handle_socket_error_msg@1799: Socket [127.0.0.1:57557] zk retcode=-112, errno=19(No such device): sessionId=0x15d5c455ad70001 has expired.
2017-07-19 12:13:55,562:18232(0xc0c):ZOO_ERROR@handle_socket_error_msg@1781: Socket [127.0.0.1:57557] zk retcode=-4, errno=32(Broken pipe): failed while receiving a server response
2017-07-19 12:13:55,564:18232(0xc0c):ZOO_ERROR@handle_socket_error_msg@1519: Socket [127.0.0.1:57557] zk retcode=-4, errno=140(Unknown error): failed to send a handshake packet: Unknown error
[ OK ] ZooKeeperTest.LeaderContender (714 ms)
[----------] 7 tests from ZooKeeperTest (15727 ms total)
[----------] Global test environment tear-down
[==========] 7 tests from 1 test case ran. (16802 ms total)
[ PASSED ] 7 tests.
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/313
It's fixed now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/313
Huzzah!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #313: ZOOKEEPER-2841: ZooKeeper public include files ...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa closed the pull request at:
https://github.com/apache/zookeeper/pull/313
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/313
>> Would you be willing to backport ZOOKEEPER-1643 too?
Sounds good to me. I don't see why this change should not be part of 3.4 release.
>> I observed it failing consistently even on branch-3.4 built with Autotools
Hmm, that is interesting. I did not remember this test was ever on radar. I'll try run it on my env and see what i got.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/313
@andschwa merged, please close this pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/313
@hanm this backports #306 to `branch-3.4`. I think I caught everything, but do give it a review. The build changed a bit between 3.4 and 3.5, so I had to remove some files from `CMakeLists.txt`, and I set the version to `3.4.10`. Then there were some fixes I had to do a bit differently (moved the `#define random` into `zookeeper.c` since `addrvec.c` wasn't created yet, reapply the changes I made to `zookeeper_interest` by hand, and one trivial merge conflict in `recordio.h`).
I build and ran the tests on Linux, all passing save:
```
1: Zookeeper_simpleSystem::testAsyncWatcherAutoResetterminate called after throwing an instance of 'CppUnit::Exception'
1: what(): equality assertion failed
1: - Expected: -101
1: - Actual : -4
```
which is odd. Is it a flaky test, or very machine dependent?
I built on Windows, and it all compiled and linked successfully. If you want, I can integration test it. I probably should anyway so I can move Mesos on Windows back to down to ZK 3.4 instead of 3.5.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---