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/07 22:56:18 UTC
[GitHub] zookeeper pull request #306: ZOOKEEPER-2841: ZooKeeper public include files ...
GitHub user andschwa opened a pull request:
https://github.com/apache/zookeeper/pull/306
ZOOKEEPER-2841: ZooKeeper public include files leak porting changes
This PR includes the patches [ZOOKEEPER-2756](https://issues.apache.org/jira/browse/ZOOKEEPER-2756) and [ZOOKEEPER-2841](https://issues.apache.org/jira/browse/ZOOKEEPER-2841), and can supplant PR #255.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andschwa/zookeeper ZOOKEEPER-2841
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/306.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 #306
----
commit 187ce8acc1707a0dd20752b624a3fa5648706f97
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 22a378ae2a7c30657326b6a602b62116ab7c050a
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 pull request #306: ZOOKEEPER-2841: ZooKeeper public include files ...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/306#discussion_r126292628
--- Diff: src/c/include/winconfig.h ---
@@ -1,196 +1,15 @@
-/* Define to 1 if you have the <arpa/inet.h> header file. */
-#undef HAVE_ARPA_INET_H
-
-/* Define to 1 if you have the <dlfcn.h> header file. */
-#undef HAVE_DLFCN_H
-
-/* Define to 1 if you have the <fcntl.h> header file. */
-#undef HAVE_FCNTL_H
-
-/* Define to 1 if you have the file `generated/zookeeper.jute.c'. */
-#define HAVE_GENERATED_ZOOKEEPER_JUTE_C 1
-
-/* Define to 1 if you have the file `generated/zookeeper.jute.h'. */
-#define HAVE_GENERATED_ZOOKEEPER_JUTE_H 1
-
-/* Define to 1 if you have the `getcwd' function. */
-#undef HAVE_GETCWD
-
-/* Define to 1 if you have the `gethostbyname' function. */
-#undef HAVE_GETHOSTBYNAME
-
-/* Define to 1 if you have the `gethostname' function. */
-#define HAVE_GETHOSTNAME 1
-
-/* Define to 1 if you have the `getlogin' function. */
-#undef HAVE_GETLOGIN
-
-/* Define to 1 if you have the `getpwuid_r' function. */
-#undef HAVE_GETPWUID_R
-
-/* Define to 1 if you have the `gettimeofday' function. */
-#undef HAVE_GETTIMEOFDAY
-
-/* Define to 1 if you have the `getuid' function. */
-#undef HAVE_GETUID
-
-/* Define to 1 if you have the <inttypes.h> header file. */
-#undef HAVE_INTTYPES_H
-
-/* Define to 1 if you have the `memmove' function. */
-#undef HAVE_MEMMOVE
-
-/* Define to 1 if you have the <memory.h> header file. */
-#undef HAVE_MEMORY_H
-
-/* Define to 1 if you have the `memset' function. */
-#undef HAVE_MEMSET
-
-/* Define to 1 if you have the <netdb.h> header file. */
-#undef HAVE_NETDB_H
-
-/* Define to 1 if you have the <netinet/in.h> header file. */
-#undef HAVE_NETINET_IN_H
-
-/* Define to 1 if you have the `poll' function. */
-#undef HAVE_POLL
-
-/* Define to 1 if you have the `socket' function. */
-#undef HAVE_SOCKET
-
-/* Define to 1 if you have the <stdint.h> header file. */
-#undef HAVE_STDINT_H
-
-/* Define to 1 if you have the <stdlib.h> header file. */
-#define HAVE_STDLIB_H 1
-
-/* Define to 1 if you have the `strchr' function. */
-#define HAVE_STRCHR 1
-
-/* Define to 1 if you have the `strdup' function. */
-#define HAVE_STRDUP 1
-
-/* Define to 1 if you have the `strerror' function. */
-#define HAVE_STRERROR 1
-
-/* Define to 1 if you have the <strings.h> header file. */
-#undef HAVE_STRINGS_H
-
-/* Define to 1 if you have the <string.h> header file. */
-#undef HAVE_STRING_H
-
-/* Define to 1 if you have the `strtol' function. */
-#undef HAVE_STRTOL
-
-/* Define to 1 if you have the <sys/socket.h> header file. */
-#undef HAVE_SYS_SOCKET_H
-
-/* Define to 1 if you have the <sys/stat.h> header file. */
-#undef HAVE_SYS_STAT_H
-
-/* Define to 1 if you have the <sys/time.h> header file. */
-#undef HAVE_SYS_TIME_H
-
-/* Define to 1 if you have the <sys/types.h> header file. */
-#undef HAVE_SYS_TYPES_H
-
-/* Define to 1 if you have the <sys/utsname.h> header file. */
-#undef HAVE_SYS_UTSNAME_H
-
-/* Define to 1 if you have the <unistd.h> header file. */
-#undef HAVE_UNISTD_H
-
-/* Define to the sub-directory in which libtool stores uninstalled libraries.
- */
-#define LT_OBJDIR
-
-/* Define to 1 if your C compiler doesn't accept -c and -o together. */
-/* #undef NO_MINUS_C_MINUS_O */
-
-/* Name of package */
-#define PACKAGE "c-client-src"
-
-/* Define to the address where bug reports for this package should be sent. */
-#define PACKAGE_BUGREPORT "user@zookeeper.apache.org"
-
-/* Define to the full name of this package. */
-#define PACKAGE_NAME "zookeeper C client"
-
-/* Define to the full name and version of this package. */
-#define PACKAGE_STRING "zookeeper C client 3.5.0 win32"
-
-/* Define to the one symbol short name of this package. */
-#define PACKAGE_TARNAME "c-client-src"
-
-/* Define to the home page for this package. */
-#define PACKAGE_URL ""
-
-/* Define to the version of this package. */
-#define PACKAGE_VERSION "3.5.0"
--- End diff --
Yes, your observation is correct. We only bump versions encoded in this file of master branch when we do a major release (e.g. from 3.4.x to 3.5.0). The same file in branch-3.5 contains more up to date version number for previous release (3.5.3 for 3.5, and 3.4.10 for 3.4). So this issue is not a problem.
---
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 #306: 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/306
@andschwa Thanks for clarification regarding the coexisting of CMake and makefile. I agree and let's keep the current folder structure, but I think some documentations would be good to help clarify the case. How about document the build toolchain for windows in the README file under the same folder?
---
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 #306: 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/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.
---
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 #306: 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/306
@hanm ah, documentation for it would indeed be good! Can do. Anything else you need for this? If not, I'll get the readme fixed up Monday morning and ping you afterward.
---
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 #306: ZOOKEEPER-2841: ZooKeeper public include files ...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/306#discussion_r126291135
--- Diff: src/c/CMakeLists.txt ---
@@ -0,0 +1,220 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+cmake_minimum_required(VERSION 3.6)
+
+project(zookeeper VERSION 3.5.2)
--- End diff --
I can fix it up in a few minutes here.
---
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 #306: 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/306
>> Is this failing test my fault? It's... odd.
No it's not your fault. This is a known flaky test, and I will have a patch for this soon.
>> I'm not sure how you guys like to do dependent patches.
It's OK to include ZOOKEEPER-2841 in ZOOKEEPER-2756; otherwise it might take years to get the improvement in because the lack of love on the C client. While we are at this, let's get both in.
---
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 #306: 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/306
@hanm I ended up just adding it [straight to the src/c/README](https://github.com/apache/zookeeper/pull/306/files#diff-a722fe5ba032cb8da6c78d0e929f4ac5R74), let me know if you think it should have more/less etc.
---
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 #306: ZOOKEEPER-2841: ZooKeeper public include files ...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/306#discussion_r126290881
--- Diff: src/c/include/winconfig.h ---
@@ -1,196 +1,15 @@
-/* Define to 1 if you have the <arpa/inet.h> header file. */
-#undef HAVE_ARPA_INET_H
-
-/* Define to 1 if you have the <dlfcn.h> header file. */
-#undef HAVE_DLFCN_H
-
-/* Define to 1 if you have the <fcntl.h> header file. */
-#undef HAVE_FCNTL_H
-
-/* Define to 1 if you have the file `generated/zookeeper.jute.c'. */
-#define HAVE_GENERATED_ZOOKEEPER_JUTE_C 1
-
-/* Define to 1 if you have the file `generated/zookeeper.jute.h'. */
-#define HAVE_GENERATED_ZOOKEEPER_JUTE_H 1
-
-/* Define to 1 if you have the `getcwd' function. */
-#undef HAVE_GETCWD
-
-/* Define to 1 if you have the `gethostbyname' function. */
-#undef HAVE_GETHOSTBYNAME
-
-/* Define to 1 if you have the `gethostname' function. */
-#define HAVE_GETHOSTNAME 1
-
-/* Define to 1 if you have the `getlogin' function. */
-#undef HAVE_GETLOGIN
-
-/* Define to 1 if you have the `getpwuid_r' function. */
-#undef HAVE_GETPWUID_R
-
-/* Define to 1 if you have the `gettimeofday' function. */
-#undef HAVE_GETTIMEOFDAY
-
-/* Define to 1 if you have the `getuid' function. */
-#undef HAVE_GETUID
-
-/* Define to 1 if you have the <inttypes.h> header file. */
-#undef HAVE_INTTYPES_H
-
-/* Define to 1 if you have the `memmove' function. */
-#undef HAVE_MEMMOVE
-
-/* Define to 1 if you have the <memory.h> header file. */
-#undef HAVE_MEMORY_H
-
-/* Define to 1 if you have the `memset' function. */
-#undef HAVE_MEMSET
-
-/* Define to 1 if you have the <netdb.h> header file. */
-#undef HAVE_NETDB_H
-
-/* Define to 1 if you have the <netinet/in.h> header file. */
-#undef HAVE_NETINET_IN_H
-
-/* Define to 1 if you have the `poll' function. */
-#undef HAVE_POLL
-
-/* Define to 1 if you have the `socket' function. */
-#undef HAVE_SOCKET
-
-/* Define to 1 if you have the <stdint.h> header file. */
-#undef HAVE_STDINT_H
-
-/* Define to 1 if you have the <stdlib.h> header file. */
-#define HAVE_STDLIB_H 1
-
-/* Define to 1 if you have the `strchr' function. */
-#define HAVE_STRCHR 1
-
-/* Define to 1 if you have the `strdup' function. */
-#define HAVE_STRDUP 1
-
-/* Define to 1 if you have the `strerror' function. */
-#define HAVE_STRERROR 1
-
-/* Define to 1 if you have the <strings.h> header file. */
-#undef HAVE_STRINGS_H
-
-/* Define to 1 if you have the <string.h> header file. */
-#undef HAVE_STRING_H
-
-/* Define to 1 if you have the `strtol' function. */
-#undef HAVE_STRTOL
-
-/* Define to 1 if you have the <sys/socket.h> header file. */
-#undef HAVE_SYS_SOCKET_H
-
-/* Define to 1 if you have the <sys/stat.h> header file. */
-#undef HAVE_SYS_STAT_H
-
-/* Define to 1 if you have the <sys/time.h> header file. */
-#undef HAVE_SYS_TIME_H
-
-/* Define to 1 if you have the <sys/types.h> header file. */
-#undef HAVE_SYS_TYPES_H
-
-/* Define to 1 if you have the <sys/utsname.h> header file. */
-#undef HAVE_SYS_UTSNAME_H
-
-/* Define to 1 if you have the <unistd.h> header file. */
-#undef HAVE_UNISTD_H
-
-/* Define to the sub-directory in which libtool stores uninstalled libraries.
- */
-#define LT_OBJDIR
-
-/* Define to 1 if your C compiler doesn't accept -c and -o together. */
-/* #undef NO_MINUS_C_MINUS_O */
-
-/* Name of package */
-#define PACKAGE "c-client-src"
-
-/* Define to the address where bug reports for this package should be sent. */
-#define PACKAGE_BUGREPORT "user@zookeeper.apache.org"
-
-/* Define to the full name of this package. */
-#define PACKAGE_NAME "zookeeper C client"
-
-/* Define to the full name and version of this package. */
-#define PACKAGE_STRING "zookeeper C client 3.5.0 win32"
-
-/* Define to the one symbol short name of this package. */
-#define PACKAGE_TARNAME "c-client-src"
-
-/* Define to the home page for this package. */
-#define PACKAGE_URL ""
-
-/* Define to the version of this package. */
-#define PACKAGE_VERSION "3.5.0"
--- End diff --
This 3.5.0 indicates the base version of this file you were using when start the patch is pretty old. Could you rebase the pull request on latest master?
---
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 #306: 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/306
@hanm Awesome! The patches seem to be working well, and I think we don't need to worry about maintaining compatibility with, say, Visual Studio 2008, in new versions of the ZooKeeper client. Old versions of the client are still available for those in need, and it's more important that current versions of VS/MSVC are supported.
Thanks for your help in this!
---
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 #306: 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/306
Thanks for all your help @hanm! I added a PR for the `branch-3.4` port too.
---
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 #306: 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/306
If we want two commits, then we need two pull requests. Two (or more ) commits appertain to a single pull request will be squashed to a single commit at commit time by our commit script. I think it's fine to have this pull request resolve both issue with a single commit.
I can massage the commit message at commit time to include the messages from both commit, or you could update the pull request description with both message, I am ok with either way.
---
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 #306: 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/306
Hey, yay, Jenkins passed!
---
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 #306: 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/306
@hanm ah, I see! That is no problem. I have taken the two messages, improved them a bit as one message, and updated the description. It will be easier to backport it this way.
---
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 #306: 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/306
Should we put the CMake files in a dedicated folder called "windows" or something like that? Otherwise we will have the CMakeList file, and the existing makefile and config files (which are not touched by this pull request) coexisting under the C client folder. This might create confusions for user because makefile can be generated from the CMakeList file, so the question of which makefile is the source of truth and so on...
Ultimately we might drop existing makefiles in favor of the CMake so build files across every platform are generated by a single source of truth (cmakelist), but that's more scope of work (the ant build file has to be modified and the system requirements of ZK has to be updated to make cmake a dependency for source installations etc to avoid break existing workflows), so for now I think it's good to scope cmake so it only impact windows C client builds.
---
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 #306: 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/306
@hanm I'm going to let you take care of the title, maybe it should be:
> ZOOKEEPER-2756 and ZOOKEEPER-2841: Improve cross-platform support with CMake and refactor
But I don't know.
---
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 #306: 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/306
@hanm I'm likewise giving this some more testing too. I'm integrating the patch into Mesos on the Linux side (where we were building with Autotools), and porting our ZooKeeper-explicit unit tests to Windows (because apparently they weren't). We do have tests that I've run that indirectly tested the ZooKeeper client, but I want certain tests. Expect a small update by the end of the week.
---
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 #306: 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/306
@hanm I'm not sure how you guys like to do dependent patches. ZOOKEEPER-2756 and ZOOKEEPER-2841 should both be taken to resolve problems using the C client in other projects (e.g. Mesos 😉), and the latter depends on the former. I've confirmed that 2756 (first commit) allows us to build with Visual Studio 2017 (and on Linux, because CMake!), and that 2841 (second commit) resolves [MESOS-7541](https://issues.apache.org/jira/browse/MESOS-7541), and should eliminate a lot of problems that projects which include ZooKeeper headers can encounter on Windows. That said, the two patches are clearly two very separate pieces of work.
---
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 #306: 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/306
looks good. test failures are irrelevant (we should really get the flaky tests fixed soon.).
---
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 #306: ZOOKEEPER-2841: ZooKeeper public include files ...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/306#discussion_r126292294
--- Diff: src/c/include/winconfig.h ---
@@ -1,196 +1,15 @@
-/* Define to 1 if you have the <arpa/inet.h> header file. */
-#undef HAVE_ARPA_INET_H
-
-/* Define to 1 if you have the <dlfcn.h> header file. */
-#undef HAVE_DLFCN_H
-
-/* Define to 1 if you have the <fcntl.h> header file. */
-#undef HAVE_FCNTL_H
-
-/* Define to 1 if you have the file `generated/zookeeper.jute.c'. */
-#define HAVE_GENERATED_ZOOKEEPER_JUTE_C 1
-
-/* Define to 1 if you have the file `generated/zookeeper.jute.h'. */
-#define HAVE_GENERATED_ZOOKEEPER_JUTE_H 1
-
-/* Define to 1 if you have the `getcwd' function. */
-#undef HAVE_GETCWD
-
-/* Define to 1 if you have the `gethostbyname' function. */
-#undef HAVE_GETHOSTBYNAME
-
-/* Define to 1 if you have the `gethostname' function. */
-#define HAVE_GETHOSTNAME 1
-
-/* Define to 1 if you have the `getlogin' function. */
-#undef HAVE_GETLOGIN
-
-/* Define to 1 if you have the `getpwuid_r' function. */
-#undef HAVE_GETPWUID_R
-
-/* Define to 1 if you have the `gettimeofday' function. */
-#undef HAVE_GETTIMEOFDAY
-
-/* Define to 1 if you have the `getuid' function. */
-#undef HAVE_GETUID
-
-/* Define to 1 if you have the <inttypes.h> header file. */
-#undef HAVE_INTTYPES_H
-
-/* Define to 1 if you have the `memmove' function. */
-#undef HAVE_MEMMOVE
-
-/* Define to 1 if you have the <memory.h> header file. */
-#undef HAVE_MEMORY_H
-
-/* Define to 1 if you have the `memset' function. */
-#undef HAVE_MEMSET
-
-/* Define to 1 if you have the <netdb.h> header file. */
-#undef HAVE_NETDB_H
-
-/* Define to 1 if you have the <netinet/in.h> header file. */
-#undef HAVE_NETINET_IN_H
-
-/* Define to 1 if you have the `poll' function. */
-#undef HAVE_POLL
-
-/* Define to 1 if you have the `socket' function. */
-#undef HAVE_SOCKET
-
-/* Define to 1 if you have the <stdint.h> header file. */
-#undef HAVE_STDINT_H
-
-/* Define to 1 if you have the <stdlib.h> header file. */
-#define HAVE_STDLIB_H 1
-
-/* Define to 1 if you have the `strchr' function. */
-#define HAVE_STRCHR 1
-
-/* Define to 1 if you have the `strdup' function. */
-#define HAVE_STRDUP 1
-
-/* Define to 1 if you have the `strerror' function. */
-#define HAVE_STRERROR 1
-
-/* Define to 1 if you have the <strings.h> header file. */
-#undef HAVE_STRINGS_H
-
-/* Define to 1 if you have the <string.h> header file. */
-#undef HAVE_STRING_H
-
-/* Define to 1 if you have the `strtol' function. */
-#undef HAVE_STRTOL
-
-/* Define to 1 if you have the <sys/socket.h> header file. */
-#undef HAVE_SYS_SOCKET_H
-
-/* Define to 1 if you have the <sys/stat.h> header file. */
-#undef HAVE_SYS_STAT_H
-
-/* Define to 1 if you have the <sys/time.h> header file. */
-#undef HAVE_SYS_TIME_H
-
-/* Define to 1 if you have the <sys/types.h> header file. */
-#undef HAVE_SYS_TYPES_H
-
-/* Define to 1 if you have the <sys/utsname.h> header file. */
-#undef HAVE_SYS_UTSNAME_H
-
-/* Define to 1 if you have the <unistd.h> header file. */
-#undef HAVE_UNISTD_H
-
-/* Define to the sub-directory in which libtool stores uninstalled libraries.
- */
-#define LT_OBJDIR
-
-/* Define to 1 if your C compiler doesn't accept -c and -o together. */
-/* #undef NO_MINUS_C_MINUS_O */
-
-/* Name of package */
-#define PACKAGE "c-client-src"
-
-/* Define to the address where bug reports for this package should be sent. */
-#define PACKAGE_BUGREPORT "user@zookeeper.apache.org"
-
-/* Define to the full name of this package. */
-#define PACKAGE_NAME "zookeeper C client"
-
-/* Define to the full name and version of this package. */
-#define PACKAGE_STRING "zookeeper C client 3.5.0 win32"
-
-/* Define to the one symbol short name of this package. */
-#define PACKAGE_TARNAME "c-client-src"
-
-/* Define to the home page for this package. */
-#define PACKAGE_URL ""
-
-/* Define to the version of this package. */
-#define PACKAGE_VERSION "3.5.0"
--- End diff --
(Actually, this is old because `winconfig.h` is unmaintained. I just checked the tip of `master`, it's still 3.5.0 in this file. My changes make the set based off the single version variable in the CMake build file instead.)
---
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 #306: ZOOKEEPER-2841: ZooKeeper public include files ...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/306#discussion_r126291131
--- Diff: src/c/include/winconfig.h ---
@@ -1,196 +1,15 @@
-/* Define to 1 if you have the <arpa/inet.h> header file. */
-#undef HAVE_ARPA_INET_H
-
-/* Define to 1 if you have the <dlfcn.h> header file. */
-#undef HAVE_DLFCN_H
-
-/* Define to 1 if you have the <fcntl.h> header file. */
-#undef HAVE_FCNTL_H
-
-/* Define to 1 if you have the file `generated/zookeeper.jute.c'. */
-#define HAVE_GENERATED_ZOOKEEPER_JUTE_C 1
-
-/* Define to 1 if you have the file `generated/zookeeper.jute.h'. */
-#define HAVE_GENERATED_ZOOKEEPER_JUTE_H 1
-
-/* Define to 1 if you have the `getcwd' function. */
-#undef HAVE_GETCWD
-
-/* Define to 1 if you have the `gethostbyname' function. */
-#undef HAVE_GETHOSTBYNAME
-
-/* Define to 1 if you have the `gethostname' function. */
-#define HAVE_GETHOSTNAME 1
-
-/* Define to 1 if you have the `getlogin' function. */
-#undef HAVE_GETLOGIN
-
-/* Define to 1 if you have the `getpwuid_r' function. */
-#undef HAVE_GETPWUID_R
-
-/* Define to 1 if you have the `gettimeofday' function. */
-#undef HAVE_GETTIMEOFDAY
-
-/* Define to 1 if you have the `getuid' function. */
-#undef HAVE_GETUID
-
-/* Define to 1 if you have the <inttypes.h> header file. */
-#undef HAVE_INTTYPES_H
-
-/* Define to 1 if you have the `memmove' function. */
-#undef HAVE_MEMMOVE
-
-/* Define to 1 if you have the <memory.h> header file. */
-#undef HAVE_MEMORY_H
-
-/* Define to 1 if you have the `memset' function. */
-#undef HAVE_MEMSET
-
-/* Define to 1 if you have the <netdb.h> header file. */
-#undef HAVE_NETDB_H
-
-/* Define to 1 if you have the <netinet/in.h> header file. */
-#undef HAVE_NETINET_IN_H
-
-/* Define to 1 if you have the `poll' function. */
-#undef HAVE_POLL
-
-/* Define to 1 if you have the `socket' function. */
-#undef HAVE_SOCKET
-
-/* Define to 1 if you have the <stdint.h> header file. */
-#undef HAVE_STDINT_H
-
-/* Define to 1 if you have the <stdlib.h> header file. */
-#define HAVE_STDLIB_H 1
-
-/* Define to 1 if you have the `strchr' function. */
-#define HAVE_STRCHR 1
-
-/* Define to 1 if you have the `strdup' function. */
-#define HAVE_STRDUP 1
-
-/* Define to 1 if you have the `strerror' function. */
-#define HAVE_STRERROR 1
-
-/* Define to 1 if you have the <strings.h> header file. */
-#undef HAVE_STRINGS_H
-
-/* Define to 1 if you have the <string.h> header file. */
-#undef HAVE_STRING_H
-
-/* Define to 1 if you have the `strtol' function. */
-#undef HAVE_STRTOL
-
-/* Define to 1 if you have the <sys/socket.h> header file. */
-#undef HAVE_SYS_SOCKET_H
-
-/* Define to 1 if you have the <sys/stat.h> header file. */
-#undef HAVE_SYS_STAT_H
-
-/* Define to 1 if you have the <sys/time.h> header file. */
-#undef HAVE_SYS_TIME_H
-
-/* Define to 1 if you have the <sys/types.h> header file. */
-#undef HAVE_SYS_TYPES_H
-
-/* Define to 1 if you have the <sys/utsname.h> header file. */
-#undef HAVE_SYS_UTSNAME_H
-
-/* Define to 1 if you have the <unistd.h> header file. */
-#undef HAVE_UNISTD_H
-
-/* Define to the sub-directory in which libtool stores uninstalled libraries.
- */
-#define LT_OBJDIR
-
-/* Define to 1 if your C compiler doesn't accept -c and -o together. */
-/* #undef NO_MINUS_C_MINUS_O */
-
-/* Name of package */
-#define PACKAGE "c-client-src"
-
-/* Define to the address where bug reports for this package should be sent. */
-#define PACKAGE_BUGREPORT "user@zookeeper.apache.org"
-
-/* Define to the full name of this package. */
-#define PACKAGE_NAME "zookeeper C client"
-
-/* Define to the full name and version of this package. */
-#define PACKAGE_STRING "zookeeper C client 3.5.0 win32"
-
-/* Define to the one symbol short name of this package. */
-#define PACKAGE_TARNAME "c-client-src"
-
-/* Define to the home page for this package. */
-#define PACKAGE_URL ""
-
-/* Define to the version of this package. */
-#define PACKAGE_VERSION "3.5.0"
--- End diff --
It should be freshly rebased as of yesterday, but yeah I didn't catch the version update.
---
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 #306: 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/306
@andschwa This patch just landed on master. Thanks for your contribution. I'll review the branch-3.5 port and commit it later this week if it looks good.
---
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 #306: 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/306
This [here](https://github.com/apache/zookeeper/pull/306/files#diff-1ea258ac6b9efc24b67d2c2a704cfe5fR145) is all I changed; it ensures that the `config.h` file is always put into the source `include` directory, which is what upstream projects include. This was needed because `zookeeper.h` includes the file, so this makes sure it exists next to it (instead of in the binary directory, which works for the ZK build, but if it's out-of-tree, is misplaced for upstream consumption).
---
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 #306: 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/306
Is this [failing test](https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/870/testReport/org.apache.zookeeper.server.quorum/ReconfigDuringLeaderSyncTest/testDuringLeaderSync/) my fault? It's... odd.
---
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 #306: 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/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.)
---
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 #306: 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/306
@andschwa
Patch looks good to me. The readme update looks good too.
I also verified Linux and Mac c client builds with the patch. Unfortunately I don't have a windows box to test. Can you please describe what kinds of test / integration test you did on windows?
There are two remaining issues:
* 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.
* 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?
---
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 #306: 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/306
Thanks for update. LGTM. Will merge by end of this week. Give a few more days in case someone has additional feedback for this.
---
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 #306: 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/306
Thanks for the extra efforts on integration testing, @andschwa. I'll commit this tomorrow.
---
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 #306: ZOOKEEPER-2841: ZooKeeper public include files ...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/306#discussion_r126294494
--- Diff: src/c/include/winconfig.h ---
@@ -1,196 +1,15 @@
-/* Define to 1 if you have the <arpa/inet.h> header file. */
-#undef HAVE_ARPA_INET_H
-
-/* Define to 1 if you have the <dlfcn.h> header file. */
-#undef HAVE_DLFCN_H
-
-/* Define to 1 if you have the <fcntl.h> header file. */
-#undef HAVE_FCNTL_H
-
-/* Define to 1 if you have the file `generated/zookeeper.jute.c'. */
-#define HAVE_GENERATED_ZOOKEEPER_JUTE_C 1
-
-/* Define to 1 if you have the file `generated/zookeeper.jute.h'. */
-#define HAVE_GENERATED_ZOOKEEPER_JUTE_H 1
-
-/* Define to 1 if you have the `getcwd' function. */
-#undef HAVE_GETCWD
-
-/* Define to 1 if you have the `gethostbyname' function. */
-#undef HAVE_GETHOSTBYNAME
-
-/* Define to 1 if you have the `gethostname' function. */
-#define HAVE_GETHOSTNAME 1
-
-/* Define to 1 if you have the `getlogin' function. */
-#undef HAVE_GETLOGIN
-
-/* Define to 1 if you have the `getpwuid_r' function. */
-#undef HAVE_GETPWUID_R
-
-/* Define to 1 if you have the `gettimeofday' function. */
-#undef HAVE_GETTIMEOFDAY
-
-/* Define to 1 if you have the `getuid' function. */
-#undef HAVE_GETUID
-
-/* Define to 1 if you have the <inttypes.h> header file. */
-#undef HAVE_INTTYPES_H
-
-/* Define to 1 if you have the `memmove' function. */
-#undef HAVE_MEMMOVE
-
-/* Define to 1 if you have the <memory.h> header file. */
-#undef HAVE_MEMORY_H
-
-/* Define to 1 if you have the `memset' function. */
-#undef HAVE_MEMSET
-
-/* Define to 1 if you have the <netdb.h> header file. */
-#undef HAVE_NETDB_H
-
-/* Define to 1 if you have the <netinet/in.h> header file. */
-#undef HAVE_NETINET_IN_H
-
-/* Define to 1 if you have the `poll' function. */
-#undef HAVE_POLL
-
-/* Define to 1 if you have the `socket' function. */
-#undef HAVE_SOCKET
-
-/* Define to 1 if you have the <stdint.h> header file. */
-#undef HAVE_STDINT_H
-
-/* Define to 1 if you have the <stdlib.h> header file. */
-#define HAVE_STDLIB_H 1
-
-/* Define to 1 if you have the `strchr' function. */
-#define HAVE_STRCHR 1
-
-/* Define to 1 if you have the `strdup' function. */
-#define HAVE_STRDUP 1
-
-/* Define to 1 if you have the `strerror' function. */
-#define HAVE_STRERROR 1
-
-/* Define to 1 if you have the <strings.h> header file. */
-#undef HAVE_STRINGS_H
-
-/* Define to 1 if you have the <string.h> header file. */
-#undef HAVE_STRING_H
-
-/* Define to 1 if you have the `strtol' function. */
-#undef HAVE_STRTOL
-
-/* Define to 1 if you have the <sys/socket.h> header file. */
-#undef HAVE_SYS_SOCKET_H
-
-/* Define to 1 if you have the <sys/stat.h> header file. */
-#undef HAVE_SYS_STAT_H
-
-/* Define to 1 if you have the <sys/time.h> header file. */
-#undef HAVE_SYS_TIME_H
-
-/* Define to 1 if you have the <sys/types.h> header file. */
-#undef HAVE_SYS_TYPES_H
-
-/* Define to 1 if you have the <sys/utsname.h> header file. */
-#undef HAVE_SYS_UTSNAME_H
-
-/* Define to 1 if you have the <unistd.h> header file. */
-#undef HAVE_UNISTD_H
-
-/* Define to the sub-directory in which libtool stores uninstalled libraries.
- */
-#define LT_OBJDIR
-
-/* Define to 1 if your C compiler doesn't accept -c and -o together. */
-/* #undef NO_MINUS_C_MINUS_O */
-
-/* Name of package */
-#define PACKAGE "c-client-src"
-
-/* Define to the address where bug reports for this package should be sent. */
-#define PACKAGE_BUGREPORT "user@zookeeper.apache.org"
-
-/* Define to the full name of this package. */
-#define PACKAGE_NAME "zookeeper C client"
-
-/* Define to the full name and version of this package. */
-#define PACKAGE_STRING "zookeeper C client 3.5.0 win32"
-
-/* Define to the one symbol short name of this package. */
-#define PACKAGE_TARNAME "c-client-src"
-
-/* Define to the home page for this package. */
-#define PACKAGE_URL ""
-
-/* Define to the version of this package. */
-#define PACKAGE_VERSION "3.5.0"
--- End diff --
Ah, that makes more sense!
---
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 #306: ZOOKEEPER-2841: ZooKeeper public include files ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/306
---
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 #306: 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/306
I managed to get the Mesos unit tests for ZooKeeper ported to our CMake system, which much more thoroughly exercises the C client. I've integrated this patch and the CMake system with the Mesos build on Linux, and all our tests passed:
```
[----------] 7 tests from ZooKeeperTest
[ RUN ] ZooKeeperTest.Auth
[ OK ] ZooKeeperTest.Auth (6923 ms)
[ RUN ] ZooKeeperTest.SessionTimeoutNegotiation
[ OK ] ZooKeeperTest.SessionTimeoutNegotiation (46 ms)
[ RUN ] ZooKeeperTest.Create
[ OK ] ZooKeeperTest.Create (6770 ms)
[ RUN ] ZooKeeperTest.LeaderDetector
[ OK ] ZooKeeperTest.LeaderDetector (57 ms)
[ RUN ] ZooKeeperTest.LeaderDetectorTimeoutHandling
2017-07-14 15:23:18,510:29970(0x7f01e4d0a700):ZOO_ERROR@handle_socket_error_msg@2409: Socket [127.0.0.1:43434] zk retcode=-4, errno=112(Host is down): failed while receiving a server response
2017-07-14 15:23:18,510:29970(0x7f01e4d0a700):ZOO_ERROR@handle_socket_error_msg@2384: Socket [127.0.0.1:43434] zk retcode=-4, errno=111(Connection refused): server refused to accept the client
2017-07-14 15:23:18,514:29970(0x7f01dffff700):ZOO_ERROR@handle_socket_error_msg@2384: Socket [127.0.0.1:43434] zk retcode=-4, errno=111(Connection refused): server refused to accept the client
2017-07-14 15:23:18,514:29970(0x7f01dffff700):ZOO_ERROR@handle_socket_error_msg@2384: Socket [127.0.0.1:43434] zk retcode=-4, errno=111(Connection refused): server refused to accept the client
[ OK ] ZooKeeperTest.LeaderDetectorTimeoutHandling (50 ms)
[ RUN ] ZooKeeperTest.LeaderDetectorCancellationHandling
[ OK ] ZooKeeperTest.LeaderDetectorCancellationHandling (54 ms)
[ RUN ] ZooKeeperTest.LeaderContender
2017-07-14 15:23:18,630:29970(0x7f01dffff700):ZOO_ERROR@handle_socket_error_msg@2409: Socket [127.0.0.1:38085] zk retcode=-4, errno=112(Host is down): failed while receiving a server response
2017-07-14 15:23:18,632:29970(0x7f01dffff700):ZOO_ERROR@handle_socket_error_msg@2428: Socket [127.0.0.1:38085] zk retcode=-112, errno=116(Stale file handle): sessionId=0x10000305fd80000 has expired.
2017-07-14 15:23:18,655:29970(0x7f01e4d0a700):ZOO_ERROR@handle_socket_error_msg@2409: Socket [127.0.0.1:38085] zk retcode=-4, errno=112(Host is down): failed while receiving a server response
2017-07-14 15:23:18,657:29970(0x7f01e4d0a700):ZOO_ERROR@handle_socket_error_msg@2428: Socket [127.0.0.1:38085] zk retcode=-112, errno=116(Stale file handle): sessionId=0x10000305fd80001 has expired.
2017-07-14 15:23:18,676:29970(0x7f01dffff700):ZOO_ERROR@handle_socket_error_msg@2409: Socket [127.0.0.1:38085] zk retcode=-4, errno=112(Host is down): failed while receiving a server response
2017-07-14 15:23:18,677:29970(0x7f01dffff700):ZOO_ERROR@handle_socket_error_msg@2384: Socket [127.0.0.1:38085] zk retcode=-4, errno=111(Connection refused): server refused to accept the client
2017-07-14 15:23:18,688:29970(0x7f01e4d0a700):ZOO_ERROR@handle_socket_error_msg@2384: Socket [127.0.0.1:38085] zk retcode=-4, errno=111(Connection refused): server refused to accept the client
2017-07-14 15:23:18,688:29970(0x7f01e4d0a700):ZOO_ERROR@handle_socket_error_msg@2384: Socket [127.0.0.1:38085] zk retcode=-4, errno=111(Connection refused): server refused to accept the client
[ OK ] ZooKeeperTest.LeaderContender (304 ms)
[----------] 7 tests from ZooKeeperTest (14204 ms total)
[----------] Global test environment tear-down
[==========] 7 tests from 1 test case ran. (14376 ms total)
[ PASSED ] 7 tests.
```
And then I brought these changes over to Windows too, and while it's currently building with ~some~ a lot of irrelevant-to-this-patch hacks, they pass:
```
[----------] 7 tests from ZooKeeperTest
[ RUN ] ZooKeeperTest.Auth
[ OK ] ZooKeeperTest.Auth (7101 ms)
[ RUN ] ZooKeeperTest.SessionTimeoutNegotiation
[ OK ] ZooKeeperTest.SessionTimeoutNegotiation (737 ms)
[ RUN ] ZooKeeperTest.Create
[ OK ] ZooKeeperTest.Create (6752 ms)
[ RUN ] ZooKeeperTest.LeaderDetector
[ OK ] ZooKeeperTest.LeaderDetector (107 ms)
[ RUN ] ZooKeeperTest.LeaderDetectorTimeoutHandling
2017-07-17 12:52:08,546:15936(0x3334):ZOO_ERROR@handle_socket_error_msg@2409: Socket [127.0.0.1:54312] zk retcode=-4, errno=32(Broken pipe): failed while receiving a server response
[ OK ] ZooKeeperTest.LeaderDetectorTimeoutHandling (68 ms)
[ RUN ] ZooKeeperTest.LeaderDetectorCancellationHandling
[ OK ] ZooKeeperTest.LeaderDetectorCancellationHandling (72 ms)
[ RUN ] ZooKeeperTest.LeaderContender
2017-07-17 12:52:08,709:15936(0x47c8):ZOO_ERROR@handle_socket_error_msg@2409: Socket [127.0.0.1:54336] zk retcode=-4, errno=32(Broken pipe): failed while receiving a server response
2017-07-17 12:52:08,714:15936(0x47c8):ZOO_ERROR@handle_socket_error_msg@2428: Socket [127.0.0.1:54336] zk retcode=-112, errno=19(No such device): sessionId=0x1001b9dddba0000 has expired.
2017-07-17 12:52:08,749:15936(0x39b8):ZOO_ERROR@handle_socket_error_msg@2409: Socket [127.0.0.1:54336] zk retcode=-4, errno=32(Broken pipe): failed while receiving a server response
2017-07-17 12:52:08,754:15936(0x39b8):ZOO_ERROR@handle_socket_error_msg@2428: Socket [127.0.0.1:54336] zk retcode=-112, errno=19(No such device): sessionId=0x1001b9dddba0001 has expired.
2017-07-17 12:52:08,788:15936(0xb94):ZOO_ERROR@handle_socket_error_msg@2409: Socket [127.0.0.1:54336] zk retcode=-4, errno=32(Broken pipe): failed while receiving a server response
[ OK ] ZooKeeperTest.LeaderContender (701 ms)
[----------] 7 tests from ZooKeeperTest (15548 ms total)
[----------] Global test environment tear-down
[==========] 7 tests from 1 test case ran. (16724 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 #306: 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/306
Just to give you an idea of what we currently have to do in order to use the ZooKeeper C Client in Mesos, we have [this](https://github.com/apache/mesos/blob/b4f8d1f588841b0a7395c24630963be5620db7cb/3rdparty/cmake/Mesos3rdpartyConfigure.cmake#L51) and [this](https://github.com/apache/mesos/blob/b4f8d1f588841b0a7395c24630963be5620db7cb/3rdparty/zookeeper-3.5.2-alpha.patch) and [this](https://github.com/apache/mesos/blob/b4f8d1f588841b0a7395c24630963be5620db7cb/3rdparty/CMakeLists.txt#L394), all to work around the current state of the client build system.
My goal is that, with these changes, almost everything can be replaced with just this [one piece of code](https://github.com/apache/mesos/blob/b4f8d1f588841b0a7395c24630963be5620db7cb/3rdparty/CMakeLists.txt#L424).
---
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 #306: ZOOKEEPER-2841: ZooKeeper public include files ...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/306#discussion_r126290847
--- Diff: src/c/CMakeLists.txt ---
@@ -0,0 +1,220 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+cmake_minimum_required(VERSION 3.6)
+
+project(zookeeper VERSION 3.5.2)
--- End diff --
The version here should be 3.5.3. I think you got 3.5.2 because when you start working on this, 3.5.3 was not released.
---
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.
---