Qpid
  1. Qpid
  2. QPID-3464

Build improvements (was fix build on Debian squeeze)

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.12
    • Fix Version/s: 0.14, 0.15
    • Component/s: Build Tools
    • Labels:
      None
    • Environment:

      Debian Squeeze

      Description

      General build improvements (mostly for the cmake build) which were made when working with Debian Squeeze.

      From OP:
      I'm using serveral patches to build QPID on Debian Squeeze.

      1. 01_build-libqmfengine-before-libqmf.diff
        0.4 kB
        Jan-Marek Glogowski
      2. 02_configure.ac-fix-polling-detection.diff
        2 kB
        Jan-Marek Glogowski
      3. 03_install-python-libs-into-PYTHON_LIB.diff
        1 kB
        Jan-Marek Glogowski
      4. 04_reorganize-automake-qpid-perl-bindings-build.diff
        3 kB
        Jan-Marek Glogowski
      5. 05_cleanup-cmake-build.diff
        35 kB
        Jan-Marek Glogowski
      6. 06_fix-ignored-return-value-warnings.diff
        3 kB
        Jan-Marek Glogowski
      7. 07_fixes_based_on_jira_comments.diff
        5 kB
        Jan-Marek Glogowski

        Activity

        Hide
        Andrew Stitcher added a comment -

        I note that the diffs seem to be against 0.10 so there is some chance they don't apply cleanly against trunk.

        [BTW it would probably be easier to review these changes if you made a review board review as I'd get a bit more context there]

        Specifically I'm a little unclear about some of the cmake changes:

        • Introduced a help2man build in docs/man (and done a neat jobs of it) however we've actually stopped doing this build entirely from our current build. However it's unfortunately not removed completely yet though so this work isn't currently useful to us.
        • Removed Boost libs from the detection list that are required on windows even though they are not required on unix - there is even a comment just above explaining this.
        • This change:
        • else (CMAKE_SYSTEM_NAME STREQUAL Windows)
          + else (CMAKE_SYSTEM_NAME STREQUAL Linux)
          In ssl.cmake just looks wrong to me and I wonder if its ever been run, as it should make cmake give an error (CMake cares about matching the "if" parameters with the "else" parameters and gives an error if they don't match but the corresponding "if" hasn't changed.
        • Added unconditional pragmas into the code to deal with warnings from the boost library, but this must either depend on boost version or the compiler version and so should be much more tightly constrained with appropriate ifdefs and comments that explain when you need to turn these warnings off. And preferably they should be turned off only for the shortest time that's necessary - I suspect it's a template instantiation that causes the problem so this might be hard.

        Having made these detailed comments, on the whole this is a very good piece of work and neatens some things a lot. It looks like it moves in the direction of cmake build parity too which is a big plus.

        Show
        Andrew Stitcher added a comment - I note that the diffs seem to be against 0.10 so there is some chance they don't apply cleanly against trunk. [BTW it would probably be easier to review these changes if you made a review board review as I'd get a bit more context there] Specifically I'm a little unclear about some of the cmake changes: Introduced a help2man build in docs/man (and done a neat jobs of it) however we've actually stopped doing this build entirely from our current build. However it's unfortunately not removed completely yet though so this work isn't currently useful to us. Removed Boost libs from the detection list that are required on windows even though they are not required on unix - there is even a comment just above explaining this. This change: else (CMAKE_SYSTEM_NAME STREQUAL Windows) + else (CMAKE_SYSTEM_NAME STREQUAL Linux) In ssl.cmake just looks wrong to me and I wonder if its ever been run, as it should make cmake give an error (CMake cares about matching the "if" parameters with the "else" parameters and gives an error if they don't match but the corresponding "if" hasn't changed. Added unconditional pragmas into the code to deal with warnings from the boost library, but this must either depend on boost version or the compiler version and so should be much more tightly constrained with appropriate ifdefs and comments that explain when you need to turn these warnings off. And preferably they should be turned off only for the shortest time that's necessary - I suspect it's a template instantiation that causes the problem so this might be hard. Having made these detailed comments, on the whole this is a very good piece of work and neatens some things a lot. It looks like it moves in the direction of cmake build parity too which is a big plus.
        Hide
        Andrew Stitcher added a comment - - edited

        In the 06 patch there are a number of uses of strerror():

        • the project standard is to use :: to preface global library calls (to disambiguate them from any local meaning of the function name),

        BUT

        • strerror() is not thread safe so we have an internal thread safe strError() function that we use instead - you should use this.
        Show
        Andrew Stitcher added a comment - - edited In the 06 patch there are a number of uses of strerror(): the project standard is to use :: to preface global library calls (to disambiguate them from any local meaning of the function name), BUT strerror() is not thread safe so we have an internal thread safe strError() function that we use instead - you should use this.
        Hide
        Andrew Stitcher added a comment -

        The poller selection patch (02):

        I like the changes however I think they are based on a small misunderstanding:

        At present the poll based poller does not exist - it has long been suggested as a fallback poller for posix based systems but never been actually implemented, so unhappily it needs to be commented out from actually being detected.

        Actually I don't think the Solaris ecf poller actually works although there is bit rotted code in the tree for it.

        Show
        Andrew Stitcher added a comment - The poller selection patch (02): I like the changes however I think they are based on a small misunderstanding: At present the poll based poller does not exist - it has long been suggested as a fallback poller for posix based systems but never been actually implemented, so unhappily it needs to be commented out from actually being detected. Actually I don't think the Solaris ecf poller actually works although there is bit rotted code in the tree for it.
        Hide
        Jan-Marek Glogowski added a comment -

        I'm using these patches to build qpid v0.12 using cmake on Debian Squeeze.

        When I started packaging I was using v0.10, that' why some diffs say v0.10, but they still apply (and also apply to current git with some offsets, but I didn't try to build).

        I'll incorporate your comments and attach new patches.

        Show
        Jan-Marek Glogowski added a comment - I'm using these patches to build qpid v0.12 using cmake on Debian Squeeze. When I started packaging I was using v0.10, that' why some diffs say v0.10, but they still apply (and also apply to current git with some offsets, but I didn't try to build). I'll incorporate your comments and attach new patches.
        Hide
        Gordon Sim added a comment -

        Patches apply to trunk fine for me and build and test passes. One comment is that the --prefix option to configure is no longer sufficient for installing to a non-standard location. The python binding libs are installed to a directory independent of any prefix. You can of course separately set the location for these when running configure but its not as obvious. The change in location however is entirely correct (indeed this will also resolve QPID-3458!).

        Thanks for taking the time to create and submit these!

        Show
        Gordon Sim added a comment - Patches apply to trunk fine for me and build and test passes. One comment is that the --prefix option to configure is no longer sufficient for installing to a non-standard location. The python binding libs are installed to a directory independent of any prefix. You can of course separately set the location for these when running configure but its not as obvious. The change in location however is entirely correct (indeed this will also resolve QPID-3458 !). Thanks for taking the time to create and submit these!
        Hide
        Jan-Marek Glogowski added a comment -

        Thanks for the comments. I've attached an additional patch which

        • fixes another ssl.cmake CnP error,
        • changes the strerror calls to strError,
        • distinguishes between Linux and Windows on find_package(Boost ...)
        • makes the pragma code depend on GCC >= 4.2.
          (at least it's the first time documented in the gcc pragma docs in v4.2)

        Comments:

        Use review board

        I've created an account on reviews.apache.org (which was hard to find).

        • Should I upload a combined patch to reviewboard?
        • What is the "Base Directory"? (if I generate a patch using git diff, base is '/' ?)

        Probably someone can add some additional information to the Patch Submission paragraph on http://qpid.apache.org/qpid_project_etiquette_guide.html and also including a link to JIRA.

        • Introduced a help2man build in docs/man ...

        What's the preferred way to build the manpages? There is the cpp/docs/man/generate_manpage script, which calls sed...

        • Removed Boost libs from the detection list ...

        Fixed

        • ssl.cmake changes (else (CMAKE_SYSTEM_NAME STREQUAL Windows))

        My ssl.cmake change is correct; the else block contains the linux ssl build code. The original code was:

        if (CMAKE_SYSTEM_NAME STREQUAL Windows)
        else (CMAKE_SYSTEM_NAME STREQUAL Windows)
        endif (CMAKE_SYSTEM_NAME STREQUAL Windows)

        • Added unconditional pragmas ...

        Fixed

        In the 06 patch there are a number of uses of strerror() ...

        Fixed

        The poller selection patch (02)

        I'm currently building using cmake. I used this patch to allow auto-selection of the poller code, when I build QPID using autotools. I don't care if the patch won't be accepted - it just seemed convenient.

        Patches apply to trunk fine for me and build and test passes. One comment is that the --prefix option to configure is no longer sufficient ...

        PYTHON_LIB is set in the configure.ac file AC_SUBST'ed as [Directory to install python bindings in].
        Maybe it's better to use pythondir from AM_PATH_PYTHON (cpp/m4/python.m4), which uses PYTHON_PREFIX, which is set to '$

        {prefix}

        ' and drop all the PYTHON_LIB code fom configure.ac?

        Show
        Jan-Marek Glogowski added a comment - Thanks for the comments. I've attached an additional patch which fixes another ssl.cmake CnP error, changes the strerror calls to strError, distinguishes between Linux and Windows on find_package(Boost ...) makes the pragma code depend on GCC >= 4.2. (at least it's the first time documented in the gcc pragma docs in v4.2) Comments: Use review board I've created an account on reviews.apache.org (which was hard to find). Should I upload a combined patch to reviewboard? What is the "Base Directory"? (if I generate a patch using git diff, base is '/' ?) Probably someone can add some additional information to the Patch Submission paragraph on http://qpid.apache.org/qpid_project_etiquette_guide.html and also including a link to JIRA. Introduced a help2man build in docs/man ... What's the preferred way to build the manpages? There is the cpp/docs/man/generate_manpage script, which calls sed... Removed Boost libs from the detection list ... Fixed ssl.cmake changes (else (CMAKE_SYSTEM_NAME STREQUAL Windows)) My ssl.cmake change is correct; the else block contains the linux ssl build code. The original code was: if (CMAKE_SYSTEM_NAME STREQUAL Windows) else (CMAKE_SYSTEM_NAME STREQUAL Windows) endif (CMAKE_SYSTEM_NAME STREQUAL Windows) Added unconditional pragmas ... Fixed In the 06 patch there are a number of uses of strerror() ... Fixed The poller selection patch (02) I'm currently building using cmake. I used this patch to allow auto-selection of the poller code, when I build QPID using autotools. I don't care if the patch won't be accepted - it just seemed convenient. Patches apply to trunk fine for me and build and test passes. One comment is that the --prefix option to configure is no longer sufficient ... PYTHON_LIB is set in the configure.ac file AC_SUBST'ed as [Directory to install python bindings in] . Maybe it's better to use pythondir from AM_PATH_PYTHON (cpp/m4/python.m4), which uses PYTHON_PREFIX, which is set to '$ {prefix} ' and drop all the PYTHON_LIB code fom configure.ac?
        Hide
        Jan-Marek Glogowski added a comment -
        • fixes another ssl.cmake CnP error,
        • changes the strerror calls to strError,
        • distinguishes between Linux and Windows on find_package(Boost ...)
        • makes the pragma code depend on GCC >= 4.2.
        Show
        Jan-Marek Glogowski added a comment - fixes another ssl.cmake CnP error, changes the strerror calls to strError, distinguishes between Linux and Windows on find_package(Boost ...) makes the pragma code depend on GCC >= 4.2.
        Hide
        Jose Pedro Oliveira added a comment -

        Andrew,

        Would it be possible to also apply the patch "04_reorganize-automake-qpid-perl-bindings-build.diff" ?

        Note: it complements QPID-3009 and it would also fix QPID-3574.

        /jpo

        Show
        Jose Pedro Oliveira added a comment - Andrew, Would it be possible to also apply the patch "04_reorganize-automake-qpid-perl-bindings-build.diff" ? Note: it complements QPID-3009 and it would also fix QPID-3574 . /jpo
        Hide
        Andrew Stitcher added a comment -

        @Jose Pedro Olveira

        I'm planning to apply that patch.

        Show
        Andrew Stitcher added a comment - @Jose Pedro Olveira I'm planning to apply that patch.
        Hide
        Jose Pedro Oliveira added a comment -

        Andrew,

        Thanks for applying the patch "04_reorganize-automake-qpid-perl-bindings-build.diff" to trunk.
        Is it possible to backport it to the 0.14 branch? And also update the Perl examples with the
        patch in QPID-3313?

        tia,
        jpo

        Show
        Jose Pedro Oliveira added a comment - Andrew, Thanks for applying the patch "04_reorganize-automake-qpid-perl-bindings-build.diff" to trunk. Is it possible to backport it to the 0.14 branch? And also update the Perl examples with the patch in QPID-3313 ? tia, jpo
        Hide
        Andrew Stitcher added a comment -

        You should raise this on the qpid-dev list if you think it is important enough. The 0.14 release has closed down for fixes only at this point.

        Show
        Andrew Stitcher added a comment - You should raise this on the qpid-dev list if you think it is important enough. The 0.14 release has closed down for fixes only at this point.
        Hide
        Andrew Stitcher added a comment - - edited

        I have committed the most of the build improvement changes now, much of the work was used but I have modified it in places so the patches will not exactly match what has been checked in.

        • The improvements to the cmake build made it for the 0.14 release, however
        • The improvements to the autotools release just missed the cut.

        The patches I haven't committed I either didn't like, thought were wrong, didn't understand the purpose of or thought they weren't connected either to the original or final bug title. If they are important to you please resubmit them against trunk.

        Show
        Andrew Stitcher added a comment - - edited I have committed the most of the build improvement changes now, much of the work was used but I have modified it in places so the patches will not exactly match what has been checked in. The improvements to the cmake build made it for the 0.14 release, however The improvements to the autotools release just missed the cut. The patches I haven't committed I either didn't like, thought were wrong, didn't understand the purpose of or thought they weren't connected either to the original or final bug title. If they are important to you please resubmit them against trunk.

          People

          • Assignee:
            Andrew Stitcher
            Reporter:
            Jan-Marek Glogowski
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development