You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by andschwa <gi...@git.apache.org> on 2017/09/25 23:01:51 UTC
[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 <an...@schwartzmeyer.com>
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 #383: ZOOKEEPER-2905: Don't include `config.h` in `zookeeper...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/383
+1, lgtm. I compiled the c client code under Ubuntu 17.04, also compiled and successfully ran the tests for zkpython on the same environment.
---
[GitHub] zookeeper pull request #383: ZOOKEEPER-2905: Don't include `config.h` in `zo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/383
---
[GitHub] zookeeper issue #383: ZOOKEEPER-2905: Don't include `config.h` in `zookeeper...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/383
Thanks @andschwa - no worries. :-)
---
[GitHub] zookeeper issue #383: ZOOKEEPER-2905: Don't include `config.h` in `zookeeper...
Posted by andschwa <gi...@git.apache.org>.
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 issue #383: ZOOKEEPER-2905: Don't include `config.h` in `zookeeper...
Posted by andschwa <gi...@git.apache.org>.
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/383
Thanks @phunt!
---