setup the correct version of Xcode

setup the correct version of Xcode

BUG=

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

[yjbanov, 2016-03-04 04:19:42]

yjbanov@google.com changed reviewers:
+ abarth@chromium.org, eseidel@chromium.org

[eseidel1, 2016-03-04 04:25:05]

eseidel@google.com changed reviewers:
+ eseidel@google.com, iannucci@chromium.org, smut@google.com

[eseidel1, 2016-03-04 04:25:06]

I think we're going to need to pull out some of the big guns (actual infra
engineers) to review this.

https://codereview.chromium.org/1768473002/diff/1/scripts/slave/recipes/flutt...
File scripts/slave/recipes/flutter/flutter.py (right):

https://codereview.chromium.org/1768473002/diff/1/scripts/slave/recipes/flutt...
scripts/slave/recipes/flutter/flutter.py:147: scripts_dir,
I think this is api.path['build'].join('scripts'), but I'd have to look at other
recipes to be sure.

https://codereview.chromium.org/1768473002/diff/1/scripts/slave/recipes/flutt...
scripts/slave/recipes/flutter/flutter.py:217: SetupXcode(api)
Do you need to take the results of finding XCode to set the DEVELOPER_DIR env
variable for xcrun?

[eseidel1, 2016-03-04 04:25:38]

Also, you'll need to run recipes.py train_simulation and include the expected
files before this could actually land.

[yjbanov, 2016-03-04 17:45:51]

https://codereview.chromium.org/1768473002/diff/1/scripts/slave/recipes/flutt...
File scripts/slave/recipes/flutter/flutter.py (right):

https://codereview.chromium.org/1768473002/diff/1/scripts/slave/recipes/flutt...
scripts/slave/recipes/flutter/flutter.py:147: scripts_dir,
On 2016/03/04 04:25:05, eseidel1 wrote:
> I think this is api.path['build'].join('scripts'), but I'd have to look at
other
> recipes to be sure.

I tried that, but for some reason api.path['build'].join('foo') takes me to
'build/scripts/build/foo', and if I do api.path['foo'] it says it doesn't know
what 'foo' is.

https://codereview.chromium.org/1768473002/diff/1/scripts/slave/recipes/flutt...
scripts/slave/recipes/flutter/flutter.py:217: SetupXcode(api)
On 2016/03/04 04:25:06, eseidel1 wrote:
> Do you need to take the results of finding XCode to set the DEVELOPER_DIR env
> variable for xcrun?

The `find_xcode.py` utility uses `xcode-select` to switch xcrun to the correct
version. We shouldn't need extra env variables.

[yjbanov, 2016-03-04 17:47:38]

On 2016/03/04 04:25:38, eseidel1 wrote:
> Also, you'll need to run recipes.py train_simulation and include the expected
> files before this could actually land.

There are no changes in the trained files because I'm not using `api.step` to
talk to `find_xcode.py`.

[iannucci, 2016-03-04 17:58:05]

https://chromiumcodereview.appspot.com/1768473002/diff/1/scripts/slave/recipe...
File scripts/slave/recipes/flutter/flutter.py (right):

https://chromiumcodereview.appspot.com/1768473002/diff/1/scripts/slave/recipe...
scripts/slave/recipes/flutter/flutter.py:141: import tempfile
Whoops. This not lgtm for sure. This will break the recipes system entirely :).

https://chromiumcodereview.appspot.com/1768473002/diff/1/scripts/slave/recipe...
scripts/slave/recipes/flutter/flutter.py:147: scripts_dir,
On 2016/03/04 at 17:45:50, yjbanov wrote:
> On 2016/03/04 04:25:05, eseidel1 wrote:
> > I think this is api.path['build'].join('scripts'), but I'd have to look at
other
> > recipes to be sure.
> 
> I tried that, but for some reason api.path['build'].join('foo') takes me to
'build/scripts/build/foo', and if I do api.path['foo'] it says it doesn't know
what 'foo' is.

This should be rewritten to use the recipe apis to invoke this script.
api.path['build'] should be what you want... if it's not then there's a bug in
the recipe engine (but I think a lot of things would break in that case).

[iannucci, 2016-03-04 18:01:20]

(for more context, the way it's written right now will actually run the xcode
select script for real every time you run the tests)

