Also detect the CXX_target enviroment vairiable for compiler version

Also detect the CXX_target enviroment vairiable for compiler version

The ninja's cross compile mode still uses CXX_target as the target compiler
which needs to be detected before the host one.

This CL might be reverted once the ninja use CXX as target compiler.

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150300

[Evan Martin, 2012-07-30 19:13:49]

If this is a gyp bug, why not fix gyp?


On Mon, Jul 30, 2012 at 12:07 PM, <michaelbai@chromium.org> wrote:

> Reviewers: Evan Martin, Yaron,
>
> Description:
> Also detect the CXX_target enviroment vairiable for compiler version
>
> The ninja's cross compile mode still uses CXX_target as the target compiler
> which needs to be detected before the host one.
>
> This CL might be reverted once the ninja use CXX as target compiler.
>
> BUG=
>
>
> Please review this at
http://codereview.chromium.**org/10837005/<http://codereview.chromium.org/108...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M build/compiler_version.py
>
>
> Index: build/compiler_version.py
> diff --git a/build/compiler_version.py b/build/compiler_version.py
> index b349199992caeefc5c63b227083a98**a5400fda3f..**
> edfd76141d677782f41ea84a6ecd70**e7147fe394 100755
> --- a/build/compiler_version.py
> +++ b/build/compiler_version.py
> @@ -33,21 +33,40 @@ def GetVersion(compiler):
>      print >> sys.stderr, e
>      return ""
>
> -def main():
> -  # Check if CXX environment variable exists and
> -  # if it does use that compiler.
> -  cxx = os.getenv("CXX", None)
> +def GetVersionFromEnvironment(**compiler_env):
> +  """ Returns the version of compiler
> +
> +  If the compiler was set by the given environment variable and exists,
> +  return its version, otherwise None is returned.
> +  """
> +  cxx = os.getenv(compiler_env, None)
>    if cxx:
> -    cxxversion = GetVersion(cxx)
> -    if cxxversion != "":
> -      print cxxversion
> -      return 0
> -  else:
> -    # Otherwise we check the g++ version.
> -    gccversion = GetVersion("g++")
> -    if gccversion != "":
> -      print gccversion
> -      return 0
> +    cxx_version = GetVersion(cxx)
> +    if cxx_version != "":
> +      return cxx_version
> +  return None
> +
> +def main():
> +  # Check if CXX_target or CXX environment variable exists an if it does
> use
> +  # that compiler.
> +  # In ninja's cross compile mode, the CXX_target is target compiler,
> while
> +  # the CXX is host. The CXX_target needs be checked first, though the
> target
> +  # and host compiler have different version, there seems no issue in
> Android.
> +  cxx_version = GetVersionFromEnvironment("**CXX_target")
> +  if cxx_version:
> +    print cxx_version
> +    return 0
> +
> +  cxx_version = GetVersionFromEnvironment("**CXX")
> +  if cxx_version:
> +    print cxx_version
> +    return 0
> +
> +  # Otherwise we check the g++ version.
> +  gccversion = GetVersion("g++")
> +  if gccversion != "":
> +    print gccversion
> +    return 0
>
>    return 1
>
>
>
>

[michaelbai, 2012-07-30 19:21:29]

The gyp's fix is here https://chromiumcodereview.appspot.com/9649016/

From piman@, it seemed that it could break something, he might need a week's
babysister for that CL, but his is so busy now and doesn't have plan to do it.

I need to get the Android NDK r8b to work as soon as possible, so this CL is
kind of temporary solution.

