scripts/slave/recipes/ios/try.py - Issue 1410743010: Rework iOS recipes to use latest MB approach

Side by Side Diff: scripts/slave/recipes/ios/try.py

Issue 1410743010: Rework iOS recipes to use latest MB approach (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/tools/build

Patch Set: Created 5 years, 1 month ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

« scripts/slave/recipe_modules/ios/api.py ('K') | « scripts/slave/recipe_modules/ios/example.expected/clobber.json ('k') | scripts/slave/recipes/ios/try.expected/basic.json » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 # Copyright 2015 The Chromium Authors. All rights reserved.	1 # Copyright 2015 The Chromium Authors. All rights reserved.

2 # Use of this source code is governed by a BSD-style license that can be	2 # Use of this source code is governed by a BSD-style license that can be

3 # found in the LICENSE file.	3 # found in the LICENSE file.

4	4

5 DEPS = [	5 DEPS = [

6 'gclient',	6 'gclient',

7 'ios',	7 'ios',

8 'json',	8 'json',

9 'platform',	9 'platform',

10 'properties',	10 'properties',

(...skipping 191 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
202 'compiler': 'ninja',	202 'compiler': 'ninja',

203 'configuration': 'Debug',	203 'configuration': 'Debug',

204 'sdk': 'iphonesimulator8.0',	204 'sdk': 'iphonesimulator8.0',

205 'tests': [	205 'tests': [

206 {	206 {

207 'app': 'fake tests',	207 'app': 'fake tests',

208 'device type': 'fake device',	208 'device type': 'fake device',

209 'os': '8.1',	209 'os': '8.1',

210 },	210 },

211 ],	211 ],

212 'use_mb': True,	212 'mb_type': 'gn',

	213 'gn_args': 'target_os="ios" ios_enable_code_signing=false',
	smut 2015/11/10 22:01:18 Can this be a dict like gyp_defines? Can this be a dict like gyp_defines? Dirk Pranke 2015/11/10 22:34:15 I actually don't think modeling gyp_defines as a d Show quoted text On 2015/11/10 22:01:18, smut wrote: > Can this be a dict like gyp_defines? I actually don't think modeling gyp_defines as a dict is a good pattern to follow. It's not how GYP (or GN) actually gets them, and modeling them as either Python dicts or JSON dicts introduces type and quoting uncertainties that you have to write code to account for. In particular, GN uses C++-style syntax, not python syntax, so you have to use "true" instead of "True" and '"ios"' instead of 'ios'. You also end up potentially hiding (or duplicating) the work that GN and GYP will do automatically to de-dup arguments. But, if I can't convince you that treating these things as strings instead of dicts is the right way to go, we should probably make the gn args a dict. smut 2015/11/10 23:25:34 It's hard to parse the diff when someone updates o Show quoted text On 2015/11/10 22:34:15, Dirk Pranke wrote: > On 2015/11/10 22:01:18, smut wrote: > > Can this be a dict like gyp_defines? > > I actually don't think modeling gyp_defines as a dict is a good pattern to > follow. > > It's not how GYP (or GN) actually gets them, and modeling them as either Python > dicts or JSON dicts introduces type and quoting uncertainties that you have to > write code to account for. In particular, GN uses C++-style syntax, not python > syntax, so you have to use "true" instead of "True" and '"ios"' instead of > 'ios'. > > You also end up potentially hiding (or duplicating) the work that GN and GYP > will do automatically to de-dup arguments. > > But, if I can't convince you that treating these things as strings instead of > dicts is the right way to go, we should probably make the gn args a dict. It's hard to parse the diff when someone updates one really long one-line string with another. I prefer these values to be enumerated alphabetically so the diff is really easy to see. That's really where my choice of dict came in. I didn't realize at the time that it could be problematic. How about a list of strings? gn_args = [ 'ios_enable_code_signing=false', 'target_os="ios"', ] Just to ease legibility of the config itself and especially diffs in CLs. Dirk Pranke 2015/11/10 23:59:37 True, that's a good point. GN actually takes the Show quoted text On 2015/11/10 23:25:34, smut wrote: > It's hard to parse the diff when someone updates one really long one-line string > with another. True, that's a good point. GN actually takes the space-separated string of args and writes them to a newline-separated list of args in out/Release/args.gn . So there's some precedent that suggests that multi-line is good, though we'd have to rework MB to take a multiline string if we actually wanted to pass it through as is (or write the args.gn file directly and tell MB to just use that). I will think about this some more. smut 2015/11/11 00:08:09 Just add ' '.join(self.__config['gn_args']) in ios Show quoted text On 2015/11/10 23:59:37, Dirk Pranke wrote: > On 2015/11/10 23:25:34, smut wrote: > > It's hard to parse the diff when someone updates one really long one-line > string > > with another. > > True, that's a good point. > > GN actually takes the space-separated string of args and writes them to a > newline-separated list of args in out/Release/args.gn . > > So there's some precedent that suggests that multi-line is good, though we'd > have to rework MB to take a multiline string if we actually wanted to pass it > through as is (or write the args.gn file directly and tell MB to just use that). > > I will think about this some more. Just add ' '.join(self.__config['gn_args']) in ios/api.py for minimal work. It already does that for gyp defines.
213 })	214 })

214 )	215 )

215	216

216	217

OLD	NEW