zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ZOOKEEPER-2905) Don't include `config.h` in `zookeeper.h`
Date Mon, 25 Sep 2017 23:02:00 GMT

    [ https://issues.apache.org/jira/browse/ZOOKEEPER-2905?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16179921#comment-16179921
] 

ASF GitHub Bot commented on ZOOKEEPER-2905:
-------------------------------------------

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 <andrew@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.

----


> Don't include `config.h` in `zookeeper.h`
> -----------------------------------------
>
>                 Key: ZOOKEEPER-2905
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2905
>             Project: ZooKeeper
>          Issue Type: Bug
>         Environment: Linux-ish environments.
>            Reporter: Andrew Schwartzmeyer
>            Assignee: Andrew Schwartzmeyer
>
> In ZOOKEEPER-2841 I fixed the inclusion of project-specific porting changes that were
included in the public headers, which then broke upstream projects (in my case, Mesos).
> Unfortunately, I inadvertently created the exact same problem for Linux (or really any
system that uses Autotools), and it wasn't evident until the build was coupled with another
project with the same problem. More specifically, when including ZooKeeper (with my changes)
in Mesos, and including Google's Glog in Mesos, and building both with Autotools (which we
also support), both packages define the pre-processor macro {{PACKAGE_VERSION}}, and so so
publicly. This is defined in {{config.h}} by Autotools, and is not a problem _unless included
publicly_.
> 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}}. Without realizing
that I would create the exact same problem I was elsewhere fixing, I erroneously added {{#include
"config.h"}} and guarded the includes "properly." But there is _very good reasons_ not to
do this (explained above).
> The patch to fix this is simple:
> {noformat}
> diff --git a/src/c/include/zookeeper.h b/src/c/include/zookeeper.h
> index d20e70af4..b0bb09e3f 100644
> --- a/src/c/include/zookeeper.h
> +++ b/src/c/include/zookeeper.h
> @@ -21,13 +21,9 @@
>  #include <stdlib.h>
> -#include "config.h"
> -
> -#ifdef HAVE_SYS_SOCKET_H
> +/* we must not include config.h as a public header */
> +#ifndef WIN32
>  #include <sys/socket.h>
> -#endif
> -
> -#ifdef HAVE_SYS_TIME_H
>  #include <sys/time.h>
>  #endif
> diff --git a/src/c/src/zookeeper.c b/src/c/src/zookeeper.c
> index 220c57dc4..9b837f227 100644
> --- a/src/c/src/zookeeper.c
> +++ b/src/c/src/zookeeper.c
> @@ -24,6 +24,7 @@
>  #define USE_IPV6
>  #endif
> +#include "config.h"
>  #include <zookeeper.h>
>  #include <zookeeper.jute.h>
>  #include <proto.h>
> {noformat}
> I am opening pull requests in a few minutes to have this applied to branch 3.4 and 3.5.
> I'm sorry!



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Mime
View raw message