On 2012/07/30 19:13:49, Evan Martin wrote:
> If this is a gyp bug, why not fix gyp?
> 
> 
> On Mon, Jul 30, 2012 at 12:07 PM, <mailto:michaelbai@chromium.org> wrote:
> 
> > Reviewers: Evan Martin, Yaron,
> >
> > Description:
> > Also detect the CXX_target enviroment vairiable for compiler version
> >
> > The ninja's cross compile mode still uses CXX_target as the target compiler
> > which needs to be detected before the host one.
> >
> > This CL might be reverted once the ninja use CXX as target compiler.
> >
> > BUG=
> >
> >
> > Please review this at
>
http://codereview.chromium.**org/10837005/%3Chttp://codereview.chromium.org/1...>
> >
> > SVN Base:
>
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
> >
> > Affected files:
> >   M build/compiler_version.py
> >
> >
> > Index: build/compiler_version.py
> > diff --git a/build/compiler_version.py b/build/compiler_version.py
> > index b349199992caeefc5c63b227083a98**a5400fda3f..**
> > edfd76141d677782f41ea84a6ecd70**e7147fe394 100755
> > --- a/build/compiler_version.py
> > +++ b/build/compiler_version.py
> > @@ -33,21 +33,40 @@ def GetVersion(compiler):
> >      print >> sys.stderr, e
> >      return ""
> >
> > -def main():
> > -  # Check if CXX environment variable exists and
> > -  # if it does use that compiler.
> > -  cxx = os.getenv("CXX", None)
> > +def GetVersionFromEnvironment(**compiler_env):
> > +  """ Returns the version of compiler
> > +
> > +  If the compiler was set by the given environment variable and exists,
> > +  return its version, otherwise None is returned.
> > +  """
> > +  cxx = os.getenv(compiler_env, None)
> >    if cxx:
> > -    cxxversion = GetVersion(cxx)
> > -    if cxxversion != "":
> > -      print cxxversion
> > -      return 0
> > -  else:
> > -    # Otherwise we check the g++ version.
> > -    gccversion = GetVersion("g++")
> > -    if gccversion != "":
> > -      print gccversion
> > -      return 0
> > +    cxx_version = GetVersion(cxx)
> > +    if cxx_version != "":
> > +      return cxx_version
> > +  return None
> > +
> > +def main():
> > +  # Check if CXX_target or CXX environment variable exists an if it does
> > use
> > +  # that compiler.
> > +  # In ninja's cross compile mode, the CXX_target is target compiler,
> > while
> > +  # the CXX is host. The CXX_target needs be checked first, though the
> > target
> > +  # and host compiler have different version, there seems no issue in
> > Android.
> > +  cxx_version = GetVersionFromEnvironment("**CXX_target")
> > +  if cxx_version:
> > +    print cxx_version
> > +    return 0
> > +
> > +  cxx_version = GetVersionFromEnvironment("**CXX")
> > +  if cxx_version:
> > +    print cxx_version
> > +    return 0
> > +
> > +  # Otherwise we check the g++ version.
> > +  gccversion = GetVersion("g++")
> > +  if gccversion != "":
> > +    print gccversion
> > +    return 0
> >
> >    return 1
> >
> >
> >
> >

[michaelbai, 2012-07-31 18:02:57]

Marc, as Evan didn't work on chromium any more, Would you like to help review
this.

[michaelbai, 2012-08-01 17:57:41]

Mark, could you help to review this

[Mark Mentovai, 2012-08-01 18:10:30]

Is ninja supposed to use CXX for the target compiler?

This script looks like it’s used to set gcc_version in build/common.gypi. In
turn, that affects flags that are passed to the compiler. Should there be a
distinction made between the host_gcc_version and target_gcc_version in the gyp
files?

http://codereview.chromium.org/10837005/diff/1/build/compiler_version.py
File build/compiler_version.py (right):

http://codereview.chromium.org/10837005/diff/1/build/compiler_version.py#newc...
build/compiler_version.py:54: # and host compiler have different version, there
seems no issue in Android.
What do you mean “seems no issue?” If there’s no issue, why do you need to check
both?

[Nico, 2012-08-01 18:15:30]

It sounds like some form of the gyp change should land instead of this.

[michaelbai, 2012-08-01 18:25:55]

Ideally, this CL  https://chromiumcodereview.appspot.com/9649016/ should be
landed, but piman@ don't have time. I could revert my CL after the above CL
landed.

As to host_gcc_version and target_gcc_version, As I tested, just using target
compiler version worked in Android.

http://codereview.chromium.org/10837005/diff/1/build/compiler_version.py
File build/compiler_version.py (right):

