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

Sat Mar 2 12:06:49 GMT 2013

> On Feb. 25, 2013, 1:01 p.m., Arjen Hiemstra wrote:
> > CMakeLists.txt, lines 31-43
> > <http://git.reviewboard.kde.org/r/109071/diff/2/?file=115185#file115185line31>
> >
> >     Why not put these definitions into separate files like is already done with other macros like calligra_build_test?

Yes, properly better. Moving all productset related macros into a separate file. Perhaps these macros could be even reused by other projects, so trying to make them not too Calligra-specific for now, besides namespacing.

> On Feb. 25, 2013, 1:01 p.m., Arjen Hiemstra wrote:
> > CMakeLists.txt, line 120
> > <http://git.reviewboard.kde.org/r/109071/diff/2/?file=115185#file115185line120>
> >
> >     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.
> >

Okay, done. Also added a small README describing how own productsets can be added (as I implemented initial support for that as well).

> On Feb. 25, 2013, 1:01 p.m., Arjen Hiemstra wrote:
> > CMakeLists.txt, line 121
> > <http://git.reviewboard.kde.org/r/109071/diff/2/?file=115185#file115185line121>
> >
> >     Please make productset matching case insensitive so we avoid cases where things fail due to people writing -DPRODUCTSET=Creative etc.

No definite opinion on that myself, so case-insensitiveness added.

> On Feb. 25, 2013, 1:01 p.m., Arjen Hiemstra wrote:
> > CMakeLists.txt, line 205
> > <http://git.reviewboard.kde.org/r/109071/diff/2/?file=115185#file115185line205>
> >
> >     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.

Agreed. Changed the code to just fail if no matching productset is found.

> On Feb. 25, 2013, 1:01 p.m., Arjen Hiemstra wrote:
> > libs/CMakeLists.txt, line 18
> > <http://git.reviewboard.kde.org/r/109071/diff/2/?file=115191#file115191line18>
> >
> >     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.

That means to completely "producterize" all of Calligra already. Hm. Perhaps you are right and I should see to go all the way in this patch already. 

So I have to find a way to generalize the inter-product dependencies...

- Friedrich W. H.

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

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/20130302/1cf8db4a/attachment.htm>