[yjbanov, 2016-03-05 01:43:09]

On 2016/03/04 17:58:05, iannucci wrote:
>
https://chromiumcodereview.appspot.com/1768473002/diff/1/scripts/slave/recipe...
> File scripts/slave/recipes/flutter/flutter.py (right):
> 
>
https://chromiumcodereview.appspot.com/1768473002/diff/1/scripts/slave/recipe...
> scripts/slave/recipes/flutter/flutter.py:141: import tempfile
> Whoops. This not lgtm for sure. This will break the recipes system entirely
:).
> 
>
https://chromiumcodereview.appspot.com/1768473002/diff/1/scripts/slave/recipe...
> scripts/slave/recipes/flutter/flutter.py:147: scripts_dir,
> On 2016/03/04 at 17:45:50, yjbanov wrote:
> > On 2016/03/04 04:25:05, eseidel1 wrote:
> > > I think this is api.path['build'].join('scripts'), but I'd have to look at
> other
> > > recipes to be sure.
> > 
> > I tried that, but for some reason api.path['build'].join('foo') takes me to
> 'build/scripts/build/foo', and if I do api.path['foo'] it says it doesn't know
> what 'foo' is.
> 
> This should be rewritten to use the recipe apis to invoke this script.
> api.path['build'] should be what you want... if it's not then there's a bug in
> the recipe engine (but I think a lot of things would break in that case).

Done

[yjbanov, 2016-03-05 01:43:34]

Refactored to use api.python.

PTAL

