Review Request 128848: Split APK packaging logic out of Android toolchain

[Friedrich W. H. Kossebau]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20161212/d807015f/attachment.html>