ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-636

configure.ac has instructions which override the contents of CFLAGS and CXXFLAGS.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2.1
    • Fix Version/s: 3.4.0
    • Component/s: build, c client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Tags:
      autoconf autotools configure

      Description

      The information mustn't be overridden.
      The template like «CFLAGS="$CFLAGS -some-option"» should be used.

      1. ZOOKEEPER-636.patch
        2 kB
        Maxim P. Dementiev

        Activity

        Hide
        Patrick Hunt added a comment -

        could you provide a bit more detail of the issue. what's the problem? what issue are you seeing, can you give an example where it's a problem?

        I tried the following with the current code "make CFLAGS=-XXXXX" and -XXXXX was used in compiling the source. Isn't this what we want?

        Show
        Patrick Hunt added a comment - could you provide a bit more detail of the issue. what's the problem? what issue are you seeing, can you give an example where it's a problem? I tried the following with the current code "make CFLAGS=-XXXXX" and -XXXXX was used in compiling the source. Isn't this what we want?
        Hide
        Maxim P. Dementiev added a comment -

        Dear Patrick,

        You've mentioned only one possible usage.
        Please, try to think wider.

        ZooKeeper C client library uses autoconf framework (http://www.gnu.org/software/autoconf/autoconf.html).
        Try to run "./configure --help" on any version of C client library, it says:
        "Some influential environment variables: CFLAGS - C compiler flags"
        It means that the way like this:
        ------------- beginning -------------------
        max@localhost ~/sources/zookeeper-3.2.2/src/c $ export CFLAGS="-march=k8 -Os -pipe -fomit-frame-pointer"
        max@localhost ~/sources/zookeeper-3.2.2/src/c $ ./configure
        checking for doxygen... /usr/bin/doxygen
        ------------- end -------------------
        This way is perfectly legal, intentional and must influence the result of configure script.

        Of course, nothing can prevent a developer to do the trick like this in a configure script:
        ------------- beginning -------------------
        CFLAGS="-g -O0";CXXFLAGS="-g -O0"
        ------------- end -------------------
        This is quote from zookeeper-3.2.2/src/c/configure.ac, line 57.
        The end-user of "configure" command doesn't expect this behaviour, it violates that "configure --help" states.

        More, there is no open source project in widespread usage which does this overriding in its configure scripts.

        The value of CFLAGS environment variable is usually used in configure script itself, if it is present, to check some functionality.
        For example, "gcc -march=XXX" can turn on atomic intrinsics (http://gcc.gnu.org/onlinedocs/gcc-4.4.1/gcc/Atomic-Builtins.html), "g++ -std=std+0x" can turn on moving methods in STL implementation, and so on.

        Show
        Maxim P. Dementiev added a comment - Dear Patrick, You've mentioned only one possible usage. Please, try to think wider. ZooKeeper C client library uses autoconf framework ( http://www.gnu.org/software/autoconf/autoconf.html ). Try to run "./configure --help" on any version of C client library, it says: "Some influential environment variables: CFLAGS - C compiler flags" It means that the way like this: ------------- beginning ------------------- max@localhost ~/sources/zookeeper-3.2.2/src/c $ export CFLAGS="-march=k8 -Os -pipe -fomit-frame-pointer" max@localhost ~/sources/zookeeper-3.2.2/src/c $ ./configure checking for doxygen... /usr/bin/doxygen ------------- end ------------------- This way is perfectly legal, intentional and must influence the result of configure script. Of course, nothing can prevent a developer to do the trick like this in a configure script: ------------- beginning ------------------- CFLAGS="-g -O0";CXXFLAGS="-g -O0" ------------- end ------------------- This is quote from zookeeper-3.2.2/src/c/configure.ac, line 57. The end-user of "configure" command doesn't expect this behaviour, it violates that "configure --help" states. More, there is no open source project in widespread usage which does this overriding in its configure scripts. The value of CFLAGS environment variable is usually used in configure script itself, if it is present, to check some functionality. For example, "gcc -march=XXX" can turn on atomic intrinsics ( http://gcc.gnu.org/onlinedocs/gcc-4.4.1/gcc/Atomic-Builtins.html ), "g++ -std=std+0x" can turn on moving methods in STL implementation, and so on.
        Hide
        Patrick Hunt added a comment -

        @maxim, thanks, that's what I was looking for. Not being an expert at autotools myself it's hard to know what correct behavior is.

        Do you want to take a shot at this?

        Also, if you had any other ideas how to make our auto* integration better we'd love to have you submit patches. In particular
        there's a long standing issue that we require cppunit, if you know how to address this we'd love to have a patch (I assume
        this would be an option to configure to turn off cppunit being required, obv this would make it impossible to run the tests
        but at least one could compile). We've tried fixing this issue previously but no luck.

        Please continue making suggestions, patches would even be better!

        Show
        Patrick Hunt added a comment - @maxim, thanks, that's what I was looking for. Not being an expert at autotools myself it's hard to know what correct behavior is. Do you want to take a shot at this? Also, if you had any other ideas how to make our auto* integration better we'd love to have you submit patches. In particular there's a long standing issue that we require cppunit, if you know how to address this we'd love to have a patch (I assume this would be an option to configure to turn off cppunit being required, obv this would make it impossible to run the tests but at least one could compile). We've tried fixing this issue previously but no luck. Please continue making suggestions, patches would even be better!
        Hide
        Maxim P. Dementiev added a comment -

        I don't think I'm an autotools expert, but many things have changed since ZOOKEEPER-281.
        I even posted an article about Autoconf and CppUnit integration in my blog: http://mpd-eng.livejournal.com/700.html
        I'll do my best to fix this one issue (CFLAGS overriding).
        And please, fill free to assign to me any autoconf issue (is there corresponding opened issue with cppunit that you've talked about?).

        Show
        Maxim P. Dementiev added a comment - I don't think I'm an autotools expert, but many things have changed since ZOOKEEPER-281 . I even posted an article about Autoconf and CppUnit integration in my blog: http://mpd-eng.livejournal.com/700.html I'll do my best to fix this one issue (CFLAGS overriding). And please, fill free to assign to me any autoconf issue (is there corresponding opened issue with cppunit that you've talked about?).
        Hide
        Patrick Hunt added a comment -

        That would be great. I think the closest thing we have is "build" component. Feel free to work on open JIRAs, it's fine if you want to assign it to yourself (you don't
        need to wait for me). Regards.

        Show
        Patrick Hunt added a comment - That would be great. I think the closest thing we have is "build" component. Feel free to work on open JIRAs, it's fine if you want to assign it to yourself (you don't need to wait for me). Regards.
        Hide
        Maxim P. Dementiev added a comment -

        This patch modifies src/c/configure.ac and src/c/Makefile.am files in order to not override user values of CFLAGS and CXXFLAGS environment variables like it is expected and defined by design.
        Some C and C++ options are still in Makefile.am.

        Show
        Maxim P. Dementiev added a comment - This patch modifies src/c/configure.ac and src/c/Makefile.am files in order to not override user values of CFLAGS and CXXFLAGS environment variables like it is expected and defined by design. Some C and C++ options are still in Makefile.am.
        Hide
        Maxim P. Dementiev added a comment -

        Sorry for delay. Our project is in trouble (http://mpd-eng.livejournal.com/1159.html).

        I've changed src/c/Makefile.am: CXXFLAGS should not be overridden.
        Anyway, I kept some additional options there ("AM_CFLAGS = -Wall -Werror" and "AM_CXXFLAGS = -Wall"). I think it must be moved in configure.ac as well.

        It is possible to parse the contents of CFLAGS and CXXFLAGS variable and to add specific options conditionally (like it is done for example in http://git.gnome.org/browse/gtk+/tree/configure.in?h=gtk-2-18#n320), but we need to define requirements for the platform and tool versions (can we use such a technique or not).

        Show
        Maxim P. Dementiev added a comment - Sorry for delay. Our project is in trouble ( http://mpd-eng.livejournal.com/1159.html ). I've changed src/c/Makefile.am: CXXFLAGS should not be overridden. Anyway, I kept some additional options there ("AM_CFLAGS = -Wall -Werror" and "AM_CXXFLAGS = -Wall"). I think it must be moved in configure.ac as well. It is possible to parse the contents of CFLAGS and CXXFLAGS variable and to add specific options conditionally (like it is done for example in http://git.gnome.org/browse/gtk+/tree/configure.in?h=gtk-2-18#n320 ), but we need to define requirements for the platform and tool versions (can we use such a technique or not).
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12444711/ZOOKEEPER-636.patch
        against trunk revision 944515.

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

        +1 tests included. The patch appears to include 1 new or modified tests.

        +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 warnings.

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

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/92/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/92/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/92/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12444711/ZOOKEEPER-636.patch against trunk revision 944515. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified tests. +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 warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/92/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/92/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/92/console This message is automatically generated.
        Hide
        Maxim P. Dementiev added a comment -

        And I wonder what does this "-1 core tests. The patch failed core unit tests." mean for me?
        Should I correct something?

        Show
        Maxim P. Dementiev added a comment - And I wonder what does this "-1 core tests. The patch failed core unit tests." mean for me? Should I correct something?
        Hide
        Mahadev konar added a comment -

        maxim, looks like the c tests failed. You can take a look at what failed in http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/92/testReport. This does not look like some problem in the patch. To verify, you can cancel and resubmit to see if the test fails again or not.

        Show
        Mahadev konar added a comment - maxim, looks like the c tests failed. You can take a look at what failed in http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/92/testReport . This does not look like some problem in the patch. To verify, you can cancel and resubmit to see if the test fails again or not.
        Hide
        Patrick Hunt added a comment -

        Maxim, this is a known issue:

        [exec] [exec] Zookeeper_simpleSystem::testAuth ZooKeeper server started : elapsed 25448 : OK
        [exec] [exec] zktest-mt: /grid/0/hudson/hudson-slave/workspace/Zookeeper-Patch-h1.grid.sp2.yahoo.net/trunk/src/c/src/zookeeper.c:1950: zookeeper_process: Assertion `cptr' failed.
        [exec] [exec] Zookeeper_simpleSystem::testHangingClient

        try retriggering the build. (cancel/submit)

        Show
        Patrick Hunt added a comment - Maxim, this is a known issue: [exec] [exec] Zookeeper_simpleSystem::testAuth ZooKeeper server started : elapsed 25448 : OK [exec] [exec] zktest-mt: /grid/0/hudson/hudson-slave/workspace/Zookeeper-Patch-h1.grid.sp2.yahoo.net/trunk/src/c/src/zookeeper.c:1950: zookeeper_process: Assertion `cptr' failed. [exec] [exec] Zookeeper_simpleSystem::testHangingClient try retriggering the build. (cancel/submit)
        Hide
        Maxim P. Dementiev added a comment -

        Resubmitting after the fail which isn't related to the patch.

        Show
        Maxim P. Dementiev added a comment - Resubmitting after the fail which isn't related to the patch.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12444711/ZOOKEEPER-636.patch
        against trunk revision 944515.

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

        +1 tests included. The patch appears to include 1 new or modified tests.

        +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 warnings.

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

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/96/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/96/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/96/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12444711/ZOOKEEPER-636.patch against trunk revision 944515. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified tests. +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 warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/96/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/96/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/96/console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        +1, thanks Maxim!

        Show
        Patrick Hunt added a comment - +1, thanks Maxim!
        Hide
        Maxim P. Dementiev added a comment -

        Thank you, Patrick!
        I do hope our team (http://mpd-eng.livejournal.com/1159.html) will find an opportunity to continue work on the project and I will do more for ZooKeeper!

        Show
        Maxim P. Dementiev added a comment - Thank you, Patrick! I do hope our team ( http://mpd-eng.livejournal.com/1159.html ) will find an opportunity to continue work on the project and I will do more for ZooKeeper!
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #831 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/831/)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #831 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/831/ )

          People

          • Assignee:
            Maxim P. Dementiev
            Reporter:
            Maxim P. Dementiev
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development