Allow using the same checkout to build both iOS and macOS.

Allow using the same checkout to build both iOS and macOS.

As it is possible to specify multiple value for target_os in
.gclient, update the script build/mac_toolchain.py to download
the file for the hermetic build on all those OSes. Change the
path where the hermetic files are downloaded to use different
path for each OS.

BUG=680069
Review-Url: https://codereview.chromium.org/2626063002
Cr-Commit-Position: refs/heads/master@{#443198}
Committed: https://chromium.googlesource.com/chromium/src/+/aacc2845873563b3361c0c9b2a7bec6ead4cd155

[justincohen, 2017-01-11 14:25:22]

justincohen@chromium.org changed reviewers:
+ justincohen@chromium.org

[justincohen, 2017-01-11 14:25:22]

https://codereview.chromium.org/2626063002/diff/1/build/mac_toolchain.py
File build/mac_toolchain.py (right):

https://codereview.chromium.org/2626063002/diff/1/build/mac_toolchain.py#newc...
build/mac_toolchain.py:238: print 'Using local toolchain for %s.' % target_os
no return value here

https://codereview.chromium.org/2626063002/diff/1/build/mac_toolchain.py#newc...
build/mac_toolchain.py:240: return 1
don't you want to return the result of DownloadHermeticBuild?

[sdefresne, 2017-01-11 14:33:06]

Description was changed from

==========
Allow using the same checkout to build both iOS and macOS.

BUG=680069
==========

to

==========
Allow using the same checkout to build both iOS and macOS.

As it is possible to specify multiple value for target_os in
.gclient, update the script build/mac_toolchain.py to download
the file for the hermetic build on all those OSes. Change the
path where the hermetic files are downloaded to use different
path for each OS.

BUG=680069
==========

[sdefresne, 2017-01-11 14:34:15]

Thank you for the review. Please take a look.

https://codereview.chromium.org/2626063002/diff/1/build/mac_toolchain.py
File build/mac_toolchain.py (right):

https://codereview.chromium.org/2626063002/diff/1/build/mac_toolchain.py#newc...
build/mac_toolchain.py:238: print 'Using local toolchain for %s.' % target_os
On 2017/01/11 14:25:22, justincohen wrote:
> no return value here

Yes, we want to continue iterating on the next target_os returned by
GetPlatforms(). I've moved the test inside DownloadHermeticBuild() to make it
clearer that this is not a condition that should stop the full script, just stop
downloading those particular files.

https://codereview.chromium.org/2626063002/diff/1/build/mac_toolchain.py#newc...
build/mac_toolchain.py:240: return 1
On 2017/01/11 14:25:22, justincohen wrote:
> don't you want to return the result of DownloadHermeticBuild?

Done.

[justincohen, 2017-01-11 14:40:57]

LGTM with return

https://codereview.chromium.org/2626063002/diff/20001/build/mac_toolchain.py
File build/mac_toolchain.py (right):

https://codereview.chromium.org/2626063002/diff/20001/build/mac_toolchain.py#...
build/mac_toolchain.py:244: return_value
return return return value value value.

[sdefresne, 2017-01-11 14:44:44]

https://codereview.chromium.org/2626063002/diff/20001/build/mac_toolchain.py
File build/mac_toolchain.py (right):

https://codereview.chromium.org/2626063002/diff/20001/build/mac_toolchain.py#...
build/mac_toolchain.py:244: return_value
On 2017/01/11 14:40:57, justincohen wrote:
> return return return value value value.

My brain was convinced there were enough returns there! Thank you for catching
this!

[sdefresne, 2017-01-11 14:45:41]

sdefresne@chromium.org changed reviewers:
+ dpranke@chromium.org

[sdefresne, 2017-01-11 14:45:42]

dpranke: can you give OWNERS approval for build/toolchain/toolchain.gni?

[sdefresne, 2017-01-11 14:45:50]

The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 14:46:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dirk Pranke, 2017-01-12 02:13:30]

lgtm!

https://codereview.chromium.org/2626063002/diff/60001/build/mac_toolchain.py
File build/mac_toolchain.py (right):

https://codereview.chromium.org/2626063002/diff/60001/build/mac_toolchain.py#...
build/mac_toolchain.py:235: if target_os == "ios":
Nit: 'ios', not "ios".

[sdefresne, 2017-01-12 08:45:03]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2017-01-12 08:45:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
justincohen@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2626063002/#ps80001
(title: "Address nitpick.")

[commit-bot: I haz the power, 2017-01-12 08:45:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sdefresne, 2017-01-12 08:45:30]

Thank you.

https://codereview.chromium.org/2626063002/diff/60001/build/mac_toolchain.py
File build/mac_toolchain.py (right):

https://codereview.chromium.org/2626063002/diff/60001/build/mac_toolchain.py#...
build/mac_toolchain.py:235: if target_os == "ios":
On 2017/01/12 02:13:30, Dirk Pranke wrote:
> Nit: 'ios', not "ios".

Done.

[commit-bot: I haz the power, 2017-01-12 10:33:41]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484210703254210,
"parent_rev": "3d34fb59dd2f1c081ef7f6a630c794dc073b37df", "commit_rev":
"aacc2845873563b3361c0c9b2a7bec6ead4cd155"}

[commit-bot: I haz the power, 2017-01-12 10:34:12]

Description was changed from

==========
Allow using the same checkout to build both iOS and macOS.

As it is possible to specify multiple value for target_os in
.gclient, update the script build/mac_toolchain.py to download
the file for the hermetic build on all those OSes. Change the
path where the hermetic files are downloaded to use different
path for each OS.

BUG=680069
==========

to

==========
Allow using the same checkout to build both iOS and macOS.

As it is possible to specify multiple value for target_os in
.gclient, update the script build/mac_toolchain.py to download
the file for the hermetic build on all those OSes. Change the
path where the hermetic files are downloaded to use different
path for each OS.

BUG=680069

Review-Url: https://codereview.chromium.org/2626063002
Cr-Commit-Position: refs/heads/master@{#443198}
Committed:
https://chromium.googlesource.com/chromium/src/+/aacc2845873563b3361c0c9b2a7b...
==========

[commit-bot: I haz the power, 2017-01-12 10:34:13]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/aacc2845873563b3361c0c9b2a7b...