Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1411183010:
Make MB aware of iOS bot configs and get iOS working. (Closed)
|
Created:
5 years, 1 month ago by Dirk Pranke Modified:
5 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake MB aware of iOS bot configs and get iOS working.
With these changes, the ios trybots should start working.
Part of this change makes MB aware of the bot config files
in //ios/build/bots, so that we don't have to configure
the iOS bots in two different places.
See the updated MB docs for details.
R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org
BUG=517216
Committed: https://crrev.com/e0f486fd71e596a87189b91c4e825a356db8ab5e
Cr-Commit-Position: refs/heads/master@{#360688}
Patch Set 1 #Patch Set 2 : add "All" placeholder target #Patch Set 3 : All => testonly #Patch Set 4 : remove missing crw_cert file #Patch Set 5 : remove the unittest, too #Patch Set 6 : build fixes #Patch Set 7 : update MB defs to match bot gyp defines (static, no goma) #Patch Set 8 : add comments about "All" and "all" #
Total comments: 2
Patch Set 9 : add passthrough mode to MB, move MB configuration into //ios files #
Total comments: 6
Patch Set 10 : rework to read GYP_DEFINES/gn_args from bot configs in MB #
Total comments: 15
Patch Set 11 : fix typos, update w/ review feedback #Patch Set 12 : merge to #360460, clean up #
Messages
Total messages: 46 (18 generated)
Description was changed from ========== Add ios GN bot configs. R=smut@google.com, sdefresne@chromium.org BUG=517216 ========== to ========== Add ios GN bot configs. R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org BUG=517216 ==========
dpranke@chromium.org changed reviewers: + stuartmorgan@chromium.org
Description was changed from ========== Add ios GN bot configs. R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org BUG=517216 ========== to ========== Add iOS GN bot configs and get iOS GN tryservers working With these changes, the "gn_all" (and "All") targets should work in iOS trybots. No guarantees are made about other sets of gn args at this point, in particular I don't think component builds work, nor does 'dcheck_always_on=true', and I don't know how well goma works. I also don't expect that "all" will work, since it GN will currently pull in a bunch of targets that are almost certainly not yet conditionally excluded on iOS and probably broken. These things can all be fixed in followup patches. R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org BUG=517216 ==========
They're alive! Once this lands I'll post CLs for waterfall bots as well. I will also start adding these to the CQ once all of the new VMs are online.
Description was changed from ========== Add iOS GN bot configs and get iOS GN tryservers working With these changes, the "gn_all" (and "All") targets should work in iOS trybots. No guarantees are made about other sets of gn args at this point, in particular I don't think component builds work, nor does 'dcheck_always_on=true', and I don't know how well goma works. I also don't expect that "all" will work, since it GN will currently pull in a bunch of targets that are almost certainly not yet conditionally excluded on iOS and probably broken. These things can all be fixed in followup patches. R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org BUG=517216 ========== to ========== Add iOS GN bot configs and get iOS GN tryservers working With these changes, the "gn_all" (and "All") targets should work in iOS trybots. No guarantees are made about other sets of gn args at this point, in particular I don't think component builds work, nor does 'dcheck_always_on=true', and I don't know how well goma works. I also don't expect that "all" will work, since it GN will currently pull in a bunch of targets that are almost certainly not yet conditionally excluded on iOS and probably broken. These things can all be fixed in followup patches. R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org BUG=517216 CQ_EXTRA_TRYBOTS=tryserver.chromium.mac:ios_dbg_simulator_gn,ios_rel_device_gn ==========
The CQ bit was checked by dpranke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411183010/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411183010/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ios/web lgtm
https://codereview.chromium.org/1411183010/diff/140001/tools/mb/mb_config.pyl File tools/mb/mb_config.pyl (right): https://codereview.chromium.org/1411183010/diff/140001/tools/mb/mb_config.pyl... tools/mb/mb_config.pyl:251: 'gyp_defines': 'OS=ios target_subarch=both chromium_ios_signing=0' For iOS we've gone to a lot of trouble to allow user-defined gyp_defines in the bot config files themselves. Here we can only assume OS=ios. I would really prefer it if somehow target_subarch and chromium_ios_signing could get their values from the config files for the bots themselves. The .json files you added to this CL already specify values for these two gyp_defines, and it would be confusing if they were just ignored and these values were used instead. It's also not just these two, but arbitrary gyp_defines that need to continue to be supported in the .json config files. Downstream we use a lot more than just target_subarch and chromium_ios_signing, and they are supported in a unified way for xcodebuild and gyp+ninja. It would be better if mb/gn+ninja conformed.
I suspect we might want to discuss this face-to-face to figure out a design we're both okay with, but take a look at my response below and let me know what you think. https://codereview.chromium.org/1411183010/diff/140001/tools/mb/mb_config.pyl File tools/mb/mb_config.pyl (right): https://codereview.chromium.org/1411183010/diff/140001/tools/mb/mb_config.pyl... tools/mb/mb_config.pyl:251: 'gyp_defines': 'OS=ios target_subarch=both chromium_ios_signing=0' On 2015/11/09 20:48:04, smut wrote: > For iOS we've gone to a lot of trouble to allow user-defined gyp_defines in the > bot config files themselves. Yup, I understand and am very sympathetic. If all of the bots had done what you did, I arguably wouldn't have needed to write MB at all all (or at least, I would've needed it to do much less). But, unfortunately, none of the other bots can be configured via src-side config files. And, it's much more important to me that we have one mechanism for configuring what GYP_DEFINES and GN args we use. We will need to update the downstream bots to use MB as well, and we will need to update MB to support xcodebuild unless we kill off xcodebuild instead. Ideally we would delete the GYP_DEFINES and the configuration info for ninja vs. xcode altogether. I think in order to do that we would need to ensure that there are no gclient hooks that look at GYP defines, and we'd need to see if we can make your recipes look at the output from MB instead of the config file directly if the recipe needs to do something different based on xcodebuild vs. ninja.
Description was changed from ========== Add iOS GN bot configs and get iOS GN tryservers working With these changes, the "gn_all" (and "All") targets should work in iOS trybots. No guarantees are made about other sets of gn args at this point, in particular I don't think component builds work, nor does 'dcheck_always_on=true', and I don't know how well goma works. I also don't expect that "all" will work, since it GN will currently pull in a bunch of targets that are almost certainly not yet conditionally excluded on iOS and probably broken. These things can all be fixed in followup patches. R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org BUG=517216 CQ_EXTRA_TRYBOTS=tryserver.chromium.mac:ios_dbg_simulator_gn,ios_rel_device_gn ========== to ========== Add passthrough mode to MB and get iOS working. With these changes, the ios trybots should start working. Part of this change adds a 'passthrough' mode to MB so that we can keep all of the ios bot configuration in one place (almost). See the updated MB docs for details. R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org BUG=517216 CQ_EXTRA_TRYBOTS=tryserver.chromium.mac:ios_dbg_simulator_gn,ios_rel_device_gn ==========
Sana, please take another look. This, along with https://codereview.chromium.org/1410743010/, gets closer to what you wanted, I think. There's still a minor downside in that you still have to add a 'passthrough' entry for each iOS bot in mb_config.pyl, but I think having that explicit entry is a good sanity check for the configuration. https://codereview.chromium.org/1411183010/diff/160001/ios/build/bots/chromiu... File ios/build/bots/chromium.mac/iOS_Device_GN.json (right): https://codereview.chromium.org/1411183010/diff/160001/ios/build/bots/chromiu... ios/build/bots/chromium.mac/iOS_Device_GN.json:11: "gn_args": "target_os=\"ios\" target_cpu=\"arm\" ios_enable_code_signing=false", I'm waffling as to whether we should make the config files specify *both* the GYP_DEFINES and gn_args, even if only one will be used. There are two reasons to specify both: 1) Some gclient hooks will change what they do based on GYP_DEFINES settings, so if we don't set the GYP_DEFINES the wrong thing might happen. At a quick look, it didn't seem like any of the iOS hooks did this, but it's a risk. 2) If we specify both in the file, then switching between them is a one line (simple) change that would not involve looking up how to translate one set of args to the other. The downside of specifying both is that it's perhaps unnecessarily verbose and duplicative.
https://codereview.chromium.org/1411183010/diff/160001/ios/build/bots/chromiu... File ios/build/bots/chromium.mac/iOS_Device_GN.json (right): https://codereview.chromium.org/1411183010/diff/160001/ios/build/bots/chromiu... ios/build/bots/chromium.mac/iOS_Device_GN.json:11: "gn_args": "target_os=\"ios\" target_cpu=\"arm\" ios_enable_code_signing=false", On 2015/11/10 02:10:01, Dirk Pranke wrote: > I'm waffling as to whether we should make the config files > specify *both* the GYP_DEFINES and gn_args, even if only > one will be used. > > There are two reasons to specify both: > > 1) Some gclient hooks will change what they do based on > GYP_DEFINES settings, so if we don't set the GYP_DEFINES > the wrong thing might happen. At a quick look, it didn't > seem like any of the iOS hooks did this, but it's a risk. > > 2) If we specify both in the file, then switching between > them is a one line (simple) change that would not involve > looking up how to translate one set of args to the other. > > The downside of specifying both is that it's perhaps > unnecessarily verbose and duplicative. Both were specified in the .pyl anyways so both might as well be specified here. But I'd prefer to specify gn_args in a dict like gyp_defines. https://codereview.chromium.org/1411183010/diff/160001/tools/mb/mb_config.pyl File tools/mb/mb_config.pyl (right): https://codereview.chromium.org/1411183010/diff/160001/tools/mb/mb_config.pyl... tools/mb/mb_config.pyl:411: 'iOS Simulator GN (dbg)': 'passthrough', But you still need to enter every GN bot here, in addition to master.cfg, slaves.cfg, and the bot-specific .json. Can MB be changed so there's a way to invoke it in --passthrough mode, rather than having it consult mb_config.pyl and find out it should be in passthrough mode? Alternately, can it be set up so any bot that has no entry here defaults to passthrough mode?
https://codereview.chromium.org/1411183010/diff/160001/ios/build/bots/chromiu... File ios/build/bots/chromium.mac/iOS_Device_GN.json (right): https://codereview.chromium.org/1411183010/diff/160001/ios/build/bots/chromiu... ios/build/bots/chromium.mac/iOS_Device_GN.json:11: "gn_args": "target_os=\"ios\" target_cpu=\"arm\" ios_enable_code_signing=false", On 2015/11/10 22:08:37, smut wrote: > But I'd prefer to specify gn_args in a dict like gyp_defines. See my reply in the other CL. https://codereview.chromium.org/1411183010/diff/160001/tools/mb/mb_config.pyl File tools/mb/mb_config.pyl (right): https://codereview.chromium.org/1411183010/diff/160001/tools/mb/mb_config.pyl... tools/mb/mb_config.pyl:411: 'iOS Simulator GN (dbg)': 'passthrough', On 2015/11/10 22:08:37, smut wrote: > But you still need to enter every GN bot here, in addition to master.cfg, > slaves.cfg, and the bot-specific .json. > > Can MB be changed so there's a way to invoke it in --passthrough mode, rather > than having it consult mb_config.pyl and find out it should be in passthrough > mode? > > Alternately, can it be set up so any bot that has no entry here defaults to > passthrough mode? MB can't do either of those things the code exists in this patch. One could of course modify the code to do either of these things. I am reluctant to do so because I'm using mb_config.pyl as the source of truth for the GYP->GN migration, and if I don't list bots here then it will not be obvious whether or not they've been even looked at in the context of the migration. Of course, adding passthrough at all partially defeats my intentions. Can we let this go as-is for the moment and revisit this in a subsequent patch while I think about it some more?
https://codereview.chromium.org/1411183010/diff/160001/tools/mb/mb_config.pyl File tools/mb/mb_config.pyl (right): https://codereview.chromium.org/1411183010/diff/160001/tools/mb/mb_config.pyl... tools/mb/mb_config.pyl:411: 'iOS Simulator GN (dbg)': 'passthrough', On 2015/11/10 22:46:06, Dirk Pranke wrote: > On 2015/11/10 22:08:37, smut wrote: > > But you still need to enter every GN bot here, in addition to master.cfg, > > slaves.cfg, and the bot-specific .json. > > > > Can MB be changed so there's a way to invoke it in --passthrough mode, rather > > than having it consult mb_config.pyl and find out it should be in passthrough > > mode? > > > > Alternately, can it be set up so any bot that has no entry here defaults to > > passthrough mode? > > MB can't do either of those things the code exists in this patch. > > One could of course modify the code to do either of these things. > > I am reluctant to do so because I'm using mb_config.pyl as the source of truth > for the GYP->GN migration, and if I don't list bots here then it will not be > obvious whether or not they've been even looked at in the context of the > migration. > > Of course, adding passthrough at all partially defeats my intentions. > > Can we let this go as-is for the moment and revisit this in a subsequent patch > while I think about it some more? Agreed that MB can't do these things, but I'm asking for it to be able to. I hesitate to let this go, because it might end up just staying like this for awhile, and this makes it even more annoying to add a new bot since it now has to be enumerated here too. But, iOS upstream bots are added so rarely that I guess it's ok.
This is lgtm % the gn_args in .json files discussion going on elsewhere.
lgtm
Description was changed from ========== Add passthrough mode to MB and get iOS working. With these changes, the ios trybots should start working. Part of this change adds a 'passthrough' mode to MB so that we can keep all of the ios bot configuration in one place (almost). See the updated MB docs for details. R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org BUG=517216 CQ_EXTRA_TRYBOTS=tryserver.chromium.mac:ios_dbg_simulator_gn,ios_rel_device_gn ========== to ========== Make MB aware of iOS bot configs and get iOS working. With these changes, the ios trybots should start working. Part of this change makes MB aware of the bot config files in //ios/build/bots, so that we don't have to configure the iOS bots in two different places. See the updated MB docs for details. R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org BUG=517216 CQ_EXTRA_TRYBOTS=tryserver.chromium.mac:ios_dbg_simulator_gn,ios_rel_device_gn ==========
Patchset #10 (id:180001) has been deleted
Please take another look? See the updated recipe-side CL as well: https://codereview.chromium.org/1410743010/ . That one will need to land before this CL will pass the new ios trybots.
On 2015/11/18 at 04:28:06, dpranke wrote: > Please take another look? See the updated recipe-side CL as well: https://codereview.chromium.org/1410743010/ . That one will need to land before this CL will pass the new ios trybots. Who should take another look? Everyone or just smut?
On 2015/11/18 09:56:52, sdefresne wrote: > Who should take another look? Everyone or just smut? Sorry, just Sana needs to take another look.
https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/chromiu... File ios/build/bots/chromium.mac/iOS_Device_GN.json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/chromiu... ios/build/bots/chromium.mac/iOS_Device_GN.json:17: "ios_enable_code_signing=false" Reverse the list so this is alphabetical. The escaped quotation marks seem annoying, but I guess they are unavoidable. https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/chromiu... File ios/build/bots/chromium.mac/iOS_Simulator_GN_(dbg).json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/chromiu... ios/build/bots/chromium.mac/iOS_Simulator_GN_(dbg).json:16: "use_ios_simulator=false" Do you need ios_enable_code_signing=false? It's implied for simulator (even for gyp), but I've been explicit about it for gyp. Also what was the deal with target_subarch? Supporting it later? Do iOS GN builds build 32, 64, or 32+64 bit universal binaries if unspecified? https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... File ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json:16: "use_ios_simulator=false" What is meant by use_ios_simulator=false? Are you skipping compilation of iossim because you aren't running any tests below? https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... File ios/build/bots/tryserver.chromium.mac/ios_rel_device_gn.json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... ios/build/bots/tryserver.chromium.mac/ios_rel_device_gn.json:17: "ios_enable_code_signing=false" Reverse order (alpha sort). https://codereview.chromium.org/1411183010/diff/220001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/1411183010/diff/220001/tools/mb/mb.py#newcode370 tools/mb/mb.py:370: self.args.builder.replace(' ', '_') + '.json') Shouldn't need this. The recipes read the builder name exactly, and because of that I renamed the iOS builders with spaces in them (the main waterfall ones) to have underscores instead.
https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/chromiu... File ios/build/bots/chromium.mac/iOS_Device_GN.json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/chromiu... ios/build/bots/chromium.mac/iOS_Device_GN.json:17: "ios_enable_code_signing=false" On 2015/11/18 22:00:29, smut wrote: > Reverse the list so this is alphabetical. Okay. I was wondering if you'd want that or not. > The escaped quotation marks seem annoying, but I guess they are unavoidable. Yup. I didn't pick JSON as the format :). https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/chromiu... File ios/build/bots/chromium.mac/iOS_Simulator_GN_(dbg).json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/chromiu... ios/build/bots/chromium.mac/iOS_Simulator_GN_(dbg).json:16: "use_ios_simulator=false" On 2015/11/18 22:00:29, smut wrote: > Do you need ios_enable_code_signing=false? It's implied for simulator (even for > gyp), but I've been explicit about it for gyp. It's not needed; as you say, it's implied. I've been trying to prune out arguments that are not needed, because I think that makes things harder to maintain. > Also what was the deal with target_subarch? Supporting it later? Do iOS GN > builds build 32, 64, or 32+64 bit universal binaries if unspecified? I don't think we've really worked out how we want to handle target_subarch in a GN build yet. For starters, we don't support fat binaries at all on iOS. I think by default it'll just build 32-bit arm at the moment, but clearly we will need to fix this before too long. https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... File ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json:16: "use_ios_simulator=false" On 2015/11/18 22:00:29, smut wrote: > What is meant by use_ios_simulator=false? Are you skipping compilation of iossim > because you aren't running any tests below? I don't think iossim even builds yet in GN, period :). Regardless, use_ios_simulator doesn't affect it. The flag is defined in //build/config/ios/ios_sdk.gni and only used there; it controls which SDK we build (iphonesimulator or iphoneos), and also toggles a couple of defaults for code signing. https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... File ios/build/bots/tryserver.chromium.mac/ios_rel_device_gn.json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... ios/build/bots/tryserver.chromium.mac/ios_rel_device_gn.json:17: "ios_enable_code_signing=false" On 2015/11/18 22:00:29, smut wrote: > Reverse order (alpha sort). Acknowledged. https://codereview.chromium.org/1411183010/diff/220001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/1411183010/diff/220001/tools/mb/mb.py#newcode370 tools/mb/mb.py:370: self.args.builder.replace(' ', '_') + '.json') On 2015/11/18 22:00:29, smut wrote: > Shouldn't need this. The recipes read the builder name exactly, and because of > that I renamed the iOS builders with spaces in them (the main waterfall ones) to > have underscores instead. Ah, you're right. That's an unorthodox naming scheme for waterfall builders. Will update.
https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... File ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json:16: "use_ios_simulator=false" On 2015/11/18 22:30:27, Dirk Pranke wrote: > On 2015/11/18 22:00:29, smut wrote: > > What is meant by use_ios_simulator=false? Are you skipping compilation of > iossim > > because you aren't running any tests below? > > I don't think iossim even builds yet in GN, period :). > > Regardless, use_ios_simulator doesn't affect it. > > The flag is defined in //build/config/ios/ios_sdk.gni and only used there; > it controls which SDK we build (iphonesimulator or iphoneos), and also > toggles a couple of defaults for code signing. Toggles SDK? Can you use what I set on line 21? That's supposed to say whether it's iphoneos or iphonesimulator. Also, if that's what it means, then shouldn't use_ios_simulator be true? This should use iphonesimulator instead of iphoneos.
https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/chromiu... File ios/build/bots/chromium.mac/iOS_Simulator_GN_(dbg).json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/chromiu... ios/build/bots/chromium.mac/iOS_Simulator_GN_(dbg).json:16: "use_ios_simulator=false" I think this flag is wrong, just like ios_dbg_simulator_gn. https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... File ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json:16: "use_ios_simulator=false" On 2015/11/18 22:34:19, smut wrote: > Toggles SDK? Can you use what I set on line 21? That's supposed to say whether > it's iphoneos or iphonesimulator. Hmm. It looks like the way this is working is because GYP currently generates both -iphoneos and -iphonesimulator configurations, and then you use 'sdk' to figure out which directory to build (it generates 4 directories in total when you multiple by Release/Debug) GN doesn't work that way; we only generate a single directory's worth of files. So it needs to be told which config we want via gn_args. We could modify MB to be aware of more stuff from the config file, but that would make me increasingly unhappy. WDYT? > > Also, if that's what it means, then shouldn't use_ios_simulator be true? This > should use iphonesimulator instead of iphoneos. Yeah, actually I think this is just a typo on my part, so, good catch :).
https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... File ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json:16: "use_ios_simulator=false" On 2015/11/18 22:46:20, Dirk Pranke wrote: > On 2015/11/18 22:34:19, smut wrote: > > Toggles SDK? Can you use what I set on line 21? That's supposed to say whether > > it's iphoneos or iphonesimulator. > > Hmm. > > It looks like the way this is working is because GYP currently generates both > -iphoneos and -iphonesimulator configurations, and then you use 'sdk' to figure > out which directory to build (it generates 4 directories in total when you > multiple by Release/Debug) > > GN doesn't work that way; we only generate a single directory's worth of files. > So it needs to be told which config we want via gn_args. > > We could modify MB to be aware of more stuff from the config file, but > that would make me increasingly unhappy. WDYT? > > > > > Also, if that's what it means, then shouldn't use_ios_simulator be true? This > > should use iphonesimulator instead of iphoneos. > > Yeah, actually I think this is just a typo on my part, so, good catch :). In that case can it be made clearer that this corresponds with the "sdk" entry? e.g. sdk=\"iphonesimulator\" here and sdk=\"iphoneos\" in the device files. use_ios_simulator doesn't seem to really indicate that it's about SDK.
https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... File ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json (right): https://codereview.chromium.org/1411183010/diff/220001/ios/build/bots/tryserv... ios/build/bots/tryserver.chromium.mac/ios_dbg_simulator_gn.json:16: "use_ios_simulator=false" Let me fix all of the typos and try again. I'm getting lost at the moment otherwise :).
Patchset #10 (id:200001) has been deleted
Patchset #11 (id:240001) has been deleted
Patchset #13 (id:300001) has been deleted
Patchset #12 (id:280001) has been deleted
okay, sadly, the build is broken at ToT again, but let's see if we can at least get the config changes landed for now. Sana, please take another look? You'll see the use_ios_simulator target is gone completely now; that defaults to true when target_arch == "x64" (the default). We may still want to clean up the SDK flags, but I think we can deal w/ that in a separate patch that also looks at //build/config/ios_sdk.gni at the same time. Sound good?
On 2015/11/19 01:39:34, Dirk Pranke wrote: > okay, sadly, the build is broken at ToT again, but let's see if we can at least > get the config changes landed for now. > > Sana, please take another look? > > You'll see the use_ios_simulator target is gone completely now; that defaults to > true when target_arch == "x64" (the default). > > We may still want to clean up the SDK flags, but I think we can deal w/ that in > a separate patch that also looks at //build/config/ios_sdk.gni at the same time. > > Sound good? lgtm ship it
The CQ bit was checked by smut@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from stuartmorgan@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/1411183010/#ps320001 (title: "merge to #360460, clean up")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411183010/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411183010/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Description was changed from ========== Make MB aware of iOS bot configs and get iOS working. With these changes, the ios trybots should start working. Part of this change makes MB aware of the bot config files in //ios/build/bots, so that we don't have to configure the iOS bots in two different places. See the updated MB docs for details. R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org BUG=517216 CQ_EXTRA_TRYBOTS=tryserver.chromium.mac:ios_dbg_simulator_gn,ios_rel_device_gn ========== to ========== Make MB aware of iOS bot configs and get iOS working. With these changes, the ios trybots should start working. Part of this change makes MB aware of the bot config files in //ios/build/bots, so that we don't have to configure the iOS bots in two different places. See the updated MB docs for details. R=smut@google.com, sdefresne@chromium.org, stuartmorgan@chromium.org BUG=517216 ==========
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411183010/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411183010/320001
Message was sent while issue was closed.
Committed patchset #12 (id:320001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/e0f486fd71e596a87189b91c4e825a356db8ab5e Cr-Commit-Position: refs/heads/master@{#360688} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
