Re: Review Request 113406: Add a macro to automatically generate forward headers
On Oct. 30, 2013, 10:47 a.m., David Faure wrote: Looks good to me (apart from the expired pastebin links in the initial description). I can't Ship it because I'm not a cmake guru/maintainer though. Same here, looks OK, but I'd like Alex or Stephen to give it the ship it. - Kevin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/#review42700 --- On Oct. 29, 2013, 10:52 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 29, 2013, 10:52 a.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. File Attachments kparts changes example http://git.reviewboard.kde.org/media/uploaded/files/2013/10/29/a3557579-801b-4ee6-8e3d-5d487428759a__kf5-kparts-headers-test.patch Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote: modules/ECMGenerateHeaders.cmake, line 32 http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line32 I recommend not putting this in the API of the function, and instead users should use install(DIRECTORY) to install the generated files. Aleix Pol Gonzalez wrote: Why? Stephen Kelly wrote: I think it's better API. It's what all existing functions which generate headers do. Aleix Pol Gonzalez wrote: Then it's weird because we'll need to have a separate call for installing all the headers as well, so we will have them all listed twice. Stephen Kelly wrote: Are you sure? You wouldn't just have a single install(DIRECTORY), as I wrote before? See the KParts patch I attached. We want to install the actual header, not the forward header, right? - Aleix --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/#review42280 --- On Oct. 29, 2013, 12:32 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 29, 2013, 12:32 a.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. File Attachments example of how KParts would change http://git.reviewboard.kde.org/media/uploaded/files/2013/10/29/43e2c958-9908-4249-9bb6-9395d91b5643__kf5-kparts-headers-test.patch Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 29, 2013, 12:32 a.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. File Attachments example of how KParts would change http://git.reviewboard.kde.org/media/uploaded/files/2013/10/29/43e2c958-9908-4249-9bb6-9395d91b5643__kf5-kparts-headers-test.patch Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/#review42415 --- Thanks for working on this! modules/ECMGenerateHeaders.cmake http://git.reviewboard.kde.org/r/113406/#comment30804 is used += to modules/ECMGenerateHeaders.cmake http://git.reviewboard.kde.org/r/113406/#comment30805 This variable name isn't intuitive, there are headers everywhere (source, binary, install dir). Maybe call it SOURCE_DIR or HEADERS_SOURCE_DIR? - David Faure On Oct. 26, 2013, 10:56 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 26, 2013, 10:56 a.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote: modules/ECMGenerateHeaders.cmake, line 29 http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line29 This variable shouldn't be needed at all. Aleix Pol Gonzalez wrote: Variable? Or argument? Why? Stephen Kelly wrote: Argument, yes, sorry. I think that because the source dir is already in the INTERFACE_INCLUDE_DIRECTORIES of the target, there is no need to have to specify it, and there should be no need to use ${EGH_HEADERS_DIR}/ in the generated include. I guess you have some reason for doing that, but that just points to non-uniformity which should maybe be fixed, as I wrote in the KIO commit I linked to. Alexander Neundorf wrote: I don't see any target involved here... So maybe we should link the generated headers to a target? - Aleix --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/#review42280 --- On Oct. 24, 2013, 1:40 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 24, 2013, 1:40 p.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 26, 2013, 9:35 a.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Changes --- Remove the install functionality, and the INSTALL_DIR argument with it, cmake can install the files quite easily anyway. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs (updated) - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/#review42396 --- The install dir is still in the documentation and in the cmake_parse_arguments() call. - Alexander Neundorf On Oct. 26, 2013, 9:35 a.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 26, 2013, 9:35 a.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 26, 2013, 10:56 a.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Changes --- My bad, I uploaded the wrong patch version. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs (updated) - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 24, 2013, 1:40 p.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Changes --- Adopt changes proposed by Neundorf. - Improved documentation. - Added different arguments to configure how the macro works. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs (updated) - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/#review42280 --- modules/ECMGenerateHeaders.cmake http://git.reviewboard.kde.org/r/113406/#comment30766 I really think the answers to my questions here need to be found first: http://thread.gmane.org/gmane.comp.kde.cvs/1272841 It should be more-simple than it currently is. modules/ECMGenerateHeaders.cmake http://git.reviewboard.kde.org/r/113406/#comment30767 This variable shouldn't be needed at all. modules/ECMGenerateHeaders.cmake http://git.reviewboard.kde.org/r/113406/#comment30768 I recommend not putting this in the API of the function, and instead users should use install(DIRECTORY) to install the generated files. - Stephen Kelly On Oct. 24, 2013, 1:40 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 24, 2013, 1:40 p.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote: modules/ECMGenerateHeaders.cmake, line 29 http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line29 This variable shouldn't be needed at all. Variable? Or argument? Why? On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote: modules/ECMGenerateHeaders.cmake, line 32 http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line32 I recommend not putting this in the API of the function, and instead users should use install(DIRECTORY) to install the generated files. Why? On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote: modules/ECMGenerateHeaders.cmake, line 11 http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line11 I really think the answers to my questions here need to be found first: http://thread.gmane.org/gmane.comp.kde.cvs/1272841 It should be more-simple than it currently is. I think that this patch answers most of these questions, leaving up to discussion if we expect people to include module/something.h or just something.h. I'd say that if Qt5 transition teaches something is that it's not very flexible to namespace the include files, but that's up to the developer anyway... - Aleix --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/#review42280 --- On Oct. 24, 2013, 1:40 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 24, 2013, 1:40 p.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote: modules/ECMGenerateHeaders.cmake, line 11 http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line11 I really think the answers to my questions here need to be found first: http://thread.gmane.org/gmane.comp.kde.cvs/1272841 It should be more-simple than it currently is. Aleix Pol Gonzalez wrote: I think that this patch answers most of these questions, leaving up to discussion if we expect people to include module/something.h or just something.h. I'd say that if Qt5 transition teaches something is that it's not very flexible to namespace the include files, but that's up to the developer anyway... I'm not sure it does... For example, grantlee installs headers like this: include/grantlee/engine.h include/grantlee_export.h include/grantlee/engine.h has include grantlee_export.h and even if it is grantlee_export.h, the intent is that the user only needs -Iinclude/, and does not need -Iinclude/grantlee. The same is not true of where KF5 installs headers and how they are included. There is scope for improvement there, and this patch does not affect that. On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote: modules/ECMGenerateHeaders.cmake, line 29 http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line29 This variable shouldn't be needed at all. Aleix Pol Gonzalez wrote: Variable? Or argument? Why? Argument, yes, sorry. I think that because the source dir is already in the INTERFACE_INCLUDE_DIRECTORIES of the target, there is no need to have to specify it, and there should be no need to use ${EGH_HEADERS_DIR}/ in the generated include. I guess you have some reason for doing that, but that just points to non-uniformity which should maybe be fixed, as I wrote in the KIO commit I linked to. On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote: modules/ECMGenerateHeaders.cmake, line 32 http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line32 I recommend not putting this in the API of the function, and instead users should use install(DIRECTORY) to install the generated files. Aleix Pol Gonzalez wrote: Why? I think it's better API. It's what all existing functions which generate headers do. - Stephen --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/#review42280 --- On Oct. 24, 2013, 1:40 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 24, 2013, 1:40 p.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote: modules/ECMGenerateHeaders.cmake, line 29 http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line29 This variable shouldn't be needed at all. Aleix Pol Gonzalez wrote: Variable? Or argument? Why? Stephen Kelly wrote: Argument, yes, sorry. I think that because the source dir is already in the INTERFACE_INCLUDE_DIRECTORIES of the target, there is no need to have to specify it, and there should be no need to use ${EGH_HEADERS_DIR}/ in the generated include. I guess you have some reason for doing that, but that just points to non-uniformity which should maybe be fixed, as I wrote in the KIO commit I linked to. I don't see any target involved here... - Alexander --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/#review42280 --- On Oct. 24, 2013, 1:40 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 24, 2013, 1:40 p.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Review Request 113406: Add a macro to automatically generate forward headers
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
Re: Review Request 113406: Add a macro to automatically generate forward headers
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/#review42250 --- IMO the documentation could be improved. It should mention the assumption about the location of ${actualheader}. It should also mention the use of ${PROJECT_NAME}. It needs to mention that it assumes that a variable ${INCLUDE_INSTALL_DIR} is set, and that this is used for installing the generated headers. I often found that a function/macro started like this, with just a list of arguments, and later on additional options became necessary (the use of PROJECT_NAME and the install dir are candidates here). This is hard to do without having keywords in the arguments which separate the different options. It would be nice if the signature could be changed to ecm_generate_headers(CLASSES ClassA...ClassN [MODULE_NAME SomeName] [INSTALL_DIR /some/dir] ...) using the cmake_parse_arguments() function. - Alexander Neundorf On Oct. 23, 2013, 5:38 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113406/ --- (Updated Oct. 23, 2013, 5:38 p.m.) Review request for Build System, KDE Frameworks and Stephen Kelly. Repository: extra-cmake-modules Description --- Create a macro that will generate the forward headers like we used to, in cmake configure time. Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051 After the change, these are the installed headers: http://paste.opensuse.org/90980400 Diffs - modules/ECMGenerateHeaders.cmake PRE-CREATION Diff: http://git.reviewboard.kde.org/r/113406/diff/ Testing --- Ported KParts and still everything works. Thanks, Aleix Pol Gonzalez ___ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem