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

[Alexandre Courbot]

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

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.


> On April 13, 2012, 9:27 p.m., Milian Wolff wrote:
> > projectbuilders/makebuilder/imakebuilder.h, line 39
> > <http://git.reviewboard.kde.org/r/104537/diff/2/?file=56290#file56290line39>
> >
> >     both versions of these functions should get a doxygen comment, could you add that, describing the new functionality please?

Sure.


> On April 13, 2012, 9:27 p.m., Milian Wolff wrote:
> > projectbuilders/makebuilder/imakebuilder.h, line 42
> > <http://git.reviewboard.kde.org/r/104537/diff/2/?file=56290#file56290line42>
> >
> >     please don't inline this (it's virtual anyways), put it into the .cpp file

There is no cpp file for IMakeBuilder yet, but ok.


> On April 13, 2012, 9:27 p.m., Milian Wolff wrote:
> > projectbuilders/makebuilder/imakebuilder.h, line 49
> > <http://git.reviewboard.kde.org/r/104537/diff/2/?file=56290#file56290line49>
> >
> >     a comment here would be awesome as well please

Makes sense.


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

Fine by me. Please just let me know whether you changed you mind about QMaps here. :)

Thanks for the review.


- Alexandre


-----------------------------------------------------------
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/8ee25ef7/attachment.html>