android envsetup: Stop honoring --target-arch parameter.

tools/cr/cr/base/android.py

 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """The android specific platform implementation module."""
 
 import os
 import subprocess
 
 import cr
@@ skipping 41 matching lines @@
     return super(AndroidPlatform, self).priority + 1
 
   def Prepare(self, context):
     """Override Prepare from cr.Platform."""
     super(AndroidPlatform, self).Prepare(context)
     try:
       # capture the result of env setup if we have not already done so
       if not self._env_ready:
         # See what the env would be without env setup
         before = context.exported
-        # Run env setup and capture/parse it's output
-        envsetup = 'source {CR_ENVSETUP} --target-arch={CR_ENVSETUP_ARCH}'
+        before['GYP_DEFINES'] = before.get(
+            'GYP_DEFINES', '') + ' target_arch={CR_ENVSETUP_ARCH}'

[iancottrell, 2014/02/19 09:58:21]

This feels like the wrong place to do this.
Is there any reason this needs to be android specific?
It might be better to have arch directly modify GYP_DEFINES for all platforms.
GYP_DEFINES in general has some problems, mostly because ramming a map of
settings into a single string is a vile and horrible practice to start with :) 
I have considered adding more magic to help with it, for instance auto merging
all variables that start with a GYP_SET_ prefix or something.
This would allow arch to have GYP_SET_target_arch set on it, and it would just
work. It would also make it easy to override and view all the other gyp options
on a per output basis.
If it is going to stay in this file, it should probably move down to the
GYP_DEFINES specific modification part below this.

[Nico, 2014/02/19 14:04:06]

Yes: envsetup is android-specific, and I'm changing how envsetup works.
You can pass -D for every single define to gyp instead if you think that's
nicer.
Which part? The `if key == 'GYP_DEFINES'` bit? That only runs if envsetup
changes GYP_DEFINES, right? It'll stop doing that soon, so that line will be
dead soon.

(I have no opinion how the cr bit should work. Tell me what to do, and I'll
gladly do it.)

[iancottrell, 2014/02/19 14:18:24]

I mean that setting the arch for gyp does not need to be specific to android, it
would be cleaner for it not to be in this file at all.
I am happy for you to commit it here and file a bug against me to make it more
general though.
Except the code you are adding is faking envsetup modifying GYP_DEFINES :)
The reason it would be better down there is that up here it is modifying
context.exported which is not safe, and will also get dropped if the line below
is removed
Down there, it would get added to the new override added to context.

+        # Run env setup and capture/parse its output
+        envsetup = 'source {CR_ENVSETUP}'
         output = cr.Host.CaptureShell(context, envsetup + ' > /dev/null && env')
         env_setup = cr.Config('envsetup', literal=True, export=True)
         for line in output.split('\n'):
           (key, op, value) = line.partition('=')
           if op:
             key = key.strip()
             if key not in _IGNORE_ENV:
               env_setup[key] = env_setup.ParseValue(value.strip())
             if key == 'PATH':
               self._env_paths = value.strip().split(os.path.pathsep)
@@ skipping 50 matching lines @@
       print 'Abandoning the creation of and android output directory.'
       exit(1)
     target_os.append('android')
     context.gclient['target_os'] = target_os
     context.WriteGClient()
     print 'Client updated.'
     print 'You may need to sync before an output directory can be made.'
     if cr.Host.YesNo('Would you like to sync this client now?'):
       cr.SyncCommand.Sync(context, ["--nohooks"])