[smut, 2016-03-05 02:03:47]

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
File scripts/slave/recipes/flutter/flutter.py (right):

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
scripts/slave/recipes/flutter/flutter.py:135: of Xcode.
"""<One liner>

<Additional info>
"""

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
scripts/slave/recipes/flutter/flutter.py:140: args.extend(['--version',
'7.2.1'])
I'm not sure this is what you intended. If target_version is None you omit it,
if it's not None you always set it to 7.2.1. Why not just "if target_version:
args.extend(['--version', target_version])"?

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
scripts/slave/recipes/flutter/flutter.py:157: if activate_version is None:
if not activate_version:

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
scripts/slave/recipes/flutter/flutter.py:158: raise Exception('Xcode version 7
or above not found')
raise api.step.StepFailure(...) or api.step.InfraFailure(...).

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
scripts/slave/recipes/flutter/flutter.py:159: print 'Activating Xcode version
%s' % version
Don't print outside a step. You can just delete this, the step created on line
160 should make it clear what's happening.

[yjbanov, 2016-03-06 22:53:42]

PTAL

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
File scripts/slave/recipes/flutter/flutter.py (right):

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
scripts/slave/recipes/flutter/flutter.py:135: of Xcode.
On 2016/03/05 02:03:47, smut wrote:
> """<One liner>
> 
> <Additional info>
> """

Done.

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
scripts/slave/recipes/flutter/flutter.py:140: args.extend(['--version',
'7.2.1'])
On 2016/03/05 02:03:47, smut wrote:
> I'm not sure this is what you intended. If target_version is None you omit it,
> if it's not None you always set it to 7.2.1. Why not just "if target_version:
> args.extend(['--version', target_version])"?

Oops. Left-over. Fixed.

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
scripts/slave/recipes/flutter/flutter.py:157: if activate_version is None:
On 2016/03/05 02:03:47, smut wrote:
> if not activate_version:

Done.

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
scripts/slave/recipes/flutter/flutter.py:158: raise Exception('Xcode version 7
or above not found')
On 2016/03/05 02:03:47, smut wrote:
> raise api.step.StepFailure(...) or api.step.InfraFailure(...).

Done.

https://codereview.chromium.org/1768473002/diff/20001/scripts/slave/recipes/f...
scripts/slave/recipes/flutter/flutter.py:159: print 'Activating Xcode version
%s' % version
On 2016/03/05 02:03:47, smut wrote:
> Don't print outside a step. You can just delete this, the step created on line
> 160 should make it clear what's happening.

Done.

[iannucci, 2016-03-07 07:30:21]

fwiw, this lgtm. Not sure if smut@ had more comments?

https://chromiumcodereview.appspot.com/1768473002/diff/40001/scripts/slave/re...
File scripts/slave/recipes/flutter/flutter.py (right):

https://chromiumcodereview.appspot.com/1768473002/diff/40001/scripts/slave/re...
scripts/slave/recipes/flutter/flutter.py:224: test = (test +
nit: I think `test += ` will do the right thing

[yjbanov, 2016-03-07 17:47:07]

https://chromiumcodereview.appspot.com/1768473002/diff/40001/scripts/slave/re...
File scripts/slave/recipes/flutter/flutter.py (right):

https://chromiumcodereview.appspot.com/1768473002/diff/40001/scripts/slave/re...
scripts/slave/recipes/flutter/flutter.py:224: test = (test +
On 2016/03/07 07:30:21, iannucci wrote:
> nit: I think `test += ` will do the right thing

Done.

[yjbanov, 2016-03-07 20:17:29]

The CQ bit was checked by yjbanov@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-07 20:17:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1768473002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1768473002/60001

[commit-bot: I haz the power, 2016-03-07 20:24:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-07 20:24:49]

Dry run: Try jobs failed on following builders:
  build_presubmit on chromium.buildbucket.swarming (JOB_FAILED,
https://annotation-parser.appspot.com/swarming/build/2d6b8c1fa1c0aa10)
  Build Presubmit on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)

[yjbanov, 2016-03-07 23:09:13]

The CQ bit was checked by yjbanov@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-07 23:09:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1768473002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1768473002/80001

[commit-bot: I haz the power, 2016-03-07 23:10:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-07 23:10:29]

Dry run: Try jobs failed on following builders:
  build_presubmit on chromium.buildbucket.swarming (JOB_FAILED,
https://annotation-parser.appspot.com/swarming/build/2d6c2950df678010)
  Build Presubmit on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)
  Build Try Recipe Test Trusty64 on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Try%20Recipe%20...)

[iannucci, 2016-03-07 23:21:12]

lgtm

[smut, 2016-03-07 23:26:15]

lgtm

https://codereview.chromium.org/1768473002/diff/80001/scripts/slave/recipes/f...
File scripts/slave/recipes/flutter/flutter.py (right):

https://codereview.chromium.org/1768473002/diff/80001/scripts/slave/recipes/f...
scripts/slave/recipes/flutter/flutter.py:160: RunFindXcode(api,
'set_xcode_version', target_version=activate_version)
You might want to consider setting a particular Xcode version instead of just
7.<whatever's found>, but it's up to you.

[yjbanov, 2016-03-07 23:27:53]

> You might want to consider setting a particular Xcode version instead of just
> 7.<whatever's found>, but it's up to you.

Is there a guarantee that a particular version of Xcode down to the patch
version number will be available on the build bots?

[smut, 2016-03-07 23:30:01]

On 2016/03/07 23:27:53, yjbanov wrote:
> > You might want to consider setting a particular Xcode version instead of
just
> > 7.<whatever's found>, but it's up to you.
> 
> Is there a guarantee that a particular version of Xcode down to the patch
> version number will be available on the build bots?

On my bots there is (we have 7.0). How are yours configured?

[iannucci, 2016-03-07 23:41:07]

iannucci@chromium.org changed reviewers:
+ dba@chromium.org

[iannucci, 2016-03-07 23:41:08]

+dba who knows things about xcode

[yjbanov, 2016-03-07 23:43:04]

The CQ bit was checked by yjbanov@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-07 23:43:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1768473002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1768473002/100001

[commit-bot: I haz the power, 2016-03-07 23:47:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-07 23:47:21]

Dry run: This issue passed the CQ dry run.

[yjbanov, 2016-03-07 23:54:11]

The CQ bit was checked by yjbanov@google.com

[yjbanov, 2016-03-07 23:54:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org, smut@google.com
Link to the patchset:
https://chromiumcodereview.appspot.com/1768473002/#ps100001 (title: "remove
spaces")

[commit-bot: I haz the power, 2016-03-07 23:54:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1768473002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1768473002/100001

[commit-bot: I haz the power, 2016-03-07 23:57:57]

Description was changed from

==========
setup the correct version of Xcode

BUG=
==========

to

==========
setup the correct version of Xcode

BUG=

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

[commit-bot: I haz the power, 2016-03-07 23:57:58]

Committed patchset #6 (id:100001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=299136