| Summary: | Which features are not checked, and just assumed not to be present under cmake | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Ralf Habacker <ralf.habacker> |
| Component: | core | Assignee: | D-Bus Maintainers <dbus> |
| Status: | RESOLVED MOVED | QA Contact: | D-Bus Maintainers <dbus> |
| Severity: | normal | ||
| Priority: | medium | CC: | ralf.habacker |
| Version: | 1.8 | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
example config.h autotools cmake diff on linux
example command line autotools cmake macro diff Add cmake macro autoheaderchecks() Keep include file checks in sync with autotools Add check to cmake build system if config.h.cmake is in sync with autotools Move include file checks to ConfigureChecks.cmake for cmake build system. Keep cmake generated defines for include files in sync with autotools Check config.h differences with cmake build system only when config.h.in is present |
||
|
Description
Ralf Habacker
2014-10-24 20:26:45 UTC
Created attachment 108368 [details]
example config.h autotools cmake diff on linux
Created attachment 108369 [details]
example command line autotools cmake macro diff
Created attachment 114020 [details] [review] Add cmake macro autoheaderchecks() Created attachment 114021 [details] [review] Keep include file checks in sync with autotools Comment on attachment 114020 [details] [review] Add cmake macro autoheaderchecks() Review of attachment 114020 [details] [review]: ----------------------------------------------------------------- Interesting approach! Patch looks good, thanks. Comment on attachment 114021 [details] [review] Keep include file checks in sync with autotools Review of attachment 114021 [details] [review]: ----------------------------------------------------------------- Sure, looks good. Created attachment 114029 [details] [review] Add check to cmake build system if config.h.cmake is in sync with autotools Created attachment 114030 [details] [review] Move include file checks to ConfigureChecks.cmake for cmake build system. Created attachment 114031 [details] [review] Keep cmake generated defines for include files in sync with autotools Comment on attachment 114029 [details] [review] Add check to cmake build system if config.h.cmake is in sync with autotools Review of attachment 114029 [details] [review]: ----------------------------------------------------------------- OK Comment on attachment 114030 [details] [review] Move include file checks to ConfigureChecks.cmake for cmake build system. Review of attachment 114030 [details] [review]: ----------------------------------------------------------------- OK Comment on attachment 114031 [details] [review] Keep cmake generated defines for include files in sync with autotools Review of attachment 114031 [details] [review]: ----------------------------------------------------------------- ::: cmake/config.h.cmake @@ +172,5 @@ > +/* Define to 1 if you have ws2tcpip.h */ > +#cmakedefine HAVE_WS2TCPIP_H > + > +/* Define to 1 if you have unistd.h */ > +#cmakedefine HAVE_UNISTD_H 1 Sure, looks good. Having defines for all the checks is better, and wherever the order is not significant, alphabetical order is good. Any particular reason why ws2tcpip.h and unistd.h are not in alphabetical order? (Not a merge blocker regardless, the patch is an improvement.) (In reply to Simon McVittie from comment #12) > Comment on attachment 114031 [details] [review] [review] > Keep cmake generated defines for include files in sync with autotools > > Review of attachment 114031 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: cmake/config.h.cmake > @@ +172,5 @@ > > +/* Define to 1 if you have ws2tcpip.h */ > > +#cmakedefine HAVE_WS2TCPIP_H > > + > > +/* Define to 1 if you have unistd.h */ > > +#cmakedefine HAVE_UNISTD_H 1 > > Sure, looks good. Having defines for all the checks is better, and wherever > the order is not significant, alphabetical order is good. > > Any particular reason why ws2tcpip.h and unistd.h are not in alphabetical > order? (Not a merge blocker regardless, the patch is an improvement.) no just oversaw it. Will commit with corrected order. Comment on attachment 114029 [details] [review] Add check to cmake build system if config.h.cmake is in sync with autotools applied Comment on attachment 114021 [details] [review] Keep include file checks in sync with autotools applied Comment on attachment 114020 [details] [review] Add cmake macro autoheaderchecks() applied Comment on attachment 114030 [details] [review] Move include file checks to ConfigureChecks.cmake for cmake build system. applied Comment on attachment 114031 [details] [review] Keep cmake generated defines for include files in sync with autotools applied This isn't working for me on Debian (cmake 3.0.2): CMake Error at ConfigureChecks.cmake:26 (check_include_files): Unknown CMake command "check_include_files". Call Stack (most recent call first): CMakeLists.txt:130 (INCLUDE) Is that a recently-added macro, or one that needs third-party modules? <sys/event.h> is for bus/dir-watch-kqueue.c, which is a BSD-specific feature - I don't mind that not working under CMake. Everyone uses Autotools on *BSD anyway. (In reply to Simon McVittie from comment #19) > This isn't working for me on Debian (cmake 3.0.2): > > CMake Error at ConfigureChecks.cmake:26 (check_include_files): > Unknown CMake command "check_include_files". > Call Stack (most recent call first): > CMakeLists.txt:130 (INCLUDE) > > Is that a recently-added macro, or one that needs third-party modules? > > <sys/event.h> is for bus/dir-watch-kqueue.c, which is a BSD-specific feature > - I don't mind that not working under CMake. Everyone uses Autotools on *BSD > anyway. There is an include file CheckIncludeFiles missing in cmake/ConfigureChecks.cmake and already fixed in git master. Comment on attachment 114020 [details] [review] Add cmake macro autoheaderchecks() Review of attachment 114020 [details] [review]: ----------------------------------------------------------------- ::: cmake/CMakeLists.txt @@ +17,4 @@ > include(MacrosAutotools) > autoinit(../configure.ac) > autoversion(dbus) > +autoheaderchecks(../config.h.in ConfigureChecks.cmake) Found out on another installation, that ../config.h.in is not included in git repo, but generated by autogen.sh I need to add an additional patch like this if(EXITST ../config.h.in) autoheaderchecks(../config.h.in ConfigureChecks.cmake) endif() Created attachment 114040 [details] [review] Check config.h differences with cmake build system only when config.h.in is present (In reply to Simon McVittie from comment #19) > This isn't working for me on Debian (cmake 3.0.2): > > CMake Error at ConfigureChecks.cmake:26 (check_include_files): > Unknown CMake command "check_include_files". > Call Stack (most recent call first): > CMakeLists.txt:130 (INCLUDE) > > Is that a recently-added macro, or one that needs third-party modules? > > <sys/event.h> is for bus/dir-watch-kqueue.c, which is a BSD-specific feature > - I don't mind that not working under CMake. Everyone uses Autotools on *BSD > anyway. This macro has been moved from top level CMakeList.txt into ConfigureChecks.cmake with attachment 114030 [details] [review] and has been introduced with commit http://cgit.freedesktop.org/dbus/dbus/commit/?id=1ec2e5a5387d131efc6affb7bfc49642c8186df8 Comment on attachment 114040 [details] [review] Check config.h differences with cmake build system only when config.h.in is present Review of attachment 114040 [details] [review]: ----------------------------------------------------------------- Makes sense (In reply to Ralf Habacker from comment #20) > There is an include file CheckIncludeFiles missing in > cmake/ConfigureChecks.cmake and already fixed in git master. Right, commit ee9c52c resolves this for me. (In reply to Simon McVittie from comment #24) > Comment on attachment 114040 [details] [review] [review] > Check config.h differences with cmake build system only when config.h.in is > present > > Makes sense Has been superseeded by commit b7086e051347cff5a71b63a5010e4c280c4adf4b The remaining part is to print a cmake status message if config.h.in is not present. As the patch has been already reviewed, I'm going to commit the rest with a modified commit message. Comment on attachment 114040 [details] [review] Check config.h differences with cmake build system only when config.h.in is present committed to master -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/117. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.