----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128848/#review101383 -----------------------------------------------------------
Only had a quick look, did not try to understand in detail or even did a test of the new code. No principal issue spotted for now. Just can tell that building Marble for Android works also with this patch applied (as it has it's own packaging code currently for various reasons, so the changes do not affect it :) ). modules/ECMAddApkPackageTarget.cmake (line 1) <https://git.reviewboard.kde.org/r/128848/#comment67883> To have this documentation appear in the generated documentation, also a file docs/module/ECMAddApkPackageTarget.rst with content ``` .. ecm-module:: ../../modules/ECMAddApkPackageTarget.cmake ``` seems to be needed. modules/ECMAddApkPackageTarget.cmake (line 9) <https://git.reviewboard.kde.org/r/128848/#comment67880> "Usually," what? you leave us curious :) modules/ECMAddApkPackageTarget.cmake (line 24) <https://git.reviewboard.kde.org/r/128848/#comment67881> Please consider adding a separate comment about each argument, as well as one example how to use it. Cmp. the documentation of other ECM macros. modules/ECMAddApkPackageTarget.cmake (line 40) <https://git.reviewboard.kde.org/r/128848/#comment67884> Just `ECM_ADD_APK_PACKAGE` should be enough IMHO, the target is implicite and also not used as term with similar ECM macros. modules/ECMAddApkPackageTarget.cmake (line 55) <https://git.reviewboard.kde.org/r/128848/#comment67886> Also needs to check for presence of the androiddeployqt executable. modules/ECMAddApkPackageTarget_dependencyHelper.cmake (line 9) <https://git.reviewboard.kde.org/r/128848/#comment67882> Please consider following the documentation style of the ECM macros for consistency: first the generic macro signature, then listing each argument in a separate block. modules/ECMAddApkPackageTarget_dependencyHelper.cmake (line 44) <https://git.reviewboard.kde.org/r/128848/#comment67885> "#we" what ? :) - Friedrich W. H. Kossebau On Sept. 6, 2016, 12:28 p.m., Andreas Cord-Landwehr wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128848/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2016, 12:28 p.m.) > > > Review request for Extra Cmake Modules and Aleix Pol Gonzalez. > > > Repository: extra-cmake-modules > > > Description > ------- > > Create a module named "ECMAddApkPackageTarget" that handles > all APK packaging logic for Android builds. This will helper > in a later integration of APK creation for autotests. > > > Diffs > ----- > > modules/ECMAddApkPackageTarget.cmake PRE-CREATION > modules/ECMAddApkPackageTarget_dependencyHelper.cmake PRE-CREATION > tests/CMakeLists.txt dd1350ada4d2316a3329e4f4fcdcd74c628ab30e > tests/ECMToolchainAndroidTest/CMakeLists.txt > eb2ae2980140c32e9d5c9c7c38913fd5420021d3 > tests/ECMToolchainAndroidTest/main.c > > tests/ECMToolchainAndroidTest/testlinkfile/CMakeFiles/testtarget.dir/link.txt > > tests/ECMToolchainAndroidTest/testlinkfile/outputfake.json > toolchain/Android.cmake 20e65f87ac5a4fe9a0089c596d1d392214053817 > toolchain/deployment-file.json.in > toolchain/specifydependencies.cmake > 65e875bf23a7e22f56fbfad3df8c22e1444bd9ea > > Diff: https://git.reviewboard.kde.org/r/128848/diff/ > > > Testing > ------- > > Manual build testing > > > Thanks, > > Andreas Cord-Landwehr > >