http://codereview.chromium.org/10837005/diff/1/build/compiler_version.py#newc...
build/compiler_version.py:54: # and host compiler have different version, there
seems no issue in Android.
I meant that there is no issue to use target compiler's version number as
gcc_version in Andorid, also revised the comment.

On 2012/08/01 18:10:30, Mark Mentovai wrote:
> What do you mean “seems no issue?” If there’s no issue, why do you need to
check
> both?

[Craig, 2012-08-02 05:47:02]

What is the actual problem you're trying to fix? I presume build_version.py is
causing gyp to blow up because build_version.py hasn't found and printed a
compiler version to stdout?

( only one rather obscure thing seems to rely on gcc_version at the moment so
perhaps printing a fake gcc_version of 0 instead of failing spectacularly might
mask this and other compiler/path issues without any real downside ... glider
hit trouble related to this recently too for example where the compiler path was
only being set after runhooks on one of the new bots )

[michaelbai, 2012-08-02 18:01:20]

Webkit use gcc_version to set different compiler flag, it needs specific gcc
version like 4.6, 0 or 1 won't work.

Where is the build_version.py? I didn't find that file.

On 2012/08/02 05:47:02, Craig wrote:
> What is the actual problem you're trying to fix? I presume build_version.py is
> causing gyp to blow up because build_version.py hasn't found and printed a
> compiler version to stdout?
> 
> ( only one rather obscure thing seems to rely on gcc_version at the moment so
> perhaps printing a fake gcc_version of 0 instead of failing spectacularly
might
> mask this and other compiler/path issues without any real downside ... glider
> hit trouble related to this recently too for example where the compiler path
was
> only being set after runhooks on one of the new bots )

[Craig, 2012-08-03 04:36:20]

On 2012/08/02 18:01:20, michaelbai wrote:
> Webkit use gcc_version to set different compiler flag, it needs specific gcc
> version like 4.6, 0 or 1 won't work.

Ah, OK!! I used git grep which skipped over my svn webkit checkout so I missed
that. Duh.

> Where is the build_version.py? I didn't find that file.

I meant build/compiler_version.py.

The change seems to be a reasonable short-term fix to me if the real fix lands
soon but more intelligent people than me are in favour of fixing the real issue
so I will defer to them for judgement...

[Nico, 2012-08-06 19:03:03]

As discussed in person, lgtm temporarily to unblock ndk updating for android,
but please get this resolved quickly.

(After filing the bug, add it to the BUG= line of your gyp change for this)

http://codereview.chromium.org/10837005/diff/2004/build/compiler_version.py
File build/compiler_version.py (right):

http://codereview.chromium.org/10837005/diff/2004/build/compiler_version.py#n...
build/compiler_version.py:51: # that compiler.
Please file a bug (on crbug.com) about ninja getting this wrong, and add a
"TODO: Fix ninja (see http://crbug.com/XXXX) instead and remove this code"
comment here

[michaelbai, 2012-08-06 20:50:35]

http://codereview.chromium.org/10837005/diff/2004/build/compiler_version.py
File build/compiler_version.py (right):

http://codereview.chromium.org/10837005/diff/2004/build/compiler_version.py#n...
build/compiler_version.py:51: # that compiler.
On 2012/08/06 19:03:03, Nico wrote:
> Please file a bug (on http://crbug.com) about ninja getting this wrong, and
add a
> "TODO: Fix ninja (see http://crbug.com/XXXX) instead and remove this code"
> comment here

Done.

[commit-bot: I haz the power, 2012-08-06 20:50:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/10837005/5003

[commit-bot: I haz the power, 2012-08-06 22:26:24]

Try job failure for 10837005-5003 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-06 22:45:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/10837005/10003

[commit-bot: I haz the power, 2012-08-06 23:16:50]

Try job failure for 10837005-10003 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-07 06:16:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/10837005/10003

[commit-bot: I haz the power, 2012-08-07 06:35:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/10837005/5006

[commit-bot: I haz the power, 2012-08-07 07:46:53]

Change committed as 150300