Review Request 109071: Add a PRODUCTSET build config parameter (values ACTIVE, CREATIVE, DESKTOP, ALL)

[Arjen Hiemstra]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109071/#review28007
-----------------------------------------------------------


Nice idea with the productset. Here are some comments about things I noticed.


CMakeLists.txt
<http://git.reviewboard.kde.org/r/109071/#comment20971>

    Why not put these definitions into separate files like is already done with other macros like calligra_build_test?



CMakeLists.txt
<http://git.reviewboard.kde.org/r/109071/#comment20975>

    Like you have already mentioned, these product sets can be put into separate files as well. I would do that in this patch, I see no reason not to.
    



CMakeLists.txt
<http://git.reviewboard.kde.org/r/109071/#comment20976>

    Please make productset matching case insensitive so we avoid cases where things fail due to people writing -DPRODUCTSET=Creative etc.



CMakeLists.txt
<http://git.reviewboard.kde.org/r/109071/#comment20977>

    If you want to support and encourage use of alternate productsets, I would not use an empty default but rather print an error when an unrecognised product set is used. This helps people developing new product sets since they will know when things go wrong.



libs/CMakeLists.txt
<http://git.reviewboard.kde.org/r/109071/#comment20979>

    Why not add a SHOULD_BUILD_KOPAGEAPP or so for this? Since it is a library it is not inconceivable that someone wants to build it without building Flow or Stage.



libs/CMakeLists.txt
<http://git.reviewboard.kde.org/r/109071/#comment20981>

    Is this a separate library from KoReport? If so, see the comment above. If not, can we change the todo to move it to somewhere part of KoReport?


- Arjen Hiemstra


On Feb. 24, 2013, 12:11 a.m., Friedrich W. H. Kossebau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109071/
> -----------------------------------------------------------
> 
> (Updated Feb. 24, 2013, 12:11 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> PRODUCTSET is a substitute for the old non-exclusive options CREATIVEONLY and TINY (which then are handled exclusively, eek), with migration support for CREATIVEONLY flag. Predefined hardcoded productsets are ACTIVE, CREATIVE, DESKTOP, and ALL (as fallback and default).
>     
> Patch also turns buildsystem to have a SHOULD_BUILD for each product (app/plugin), which then all get turned on centrally in groups depending on the productset, instead of everywhere having overlapping and hard to oversee if-else blocks deciding what gets build and what not.
> 
> Not the perfect final solution, but a first step into the right direction IMHO.
> 
> Known issues:
> * BUILD_AUTHOR is not yet set
> 
> Patch can be also tested as branch addProductSetBuildParameter-kossebau.
> 
> 
> Diffs
> -----
> 
>   stage/part/stage.desktop 447858f 
>   words/CMakeLists.txt e6336a2 
>   words/app/CMakeLists.txt PRE-CREATION 
>   words/app/Info.plist.template PRE-CREATION 
>   words/app/main.cpp PRE-CREATION 
>   words/app/words.desktop PRE-CREATION 
>   words/part/CMakeLists.txt 56b8c6f 
>   words/part/Info.plist.template 97e1728 
>   words/part/main.cpp 875eb5d 
>   words/part/words.desktop 35bc4c3 
>   CMakeLists.txt 564c0a0 
>   devtools/CMakeLists.txt f0e7b62 
>   extras/CMakeLists.txt 70edca7 
>   filters/CMakeLists.txt 5acecef 
>   filters/sheets/CMakeLists.txt 351a8e2 
>   filters/words/CMakeLists.txt 0c2107c 
>   libs/CMakeLists.txt 2036cf5 
>   plugins/CMakeLists.txt 0e87b1e 
>   sheets/CMakeLists.txt 9f96e41 
>   stage/CMakeLists.txt 94dd31c 
>   stage/app/CMakeLists.txt PRE-CREATION 
>   stage/app/Info.plist.template PRE-CREATION 
>   stage/app/main.cpp PRE-CREATION 
>   stage/app/stage.desktop PRE-CREATION 
>   stage/part/CMakeLists.txt de57a0f 
>   stage/part/Info.plist.template 857a8d7 
>   stage/part/main.cpp 5ef9509 
>   3rdparty/CMakeLists.txt a300bd2 
> 
> Diff: http://git.reviewboard.kde.org/r/109071/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20130225/802c0304/attachment.htm>