zookeeper-dev mailing list archives

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

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

Hadoop QA commented on ZOOKEEPER-2905:

-1 overall.  GitHub Pull Request  Build

    +1 @author.  The patch does not contain any @author tags.

    +0 tests included.  The patch appears to be a documentation patch that doesn't require

    +1 javadoc.  The javadoc tool did not generate any warning messages.

    +1 javac.  The applied patch does not increase the total number of javac compiler warnings.

    +1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) warnings.

    +1 release audit.  The applied patch does not increase the total number of release audit

    -1 core tests.  The patch failed core unit tests.

    +1 contrib tests.  The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1043//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1043//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1043//console

This message is automatically generated.

> 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
> 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"
> -
> +/* 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

View raw message