[GitHub] zookeeper issue #505: ZOOKEEPER-3025: Make `hashtable` search `include`
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/505 Built successfully both in-tree and out-of-tree on Windows using the VS generator, also successfully built with the Ninja generator ð ---
[GitHub] zookeeper pull request #505: ZOOKEEPER-3025: Make `hashtable` search `includ...
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/505 ZOOKEEPER-3025: Make `hashtable` search `include` When ZOOKEEPER-2999 removed the directory side-effect of `include_directories(include)`, and added it as a target-level include to the `zookeeper` library, this broke the Windows build. It worked on Linux (where the patch was tested) because `winconfig.h` and is not included, but on Windows, the `hashtable` library included `winconfig.h` (found in `include`) but no longer knew where to find it. The fix is to add `include` to the `hashtable` library's list of include directories. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper ZOOKEEPER-3025 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/505.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 #505 commit 69afce4df5a5cc71999d67c87578e236b58d17a1 Author: Andrew Schwartzmeyer Date: 2018-04-23T23:38:03Z ZOOKEEPER-3025: Make `hashtable` search `include` When ZOOKEEPER-2999 removed the directory side-effect of `include_directories(include)`, and added it as a target-level include to the `zookeeper` library, this broke the Windows build. It worked on Linux (where the patch was tested) because `winconfig.h` and is not included, but on Windows, the `hashtable` library included `winconfig.h` (found in `include`) but no longer knew where to find it. The fix is to add `include` to the `hashtable` library's list of include directories. ---
[GitHub] zookeeper issue #499: ZOOKEEPER-3017: Link libm in CMake on FreeBSD.
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/499 Thanks @hanm and @anmolnar! ---
[GitHub] zookeeper issue #486: ZOOKEEPER-2999: CMake build should use target-level co...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/486 Thanks @hanm! ---
[GitHub] zookeeper issue #486: ZOOKEEPER-2999: CMake build should use target-level co...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/486 @anmolnar Yay! Flaky tests passed CI. ---
[GitHub] zookeeper issue #499: ZOOKEEPER-3017: Link libm in CMake on FreeBSD.
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/499 Yay @anmolnar the tests were flaky, last CI run was clean. ---
[GitHub] zookeeper issue #486: ZOOKEEPER-2999: CMake build should use target-level co...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/486 @anmolnar Same here. The changes made in this PR are not executed in the Jenkins build; but the tests are quite flaky. ---
[GitHub] zookeeper issue #499: ZOOKEEPER-3017: Link libm in CMake on FreeBSD.
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/499 The change is this: ``` -target_link_libraries(hashtable PUBLIC $<$:m>) +target_link_libraries(hashtable PUBLIC $<$,$>:m>) ``` This code is not executed on Jenkins, so I don't see how it could be. I double checked, it's still running the Autotools build, here's a snippet: ``` [exec] [exec] checking that generated files are newer than configure... done [exec] [exec] configure: creating ./config.status [exec] [exec] config.status: creating Makefile [exec] [exec] config.status: creating config.h [exec] [exec] config.status: executing depfiles commands [exec] [exec] config.status: executing libtool commands ``` ---
[GitHub] zookeeper issue #499: ZOOKEEPER-3017: Link libm in CMake on FreeBSD.
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/499 @anmolnar I can't for the life of me find which unit test failed in the Jenkins build. It can't be related though, as this just adds `libm` to the link line on FreeBSD in addition to Linux, when using CMake (which the Jenkins build, to my knowledge, does not yet use). ---
[GitHub] zookeeper issue #499: ZOOKEEPER-3017: Link libm in CMake on FreeBSD.
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/499 @dforsyth Can you rebase just to force another build? ---
[GitHub] zookeeper issue #486: ZOOKEEPER-2999: CMake build should use target-level co...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/486 @anmolnar So here's what failed on Jenkins, but it makes no sense because Jenkins doesn't even use CMake. It can't be related: ``` [exec] [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build/src/c/tests/TestReconfig.cc:577: Assertion: assertion failed [Expression: numClientsPerHost.at(i) <= upperboundClientsPerServer(numClients, numServers)] [exec] [exec] Failures !!! [exec] [exec] Run: 37 Failure total: 1 Failures: 1 Errors: 0 [exec] [exec] FAIL: zktest-st ``` ---
[GitHub] zookeeper issue #486: ZOOKEEPER-2999: CMake build should use target-level co...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/486 FWIW I was able to build and run tests using CMake just fine. That said, this was failing ``` 1: Zookeeper_simpleSystem::testAsyncWatcherAutoResetterminate called after throwing an instance of 'CppUnit::Exception' 1: what(): equality assertion failed 1: - Expected: -101 1: - Actual : -4 ``` But I don't know if it's a failure on `master` or configuration related or what. I can't see how the posted changes would affect one particular unit test like this. ---
[GitHub] zookeeper issue #486: ZOOKEEPER-2999: CMake build should use target-level co...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/486 @anmolnar Here goes... ---
[GitHub] zookeeper issue #499: ZOOKEEPER-3017: Link libm in CMake on FreeBSD.
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/499 LGTM. @hanm the build failure was spurious as this was a change to the CMake build. ---
[GitHub] zookeeper pull request #486: ZOOKEEPER-2999: CMake build should use target-l...
Github user andschwa commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/486#discussion_r176987184 --- Diff: src/c/CMakeLists.txt --- @@ -14,14 +14,13 @@ # See the License for the specific language governing permissions and # limitations under the License. -cmake_minimum_required(VERSION 3.6) +cmake_minimum_required(VERSION 3.5) --- End diff -- I guess this needs a rebase now. ---
[GitHub] zookeeper issue #485: ZOOKEEPER-2997: CMake should not force static CRT link...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/485 Thanks @hanm! ---
[GitHub] zookeeper issue #486: ZOOKEEPER-2999: CMake build should use target-level co...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/486 CI failures look spurious... ---
[GitHub] zookeeper issue #485: ZOOKEEPER-2997: CMake should not force static CRT link...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/485 CI errors look spurious... ---
[GitHub] zookeeper issue #485: ZOOKEEPER-2997: CMake should not force static CRT link...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/485 [JIRA: ZOOKEEPER-2997](https://issues.apache.org/jira/browse/ZOOKEEPER-2997) ---
[GitHub] zookeeper issue #484: ZOOKEEPER-2998: CMake declares incorrect ZooKeeper ver...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/484 [JIRA: ZOOKEEPER-2998](https://issues.apache.org/jira/browse/ZOOKEEPER-2998) ---
[GitHub] zookeeper issue #486: ZOOKEEPER-2999: CMake build should use target-level co...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/486 [JIRA: ZOOKEEPER-2999](https://issues.apache.org/jira/browse/ZOOKEEPER-2999) ---
[GitHub] zookeeper issue #486: ZOOKEEPER-2999: CMake build should use target-level co...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/486 @proller I cherry-picked and kept your authorship ð ---
[GitHub] zookeeper issue #484: ZOOKEEPER-2998: CMake declares incorrect ZooKeeper ver...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/484 That CI failure looks spurious... ---
[GitHub] zookeeper issue #486: ZOOKEEPER-2999: CMake build should use target-level co...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/486 @phunt This replaces #386. ---
[GitHub] zookeeper pull request #486: ZOOKEEPER-2999: CMake build should use target-l...
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/486 ZOOKEEPER-2999: CMake build should use target-level commands CMake is using `include_directories`, which has global side effects, instead of the more explicit `target_include_directories`, to include directories per target (and with private or public scoping). Furthermore, CMake should also use `CMAKE_CURRENT_SOURCE_DIR` over `CMAKE_SOURCE_DIR` in order to allow inclusion in other projects via `add_subdirectory()`, and we can reduce the minimally required CMake version to 3.5 from 3.6. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper ZOOKEEPER-2999 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/486.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 #486 commit 5e184a2b56561af012a09d395e687a0a2cf3cf42 Author: proller Date: 2017-09-28T14:40:38Z ZOOKEEPER-2999: CMake build should use target-level commands CMake is using `include_directories`, which has global side effects, instead of the more explicit `target_include_directories`, to include directories per target (and with private or public scoping). Furthermore, CMake should also use `CMAKE_CURRENT_SOURCE_DIR` over `CMAKE_SOURCE_DIR` in order to allow inclusion in other projects via `add_subdirectory()`, and we can reduce the minimally required CMake version to 3.5 from 3.6. ---
[GitHub] zookeeper pull request #386: Cmake fixes
Github user andschwa commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/386#discussion_r173571281 --- Diff: src/c/CMakeLists.txt --- @@ -14,14 +14,13 @@ # See the License for the specific language governing permissions and # limitations under the License. -cmake_minimum_required(VERSION 3.6) +cmake_minimum_required(VERSION 3.5) project(zookeeper VERSION 3.5.3) --- End diff -- @phunt If you could give the `andschwa` Confluence user edit permissions for the ZooKeeper wiki, I can add to that page how to update the CMake version when releasing. ---
[GitHub] zookeeper pull request #485: ZOOKEEPER-2997: CMake should not force static C...
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/485 ZOOKEEPER-2997: CMake should not force static CRT linking By removing this code, CMake will use its own defaults for the CRT flags (e.g., `/MDd` for debug configurations). With it removed, the user can override this behavior by setting the `CMAKE_CXX_FLAGS` manually when configuring ZooKeeper. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper ZOOKEEPER-2997 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/485.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 #485 commit 45a997ad0e281d967460f4eb205a4b18e4219ecd Author: Andrew Schwartzmeyer Date: 2018-03-09T21:00:48Z ZOOKEEPER-2997: CMake should not force static CRT linking By removing this code, CMake will use its own defaults for the CRT flags (e.g., `/MDd` for debug configurations). With it removed, the user can override this behavior by setting the `CMAKE_CXX_FLAGS` manually when configuring ZooKeeper. ---
[GitHub] zookeeper issue #485: ZOOKEEPER-2997: CMake should not force static CRT link...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/485 I had to reopen this because I changed which branch it was from. ---
[GitHub] zookeeper pull request #483: ZOOKEEPER-2997: CMake should not force static C...
Github user andschwa closed the pull request at: https://github.com/apache/zookeeper/pull/483 ---
[GitHub] zookeeper issue #386: Cmake fixes
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/386 @phunt Since this hasn't been updated, let me cherry-pick and open a new PR with the correct info. ---
[GitHub] zookeeper pull request #386: Cmake fixes
Github user andschwa commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/386#discussion_r173569107 --- Diff: src/c/CMakeLists.txt --- @@ -14,14 +14,13 @@ # See the License for the specific language governing permissions and # limitations under the License. -cmake_minimum_required(VERSION 3.6) +cmake_minimum_required(VERSION 3.5) project(zookeeper VERSION 3.5.3) --- End diff -- https://github.com/apache/zookeeper/pull/484 ---
[GitHub] zookeeper pull request #484: ZOOKEEPER-2998: CMake declares incorrect ZooKee...
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/484 ZOOKEEPER-2998: CMake declares incorrect ZooKeeper version This was not updated for the current development branch; it should be 3.6.0. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper ZOOKEEPER-2998 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/484.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 #484 commit 76d4f9708e9015438f5709cf19c866cd177314a5 Author: Andrew Schwartzmeyer Date: 2018-03-09T21:09:15Z ZOOKEEPER-2998: CMake declares incorrect ZooKeepeer version This was not updated for the current development branch; it should be 3.6.0. ---
[GitHub] zookeeper pull request #386: Cmake fixes
Github user andschwa commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/386#discussion_r173568056 --- Diff: src/c/CMakeLists.txt --- @@ -14,14 +14,13 @@ # See the License for the specific language governing permissions and # limitations under the License. -cmake_minimum_required(VERSION 3.6) +cmake_minimum_required(VERSION 3.5) project(zookeeper VERSION 3.5.3) --- End diff -- Sorry, forgot about this. I'll at least get a patch out that fixes master. ---
[GitHub] zookeeper pull request #483: ZOOKEEPER-2997: CMake should not force static C...
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/483 ZOOKEEPER-2997: CMake should not force static CRT linking By removing this code, CMake will use its own defaults for the CRT flags (e.g., `/MDd` for debug configurations). With it removed, the user can override this behavior by setting the `CMAKE_CXX_FLAGS` manually when configuring ZooKeeper. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/483.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 #483 commit 45a997ad0e281d967460f4eb205a4b18e4219ecd Author: Andrew Schwartzmeyer Date: 2018-03-09T21:00:48Z ZOOKEEPER-2997: CMake should not force static CRT linking By removing this code, CMake will use its own defaults for the CRT flags (e.g., `/MDd` for debug configurations). With it removed, the user can override this behavior by setting the `CMAKE_CXX_FLAGS` manually when configuring ZooKeeper. ---
[GitHub] zookeeper issue #386: Cmake fixes
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/386 If you've only tested with 3.5, then this should be set to 3.5 until it's tested with lower. CMake actually does add features in minor patches. It will _probably_ work, but if it doesn't, then someone's gonna complain eventually. ---
[GitHub] zookeeper issue #381: ZOOKEEPER-2905: Don't include `config.h` in `zookeeper...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/381 Thanks! ---
[GitHub] zookeeper pull request #382: ZOOKEEPER-2905: Don't include `config.h` in `zo...
Github user andschwa closed the pull request at: https://github.com/apache/zookeeper/pull/382 ---
[GitHub] zookeeper pull request #381: ZOOKEEPER-2905: Don't include `config.h` in `zo...
Github user andschwa closed the pull request at: https://github.com/apache/zookeeper/pull/381 ---
[GitHub] zookeeper issue #382: ZOOKEEPER-2905: Don't include `config.h` in `zookeeper...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/382 Thanks! ---
[GitHub] zookeeper issue #383: ZOOKEEPER-2905: Don't include `config.h` in `zookeeper...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/383 Thanks @phunt! ---
[GitHub] zookeeper issue #383: ZOOKEEPER-2905: Don't include `config.h` in `zookeeper...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/383 @hanm and finally this PR is for `master`. Let me reiterate: I'm sorry! Ha, seriously, I can't believe I broke practically the same thing I was fixing. You have to be _super_ careful about what gets included in public headers. ---
[GitHub] zookeeper pull request #383: ZOOKEEPER-2905: Don't include `config.h` in `zo...
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/383 ZOOKEEPER-2905: Don't include `config.h` in `zookeeper.h` In ZOOKEEPER-2841 I fixed the inclusion of project-specific porting code that were included in the public headers, which then broke upstream projects (in my case, Mesos). Unfortunately, I inadvertently created a very similar problem, and it wasn't evident until the build was coupled with another project with the same bug. More specifically, when including ZooKeeper in Mesos, and including Google's Glog in Mesos, both projects define the macros `VERSION`, `PACKAGE_VERSION`, and `PACKAGE_TARNAME`, and do so publicly. This is commonly defined in `config.h` by Autotools (and by CMake for ZooKeeper for compatibility), and is not a problem unless included publicly, such as in `zookeeper.h`, and by more than one project. When refactoring, I saw two includes in `zookeeper.h` that instead of being guarded by e.g. `#ifdef HAVE_SYS_SOCKET_H` were guarded by `#ifndef WIN32`. I erroneously added `#include "config.h"` and guarded the includes "properly" with a feature guard. However, configuration files such as `config.h` and `winconfig.h` etc. must never be included in publicly in `zookeeper.h`, for the reasons given above. This patch reverts the bug, and instead includes `config.h` in `zookeeper.c`, where it is not exposed to other projects. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper ZOOKEEPER-2905-master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/383.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 #383 commit b825ecd08724107f873c726692ea8af02173bb5d Author: Andrew Schwartzmeyer Date: 2017-08-30T23:15:28Z ZOOKEEPER-2905: Don't include `config.h` in `zookeeper.h` In ZOOKEEPER-2841 I fixed the inclusion of project-specific porting code that were included in the public headers, which then broke upstream projects (in my case, Mesos). Unfortunately, I inadvertently created a very similar problem, and it wasn't evident until the build was coupled with another project with the same bug. More specifically, when including ZooKeeper in Mesos, and including Google's Glog in Mesos, both projects define the macros `VERSION`, `PACKAGE_VERSION`, and `PACKAGE_TARNAME`, and do so publicly. This is commonly defined in `config.h` by Autotools (and by CMake for ZooKeeper for compatibility), and is not a problem unless included publicly, such as in `zookeeper.h`, and by more than one project. When refactoring, I saw two includes in `zookeeper.h` that instead of being guarded by e.g. `#ifdef HAVE_SYS_SOCKET_H` were guarded by `#ifndef WIN32`. I erroneously added `#include "config.h"` and guarded the includes "properly" with a feature guard. However, configuration files such as `config.h` and `winconfig.h` etc. must never be included in publicly in `zookeeper.h`, for the reasons given above. This patch reverts the bug, and instead includes `config.h` in `zookeeper.c`, where it is not exposed to other projects. ---
[GitHub] zookeeper issue #382: ZOOKEEPER-2905: Don't include `config.h` in `zookeeper...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/382 @hanm and this is is for `branch-3.5`. ---
[GitHub] zookeeper pull request #382: ZOOKEEPER-2905: Don't include `config.h` in `zo...
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/382 ZOOKEEPER-2905: Don't include `config.h` in `zookeeper.h` In ZOOKEEPER-2841 I fixed the inclusion of project-specific porting code that were included in the public headers, which then broke upstream projects (in my case, Mesos). Unfortunately, I inadvertently created a very similar problem, and it wasn't evident until the build was coupled with another project with the same bug. More specifically, when including ZooKeeper in Mesos, and including Google's Glog in Mesos, both projects define the macros `VERSION`, `PACKAGE_VERSION`, and `PACKAGE_TARNAME`, and do so publicly. This is commonly defined in `config.h` by Autotools (and by CMake for ZooKeeper for compatibility), and is not a problem unless included publicly, such as in `zookeeper.h`, and by more than one project. When refactoring, I saw two includes in `zookeeper.h` that instead of being guarded by e.g. `#ifdef HAVE_SYS_SOCKET_H` were guarded by `#ifndef WIN32`. I erroneously added `#include "config.h"` and guarded the includes "properly" with a feature guard. However, configuration files such as `config.h` and `winconfig.h` etc. must never be included in publicly in `zookeeper.h`, for the reasons given above. This patch reverts the bug, and instead includes `config.h` in `zookeeper.c`, where it is not exposed to other projects. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper ZOOKEEPER-2905-3.5 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/382.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 #382 commit daac5254a1a7472089f6950f60e4a4a1a3fa1f0e Author: Andrew Schwartzmeyer Date: 2017-08-30T23:15:28Z ZOOKEEPER-2905: Don't include `config.h` in `zookeeper.h` In ZOOKEEPER-2841 I fixed the inclusion of project-specific porting code that were included in the public headers, which then broke upstream projects (in my case, Mesos). Unfortunately, I inadvertently created a very similar problem, and it wasn't evident until the build was coupled with another project with the same bug. More specifically, when including ZooKeeper in Mesos, and including Google's Glog in Mesos, both projects define the macros `VERSION`, `PACKAGE_VERSION`, and `PACKAGE_TARNAME`, and do so publicly. This is commonly defined in `config.h` by Autotools (and by CMake for ZooKeeper for compatibility), and is not a problem unless included publicly, such as in `zookeeper.h`, and by more than one project. When refactoring, I saw two includes in `zookeeper.h` that instead of being guarded by e.g. `#ifdef HAVE_SYS_SOCKET_H` were guarded by `#ifndef WIN32`. I erroneously added `#include "config.h"` and guarded the includes "properly" with a feature guard. However, configuration files such as `config.h` and `winconfig.h` etc. must never be included in publicly in `zookeeper.h`, for the reasons given above. This patch reverts the bug, and instead includes `config.h` in `zookeeper.c`, where it is not exposed to other projects. ---
[GitHub] zookeeper issue #381: ZOOKEEPER-2905: Don't include `config.h` in `zookeeper...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/381 Hey @hanm, I found and fixed a bug I made. This patches `branch-3.4`. It should be very straightforward. This is for [ZOOKEEPER-2905](https://issues.apache.org/jira/browse/ZOOKEEPER-2905). ---
[GitHub] zookeeper pull request #381: ZOOKEEPER-2905: Don't include `config.h` in `zo...
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/381 ZOOKEEPER-2905: Don't include `config.h` in `zookeeper.h` In ZOOKEEPER-2841 I fixed the inclusion of project-specific porting code that were included in the public headers, which then broke upstream projects (in my case, Mesos). Unfortunately, I inadvertently created a very similar problem, and it wasn't evident until the build was coupled with another project with the same bug. More specifically, when including ZooKeeper in Mesos, and including Google's Glog in Mesos, both projects define the macros `VERSION`, `PACKAGE_VERSION`, and `PACKAGE_TARNAME`, and do so publicly. This is commonly defined in `config.h` by Autotools (and by CMake for ZooKeeper for compatibility), and is not a problem unless included publicly, such as in `zookeeper.h`, and by more than one project. When refactoring, I saw two includes in `zookeeper.h` that instead of being guarded by e.g. `#ifdef HAVE_SYS_SOCKET_H` were guarded by `#ifndef WIN32`. I erroneously added `#include "config.h"` and guarded the includes "properly" with a feature guard. However, configuration files such as `config.h` and `winconfig.h` etc. must never be included in publicly in `zookeeper.h`, for the reasons given above. This patch reverts the bug, and instead includes `config.h` in `zookeeper.c`, where it is not exposed to other projects. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper ZOOKEEPER-2905 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/381.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 #381 commit 2ec2bc6680d49f3b6917db3f58257a80b4b4144f Author: Andrew Schwartzmeyer Date: 2017-08-30T23:15:28Z ZOOKEEPER-2905: Don't include `config.h` in `zookeeper.h` In ZOOKEEPER-2841 I fixed the inclusion of project-specific porting code that were included in the public headers, which then broke upstream projects (in my case, Mesos). Unfortunately, I inadvertently created a very similar problem, and it wasn't evident until the build was coupled with another project with the same bug. More specifically, when including ZooKeeper in Mesos, and including Google's Glog in Mesos, both projects define the macros `VERSION`, `PACKAGE_VERSION`, and `PACKAGE_TARNAME`, and do so publicly. This is commonly defined in `config.h` by Autotools (and by CMake for ZooKeeper for compatibility), and is not a problem unless included publicly, such as in `zookeeper.h`, and by more than one project. When refactoring, I saw two includes in `zookeeper.h` that instead of being guarded by e.g. `#ifdef HAVE_SYS_SOCKET_H` were guarded by `#ifndef WIN32`. I erroneously added `#include "config.h"` and guarded the includes "properly" with a feature guard. However, configuration files such as `config.h` and `winconfig.h` etc. must never be included in publicly in `zookeeper.h`, for the reasons given above. This patch reverts the bug, and instead includes `config.h` in `zookeeper.c`, where it is not exposed to other projects. ---
[GitHub] zookeeper issue #335: ZOOKEEPER-2874: Windows Debug builds don't link with `...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/335 Thank you @hanm! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #335: ZOOKEEPER-2874: Windows Debug builds don't link with `...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/335 Built on Windows: ```powershell > cmake --build . --config Debug > cmake --build . --config Release > rg libcmt cli.dir\Debug\cli.tlog\link.read.1.tlog 17:C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2017\COMMUNITY\VC\TOOLS\MSVC\14.10.25017\LIB\X86\LIBCMTD.LIB cli.dir\Release\cli.tlog\link.read.1.tlog 17:C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2017\COMMUNITY\VC\TOOLS\MSVC\14.10.25017\LIB\X86\LIBCMT.LIB ``` As you can see from the logs, the correct library is now being linked. I've also integration tested this with Mesos, and it has eliminated the warning. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #335: ZOOKEEPER-2874: Windows Debug builds don't link with `...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/335 @hanm This should be backported to 3.5 and 3.4 as well; it should be a trivial cherry-pick, but let me know if you'd like two more PRs. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #335: ZOOKEEPER-2874: Windows Debug builds don't link...
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/335 ZOOKEEPER-2874: Windows Debug builds don't link with `/MTd` When building in Debug configuration, this logic ensures that `/MTd` is used instead of just `/MT`, which on Windows means to link to the multi-threaded (debug) version of the standard library. When the user does not add `/MT` as a compile option manually, CMake would otherwise link to the correct one. Because we are overriding it for threaded compilations, we also must ensure that Debug configurations are specially handled. Furthermore, this must be done using a generator expression over configuration time logic because the Visual Studio CMake generators are "multi-configuration generators", that is, the configuration is chosen at build time instead of compile time. The generator expression handles this scenario, but checking `CMAKE_BUILD_CONFIG` would not. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper ZOOKEEPER-2874 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/335.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 #335 commit 554e20e1f63c02e058f05010868c3f6b5a4b1f2c Author: Andrew Schwartzmeyer Date: 2017-08-11T22:03:28Z ZOOKEEPER-2874: Windows Debug builds don't link with `/MTd` When building in Debug configuration, this logic ensures that `/MTd` is used instead of just `/MT`, which on Windows means to link to the multi-threaded (debug) version of the standard library. When the user does not add `/MT` as a compile option manually, CMake would otherwise link to the correct one. Because we are overriding it for threaded compilations, we also must ensure that Debug configurations are specially handled. Furthermore, this must be done using a generator expression over configuration time logic because the Visual Studio CMake generators are "multi-configuration generators", that is, the configuration is chosen at build time instead of compile time. The generator expression handles this scenario, but checking `CMAKE_BUILD_CONFIG` would not. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #313: ZOOKEEPER-2841: ZooKeeper public include files ...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #319: ZOOKEEPER-2859: Fix CMake build on OS X.
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/319 Also, I'd like to test `Autotools` on OS X, because I believe it has the same problem (unconditionally adds `libm` and `librt`), but I can't get past `autoconf -if` since I don't have `cppunit.m4`. I'm looking high and low for it: it's not in `/usr/local/` nor `/opt`, the brew CppUnit didn't come with it, and none of the Autotools directories has it. I found [ZOOKEEPER-1454](https://issues.apache.org/jira/browse/ZOOKEEPER-1454), and it showed me how to specify where `cppunit.m4` is, but that's not much use if I don't have it. Do you know, does anyone have this building on OS X? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #319: ZOOKEEPER-2859: Fix CMake build on OS X.
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/319 @hanm That's bizzare. The [log](https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/906/artifact/patchprocess/patchJavadocWarnings.txt/*view*/) it provides says this: ``` build@2/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java:153: warning - Tag @link:illegal character: "58" in "https://issues.apache.org/jira/browse/ZOOKEEPER-1355"; [javadoc] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build@2/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java:153: warning - Tag @link:illegal character: "47" in "https://issues.apache.org/jira/browse/ZOOKEEPER-1355"; [javadoc] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build@2/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java:153: warning - Tag @link:illegal character: "47" in "https://issues.apache.org/jira/browse/ZOOKEEPER-1355"; [javadoc] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build@2/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java:153: warning - Tag @link:illegal character: "47" in "https://issues.apache.org/jira/browse/ZOOKEEPER-1355"; [javadoc] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build@2/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java:153: warning - Tag @link:illegal character: "47" in "https://issues.apache.org/jira/browse/ZOOKEEPER-1355"; [javadoc] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build@2/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java:153: warning - Tag @link:illegal character: "47" in "https://issues.apache.org/jira/browse/ZOOKEEPER-1355"; [javadoc] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build@2/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java:153: warning - Tag @link:illegal character: "45" in "https://issues.apache.org/jira/browse/ZOOKEEPER-1355"; [javadoc] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build@2/src/java/main/org/apache/zookeeper/client/StaticHostProvider.java:153: warning - Tag @link: reference not found: https://issues.apache.org/jira/browse/ZOOKEEPER-1355 ``` And that issue it's referencing is [ancient](https://issues.apache.org/jira/browse/ZOOKEEPER-1355). That really is weird. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #319: ZOOKEEPER-2859: Fix CMake build on OS X.
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/319 @hanm This has a few fixes for the CMake build for OS X. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #311: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/311 Thank you! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #311: ZOOKEEPER-2841: ZooKeeper public include files ...
Github user andschwa closed the pull request at: https://github.com/apache/zookeeper/pull/311 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #319: ZOOKEEPER-2859: Fix CMake build on OS X.
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/319 ZOOKEEPER-2859: Fix CMake build on OS X. And take care of a TODO for pthread. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper macos Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/319.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 #319 commit 050ece633b3a9eec70db139a7e95bc70015cb6ab Author: Andrew Schwartzmeyer Date: 2017-07-26T22:01:40Z ZOOKEEPER-2859: Fix CMake build on OS X. And take care of a TODO for pthread. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #311: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/311 `branch-3.5` has ZOOKEEPER-1643, so this patch should just be good to go. It was integration tested. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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=0x15d5c44fa61 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.LeaderDetectorCancellationHan
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #313: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[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 ``. 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 `, 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 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
[GitHub] zookeeper issue #311: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/311 @hanm oh.. good to know! Okay I'll give it another whack, thanks :) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #311: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/311 Hm... it might not backport so easily to `branch-3.4`. Let me know how badly it's wanted. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #311: ZOOKEEPER-2841: ZooKeeper public include files leak po...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/311 @hanm this backports #306 to `branch-3.5`. I rebuilt and retested, all looks good. Since original branch came from `master` the version was already what I believe to be correct at `3.5.3`. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #311: ZOOKEEPER-2841: ZooKeeper public include files ...
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/311 ZOOKEEPER-2841: ZooKeeper public include files leak porting change This patch primarily adds a cross-platform CMake build system. This is in addition to the Autotools system on Linux, until it can be deprecated, and this replaces the existing committed Visual Studio Solutions for Windows. As this commit deprecates (and breaks) the previously existing Visual Studio solutions and projects, they've been removed. Building on Windows now requires CMake, but this enables the support of any code-supported Visual Studio version. Support for Visual Studio 2017 was explicitly added by this patch, and support for future versions, barring software bugs, should be available automatically through CMake. The notable features lacking in comparison to Autotools are Solaris support, shared library builds, whitelisted symbol exports, libtool integration, and doxygen document generation. Almost everything else from Autotools has been ported, including header/function/library checks, and all targets (zookeeper, hashtable, cli, load_gen, and tests). The primary work involved for CMake (other than the writing of `CMakeLists.txt`) was transitioning the hand-written `winconfig.h` to an auto-generated `config.h` file, and guarding code with `#ifdef HAVE_FEATURE`. The `cmake_config.h.in` template was modeled after the Autotools config file so that the feature guards share the same names. This patch also refactors the Windows port of the C client. Notably, it moves as much porting code as possible out of the publicly included `winconfig.h` header and into the relevant source files, or the private `winport.h` header. While `load_gen.c` looks at first glance as if it were ported to Windows, it never actually was, so the erroneous `#include "win32port.h"` was removed, and the target is not built on Windows. The `include/winstdint.h` header was removed as it has been replaced by ``. 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 `, as it "pulls in the world" and should not be included publicly. A guard was placed around `#define snprintf` in order to enable compiling with modern versions of MSVC, including Visual Studio 2015. A problem with `DLL_EXPORT` and `USE_STATIC_LIB` being redefined was fixed. There are existent warnings which this patch did not attempt to fix. Building tests on Windows is not supported, but was not previously supported. The tests need to be ported. Future work should resolve the `ACL` / `ZKACL` conflict, and potentially remove `include/winconfig.h` altogether. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper cmake-backport-3.5 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/311.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #311 commit 081833e9899afd4937075038f78bb4faff446ef4 Author: Andrew Schwartzmeyer 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
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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(0x7f01d700):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(0x7f01d700):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(0x7f01d700):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(0x7f01d700):ZOO_ERROR@handle_socket_error_msg@2428: Socket [127.0.0.1:38085] zk retcode=-112, errno=116(Stale file handle): sessionId=0x1305fd8 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=0x1305fd80001 has expired. 2017-07-14 15:23:18,676:29970(0x7f01d700):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(0x7f01d700):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@24
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #306: ZOOKEEPER-2841: ZooKeeper public include files ...
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 header file. */ -#undef HAVE_ARPA_INET_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_DLFCN_H - -/* Define to 1 if you have the 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 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 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 header file. */ -#undef HAVE_NETDB_H - -/* Define to 1 if you have the 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 header file. */ -#undef HAVE_STDINT_H - -/* Define to 1 if you have the 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 header file. */ -#undef HAVE_STRINGS_H - -/* Define to 1 if you have the 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 header file. */ -#undef HAVE_SYS_SOCKET_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_STAT_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_TIME_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_TYPES_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_UTSNAME_H - -/* Define to 1 if you have the 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 "u...@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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #306: ZOOKEEPER-2841: ZooKeeper public include files ...
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 header file. */ -#undef HAVE_ARPA_INET_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_DLFCN_H - -/* Define to 1 if you have the 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 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 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 header file. */ -#undef HAVE_NETDB_H - -/* Define to 1 if you have the 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 header file. */ -#undef HAVE_STDINT_H - -/* Define to 1 if you have the 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 header file. */ -#undef HAVE_STRINGS_H - -/* Define to 1 if you have the 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 header file. */ -#undef HAVE_SYS_SOCKET_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_STAT_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_TIME_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_TYPES_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_UTSNAME_H - -/* Define to 1 if you have the 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 "u...@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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #306: ZOOKEEPER-2841: ZooKeeper public include files ...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #306: ZOOKEEPER-2841: ZooKeeper public include files ...
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 header file. */ -#undef HAVE_ARPA_INET_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_DLFCN_H - -/* Define to 1 if you have the 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 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 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 header file. */ -#undef HAVE_NETDB_H - -/* Define to 1 if you have the 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 header file. */ -#undef HAVE_STDINT_H - -/* Define to 1 if you have the 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 header file. */ -#undef HAVE_STRINGS_H - -/* Define to 1 if you have the 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 header file. */ -#undef HAVE_SYS_SOCKET_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_STAT_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_TIME_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_TYPES_H - -/* Define to 1 if you have the header file. */ -#undef HAVE_SYS_UTSNAME_H - -/* Define to 1 if you have the 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 "u...@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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #255: ZOOKEEPER-2756: Add CMake build system for bett...
Github user andschwa closed the pull request at: https://github.com/apache/zookeeper/pull/255 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #255: ZOOKEEPER-2756: Add CMake build system for better cros...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/255 You got 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #306: ZOOKEEPER-2841: ZooKeeper public include files leak po...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[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 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 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 ``. It fixes problems for upstream libraries by removing `#undef AF_INET6` and `#include `. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #255: ZOOKEEPER-2756: Add CMake build system for better cros...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/255 This patch also resolves [ZOOKEEPER-2491](https://issues.apache.org/jira/browse/ZOOKEEPER-2491), as I was forced to include this change in order to build on with Visual Studio 2017 on Windows 10. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #255: ZOOKEEPER-2756: Add CMake build system for better cros...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/255 (Oh just FYI on Windows, `-DWANT_CPPUNIT=OFF` should probably be the default. The tests don't seem to be ported to Windows.) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #255: ZOOKEEPER-2756: Add CMake build system for better cros...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/255 @hanm any update on this? The conversation got a bit split between GitHub and [JIRA](https://issues.apache.org/jira/browse/ZOOKEEPER-2756). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #255: ZOOKEEPER-2756: Add CMake build system for better cros...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/255 Jenkins is saying there were two new "release audit warnings" but the [file](https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/750//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt) it points me to does not exist. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #255: ZOOKEEPER-2756: Add CMake build system for better cros...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/255 @hanm I was proposing we replace the existing sln/vcproj/vcxproj entirely, as they can be built for any version of VS using CMake. I could record their deletion in this PR even. We _could_ keep them and retain compatibility with them, but it'd be ugly, as they rely on the manually generated portion of `winconfig.h` which I replaced with the auto-generated `config.h`. What do you think? Also, I'm not super comfortable including the changes in 065056b0b240b7bdff2eebe41db86a9a3ea6ecfc , your thoughts? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #255: ZOOKEEPER-2756: Add CMake build system for better cros...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/255 @hanm I actually split this into the few commits I would _normally_ have posted in my [cmake-pr](https://github.com/andschwa/zookeeper/commits/cmake-pr) branch, and then went back to a single commit after re-reading the contributing guidelines. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #255: ZOOKEEPER-2756: Add CMake build system for bett...
GitHub user andschwa opened a pull request: https://github.com/apache/zookeeper/pull/255 ZOOKEEPER-2756: Add CMake build system for better cross-platform support 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, save a few easy ones (set but unused `rc` variable). Fix DLL_EXPORT and USE_STATIC_LIB redefinition. Some changes to `winconfig.h` necessary to build with Visual Studio 2015 (and 2017) were included; these originally came from a patch embedded inside the Mesos build process. You can merge this pull request into a Git repository by running: $ git pull https://github.com/andschwa/zookeeper ZOOKEEPER-2756 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/255.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 #255 commit 932ca61b3f7d3112b5368872a4bfec7523484ee2 Author: Andrew Schwartzmeyer 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, save a few easy ones (set but unused `rc` variable). Fix DLL_EXPORT and USE_STATIC_LIB redefinition. Some changes to `winconfig.h` necessary to build with Visual Studio 2015 (and 2017) were included; these originally came from a patch embedded inside the Mesos build process. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #255: ZOOKEEPER-2756: Add CMake build system for better cros...
Github user andschwa commented on the issue: https://github.com/apache/zookeeper/pull/255 Tests can be run with the correct environment via `ctest` (`make test` won't have the right environment without manual setup). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
