Review Request: Allow to pass multiple targets and build variables to the make builder

[Milian Wolff]

> On April 13, 2012, 9:27 p.m., Milian Wolff wrote:
> > projectbuilders/makebuilder/imakebuilder.h, line 26
> > <http://git.reviewboard.kde.org/r/104537/diff/2/?file=56290#file56290line26>
> >
> >     hm a QMap will implicitly sort the variables, that might not be desired. i.e. if you want to have
> >     
> >     make FOO=bar ASDF=asdf
> >     
> >     you will actually get
> >     
> >     make ASDF=asdf FOO=bar
> >     
> >     instead it might be better to just go for a QStringList of arguments, which would also allow stuff like
> >     
> >     make FOO BAR
> >     
> >     (i.e. not A=B but just A B)
> 
> Alexandre Courbot wrote:
>     These are variables, not no targets - their order do not matter, as they are just going to prevent same-name variables from being defined in the Makefile.
>     
>     http://ftp.gnu.org/old-gnu/Manuals/make-3.79.1/html_chapter/make_9.html#SEC90
>     
>     And as the same document says, the use of '=' is what differentiates a variable from a target in the command line. In your last example, FOO and BAR are targets, not variables. The order of targets does matter, but they already use a QStringList.
>     
>     So apart from a cosmetic point of view (QMap<QString, QString> might not be so elegant to have around, maybe a typedef would be better?), it seems to me that QMap is totally adequate here.
> 
> Andreas Pakulat wrote:
>     I agree with Alexandre, a quick test shows that variables on the commandline cannot reference each other so the order is completely irrelevant. In such a case a QMap is ok I believe and makes it easier to handle addition/removal. It should also be fast enough since there are not many variables set usually.

well, then this is OK with me


> On April 13, 2012, 9:27 p.m., Milian Wolff wrote:
> > projectbuilders/makebuilder/makejob.cpp, line 236
> > <http://git.reviewboard.kde.org/r/104537/diff/2/?file=56294#file56294line236>
> >
> >     if you really leave it at a qmap (see issue above), please use iterators instead of foreach:
> >     
> >     it = m_variables.constBegin();
> >     while(it != m_variables.constEnd()) {
> >       //  it.key() , it.value()
> >       ++it;
> >     }
> 
> Alexandre Courbot wrote:
>     Fine by me. Please just let me know whether you changed you mind about QMaps here. :)
>     
>     Thanks for the review.
> 
> Andreas Pakulat wrote:
>     Hmm, I've been out of the loop for a while, whats the problem with the foreach from Qt? (I realize a C++11 foreach is not wanted)

apaku: foreach is fine when you just want to iterate over values, but it will not give you the keys. actually, that foreach should not work at all! if you need key + value, use iterators.


- Milian


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


On April 11, 2012, 2:13 a.m., Alexandre Courbot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104537/
> -----------------------------------------------------------
> 
> (Updated April 11, 2012, 2:13 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> This change augments the IMakeBuilder interface and MakeBuilder class to let them support the following:
> 
> 1) Let make be run with multiple targets to build in one run
> 2) Pass build variables as a QMap of (variable, value) pairs that are also passed to make's command line.
> 
> E.g. this change now makes it possible for the make builder to perform make invokations that look like the following (example taken from an actual Linux kernel build):
> 
> $ make ARCH=arm CROSS_COMPILE=/usr/bin/arm-elf- vmlinux modules
> 
> API compatibility is not broken, but ABI is as the former virtual method of IMakeBuilder is now an inline function.
> 
> 
> Diffs
> -----
> 
>   projectbuilders/makebuilder/imakebuilder.h 56735425d78551883f109e942145eba2aa982687 
>   projectbuilders/makebuilder/makebuilder.h 34881c6eaee775b6b8b53959dfcf825732e806da 
>   projectbuilders/makebuilder/makebuilder.cpp 6c6905db30f469958f4a0048826febea29bad15a 
>   projectbuilders/makebuilder/makejob.h 19032fdf371da793d52b3457e5aa78a6b8458150 
>   projectbuilders/makebuilder/makejob.cpp ad5636dbfdadf3ae18ad1cc5b8dff078dd34cd42 
> 
> Diff: http://git.reviewboard.kde.org/r/104537/diff/
> 
> 
> Testing
> -------
> 
> Ensured API remained compatible and prior API behaved identically, tested build variables with the kdev-kernel plugin that uses them.
> 
> 
> Thanks,
> 
> Alexandre Courbot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120414/aaf275ec/attachment.html>