[PATCH] Zorg: getLLDBuildFactory() now takes an optional 'env' parameter

Mikael Lyngvig mikael at lyngvig.org
Mon Dec 9 16:14:17 PST 2013

Yes, definitely so.

Then it must be because the previous code only overrode the CC and CXX
variables in the "make configure" step.  Of course, how silly of me...
 Your definitions are used by the "make" step, because the previous code
did not specify CC and CXX there.  I think the former code was incorrect
and that the way it is done now is much more clean, albeit perhaps not
optimal in terms of retaining the backwards compatibility that I sought.
 In other words, you were sort of lucky that it worked before with ccache -
because the getLLDBuildFactory() function did not specify its own
environment all the places it should, but only the first place.  Strictly
speaking, you could see this scenario with the old code:

   CC=gcc CXX=C++ cmake ...
   CC=clang CXX=clang++ make ...

I think the new way is the proper way.  Because previously you could not
control CC and CXX in the "make configure" step but only in the "make"
step.  Now you can control them in every step of the build factory function.

I think the best way to fix this is that you add your desired environment
variables to the call to getLLDBuildFactory() in
zorg/buildbot/builders/LLDBuilder.py.  Sorry for the inconvenience, but I
didn't realize that the former code basically was written so unfortunately
that it had to break when somebody came along and tried to change it.

If there's consensus that we should keep the old, slightly broken method,
I'll of course change it back, but I cannot see any sense in doing that.

-- Mikael

2013/12/10 Dmitri Gribenko <gribozavr at gmail.com>

> On Mon, Dec 9, 2013 at 11:34 AM, Mikael Lyngvig <mikael at lyngvig.org>
> wrote:
> > The patch has been updated and I am re-submitting everything I've
> submitted
> > to Galina as well so that it goes through a full peer review.
> >
> > Phew, thank you a ton for catching that!
> >
> > As far as the CC and CXX environment variables goes, I did it to retain
> > backwards compatibility with the existing three LLD builders:
> >
> > -                               env={
> > -                                    'CXX': "clang++",
> > -                                    'C':   "clang"},
> > +                               env=merged_env,
> >
> > So they already got overriden, I have simply made the override
> overriddable
> > :-)  Now you can specify that you want to use ccache in the env argument,
> > something you could not do before.  I am also going to use ccache.  I bet
> > your lld builds haven't been using ccache, even if you thought they did.
> I think they are using ccache, please take a look:
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-ubuntu-13.04/builds/10304/steps/cmake-configure/logs/stdio
> Dmitri
> --
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131210/87fc2081/attachment.html>

More information about the llvm-commits mailing list