Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127963003:
Use goma on ios-device, ios-simulator builders. (Closed)
|
Created:
4 years, 5 months ago by Dirk Pranke Modified:
4 years, 5 months ago CC:
chromium-reviews, Nico Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse goma on ios-device, ios-simulator builders.
Now that these builders are using ninja, there isn't a compelling
reason not to also be using goma.
R=smut@chromium.org
NOTRY=true
BUG=626005
Committed: https://crrev.com/02339c42fda6687886b7d16be3fdb1cf2900813f
Cr-Commit-Position: refs/heads/master@{#404494}
Patch Set 1 #Patch Set 2 : use MB on all builders for consistency. #
Total comments: 7
Patch Set 3 : address review comments #
Total comments: 2
Patch Set 4 : fix goma flag syntax #
Messages
Total messages: 37 (14 generated)
smut@google.com changed reviewers: + smut@google.com
So ios-device is green but compilation clearly failed due to goma in that job.
On 2016/07/06 22:25:47, smut wrote: > So ios-device is green but compilation clearly failed due to goma in that job. I'm not sure if I understand what you're saying ... in that job, compilation succeeded, but it didn't use goma, right? Of course, that's not actually what we want, which is why I need to switch to using MB everywhere, in the next patchset.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
On 2016/07/06 22:54:47, Dirk Pranke wrote: > On 2016/07/06 22:25:47, smut wrote: > > So ios-device is green but compilation clearly failed due to goma in that job. > > I'm not sure if I understand what you're saying ... in that job, compilation > succeeded, but > it didn't use goma, right? Of course, that's not actually what we want, which is > why > I need to switch to using MB everywhere, in the next patchset. I guess I was looking at the wrong one. I saw that ios-device was green but must've clicked ios-device-gn.
https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... File ios/build/bots/chromium.fyi/EarlGreyiOS.json (right): https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... ios/build/bots/chromium.fyi/EarlGreyiOS.json:16: "target_subarch=64" arm64 https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... File ios/build/bots/chromium.mac/ios-device-gn.json (right): https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... ios/build/bots/chromium.mac/ios-device-gn.json:16: "target_subarch=arm32" both
https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... File ios/build/bots/chromium.fyi/EarlGreyiOS.json (right): https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... ios/build/bots/chromium.fyi/EarlGreyiOS.json:16: "target_subarch=64" On 2016/07/06 22:57:02, smut wrote: > arm64 Yeah, I was going to ask about this. It was 64 originally (before this change), but I was guessing that was a typo. https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... File ios/build/bots/chromium.mac/ios-device-gn.json (right): https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... ios/build/bots/chromium.mac/ios-device-gn.json:16: "target_subarch=arm32" On 2016/07/06 22:57:02, smut wrote: > both I think both is actually wrong (which is why I changed it) We don't actually use target_subarch in GN builds and it's only doing a 32-bit build, and so I figured it would be better for the GYP_DEFINES to be consistent. But, since this isn't using GYP, I'm not sure that this even matters?
https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... File ios/build/bots/chromium.mac/ios-device-gn.json (right): https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... ios/build/bots/chromium.mac/ios-device-gn.json:16: "target_subarch=arm32" On 2016/07/06 23:08:35, Dirk Pranke wrote: > On 2016/07/06 22:57:02, smut wrote: > > both > > I think both is actually wrong (which is why I changed it) > > We don't actually use target_subarch in GN builds and > it's only doing a 32-bit build, and so I figured it would be > better for the GYP_DEFINES to be consistent. > > But, since this isn't using GYP, I'm not sure that this even matters? We desire to be able to use target_subarch in GN builds even if we can't currently. I think everything upstream should ideally be target_subarch=both, but that's up for debate. I guess it doesn't matter if GN can't do target_subarch=both anyways. At any rate, my only issue was that this doesn't match what it says in the comment on line 7.
Updated. Please take another look? https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... File ios/build/bots/chromium.mac/ios-device-gn.json (right): https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... ios/build/bots/chromium.mac/ios-device-gn.json:16: "target_subarch=arm32" On 2016/07/06 23:14:21, smut wrote: > On 2016/07/06 23:08:35, Dirk Pranke wrote: > > On 2016/07/06 22:57:02, smut wrote: > > > both > > > > I think both is actually wrong (which is why I changed it) > > > > We don't actually use target_subarch in GN builds and > > it's only doing a 32-bit build, and so I figured it would be > > better for the GYP_DEFINES to be consistent. > > > > But, since this isn't using GYP, I'm not sure that this even matters? > > We desire to be able to use target_subarch in GN builds even if we can't > currently. I think everything upstream should ideally be target_subarch=both, > but that's up for debate. I guess it doesn't matter if GN can't do > target_subarch=both anyways. > > At any rate, my only issue was that this doesn't match what it says in the > comment on line 7. Ah, right. I'll fix the comment for now. Once GN supports fat builds, we can/should switch this back (though it almost certainly won't use the variable "target_subarch" for that).
https://codereview.chromium.org/2127963003/diff/80001/ios/build/bots/chromium... File ios/build/bots/chromium.fyi/ClangToTiOS.json (right): https://codereview.chromium.org/2127963003/diff/80001/ios/build/bots/chromium... ios/build/bots/chromium.fyi/ClangToTiOS.json:15: "werror=" This one still does *not* use goma (it can't, because we can't use goma w/ tip-of-tree clang), but I changed these entries anyway just for consistency
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
https://codereview.chromium.org/2127963003/diff/80001/ios/build/bots/chromium... File ios/build/bots/chromium.fyi/ClangToTiOS.json (right): https://codereview.chromium.org/2127963003/diff/80001/ios/build/bots/chromium... ios/build/bots/chromium.fyi/ClangToTiOS.json:15: "werror=" On 2016/07/07 01:29:47, Dirk Pranke wrote: > This one still does *not* use goma (it can't, because we can't use goma w/ > tip-of-tree clang), but I changed these entries anyway just for consistency This is surprising, locally goma works with both clang shipped with Chromium and with clang shipped with Xcode. And do not think we have any bot using ToT clang (i.e. rebuilding clang).
Description was changed from ========== Use goma on ios-device, ios-simulator builders. Now that these builders are using ninja, there isn't a compelling reason not to also be using goma. R=smut@chromium.org BUG=626005 ========== to ========== Use goma on ios-device, ios-simulator builders. Now that these builders are using ninja, there isn't a compelling reason not to also be using goma. R=smut@chromium.org BUG=626005 ==========
On 2016/07/07 07:11:10, sdefresne wrote: > https://codereview.chromium.org/2127963003/diff/80001/ios/build/bots/chromium... > File ios/build/bots/chromium.fyi/ClangToTiOS.json (right): > > https://codereview.chromium.org/2127963003/diff/80001/ios/build/bots/chromium... > ios/build/bots/chromium.fyi/ClangToTiOS.json:15: "werror=" > On 2016/07/07 01:29:47, Dirk Pranke wrote: > > This one still does *not* use goma (it can't, because we can't use goma w/ > > tip-of-tree clang), but I changed these entries anyway just for consistency > > This is surprising, locally goma works with both clang shipped with Chromium and > with clang shipped with Xcode. And do not think we have any bot using ToT clang > (i.e. rebuilding clang). ToT clang is the tip of tree of the *clang* repo (which is why this is an FYI bot; we expect it to break often). My understanding is that the goma team has to pull new binaries to their system, and I believe they don't do that until we're rolling new clang revisions in. But, thakis@ can correct me if I'm wrong here.
On 2016/07/07 16:08:44, Dirk Pranke wrote: > On 2016/07/07 07:11:10, sdefresne wrote: > > > https://codereview.chromium.org/2127963003/diff/80001/ios/build/bots/chromium... > > File ios/build/bots/chromium.fyi/ClangToTiOS.json (right): > > > > > https://codereview.chromium.org/2127963003/diff/80001/ios/build/bots/chromium... > > ios/build/bots/chromium.fyi/ClangToTiOS.json:15: "werror=" > > On 2016/07/07 01:29:47, Dirk Pranke wrote: > > > This one still does *not* use goma (it can't, because we can't use goma w/ > > > tip-of-tree clang), but I changed these entries anyway just for consistency > > > > This is surprising, locally goma works with both clang shipped with Chromium > and > > with clang shipped with Xcode. And do not think we have any bot using ToT > clang > > (i.e. rebuilding clang). > > ToT clang is the tip of tree of the *clang* repo (which is why this is an FYI > bot; we > expect it to break often). > > My understanding is that the goma team has to pull new binaries to their system, > and > I believe they don't do that until we're rolling new clang revisions in. > > But, thakis@ can correct me if I'm wrong here. Oh, I didn't knew we had such a bot. Make sense that it is not available on goma then. lgtm
On Thu, Jul 7, 2016 at 12:08 PM, <dpranke@chromium.org> wrote: > On 2016/07/07 07:11:10, sdefresne wrote: > > > > https://codereview.chromium.org/2127963003/diff/80001/ios/build/bots/chromium... > > File ios/build/bots/chromium.fyi/ClangToTiOS.json (right): > > > > > > https://codereview.chromium.org/2127963003/diff/80001/ios/build/bots/chromium... > > ios/build/bots/chromium.fyi/ClangToTiOS.json:15: "werror=" > > On 2016/07/07 01:29:47, Dirk Pranke wrote: > > > This one still does *not* use goma (it can't, because we can't use > goma w/ > > > tip-of-tree clang), but I changed these entries anyway just for > consistency > > > > This is surprising, locally goma works with both clang shipped with > Chromium > and > > with clang shipped with Xcode. And do not think we have any bot using ToT > clang > > (i.e. rebuilding clang). > > ToT clang is the tip of tree of the *clang* repo (which is why this is an > FYI > bot; we > expect it to break often). > Correct; we have a whole bunch of bots first building clang trunk, then building chrome trunk with it, and then running chrome's tests: https://build.chromium.org/p/chromium.fyi/console?category=clang%20tot > My understanding is that the goma team has to pull new binaries to their > system, > and > I believe they don't do that until we're rolling new clang revisions in. > This is basically correct (goma needs identical compiler binaries locally and remotely), except that we manually push clang binaries to goma when we update the compiler (as opposed to goma team pulling them automatically), at least for now. I'm a bit surprised this bot builds with werror defined to nothing. I think the werror gyp define only has an effect on non-mac non-ios posix so it's a no-op and it's ok, but it's a bit confusing. > > But, thakis@ can correct me if I'm wrong here. > > https://codereview.chromium.org/2127963003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Lgtm in theory, though in practice three of the try bots are now broken. https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... File ios/build/bots/chromium.mac/ios-device-gn.json (right): https://codereview.chromium.org/2127963003/diff/60001/ios/build/bots/chromium... ios/build/bots/chromium.mac/ios-device-gn.json:16: "target_subarch=arm32" On 2016/07/07 01:09:40, Dirk Pranke wrote: > On 2016/07/06 23:14:21, smut wrote: > > On 2016/07/06 23:08:35, Dirk Pranke wrote: > > > On 2016/07/06 22:57:02, smut wrote: > > > > both > > > > > > I think both is actually wrong (which is why I changed it) > > > > > > We don't actually use target_subarch in GN builds and > > > it's only doing a 32-bit build, and so I figured it would be > > > better for the GYP_DEFINES to be consistent. > > > > > > But, since this isn't using GYP, I'm not sure that this even matters? > > > > We desire to be able to use target_subarch in GN builds even if we can't > > currently. I think everything upstream should ideally be target_subarch=both, > > but that's up for debate. I guess it doesn't matter if GN can't do > > target_subarch=both anyways. > > > > At any rate, my only issue was that this doesn't match what it says in the > > comment on line 7. > > Ah, right. I'll fix the comment for now. Once GN supports fat builds, we > can/should switch this back > (though it almost certainly won't use the variable "target_subarch" for that). It would be consistent with gyp if it did, but okay.
On 2016/07/07 21:31:36, smut wrote: > Lgtm in theory, though in practice three of the try bots are now broken. Yup, I need to fix some stuff in the recipes before this will work. > > > We desire to be able to use target_subarch in GN builds even if we can't > > > currently. I think everything upstream should ideally be > target_subarch=both, > > > but that's up for debate. I guess it doesn't matter if GN can't do > > > target_subarch=both anyways. > > > > > > At any rate, my only issue was that this doesn't match what it says in the > > > comment on line 7. > > > > Ah, right. I'll fix the comment for now. Once GN supports fat builds, we > > can/should switch this back > > (though it almost certainly won't use the variable "target_subarch" for that). > > It would be consistent with gyp if it did, but okay. The way GN handles multiple toolchains is substantially different from how GYP does it, and target_subarch doesn't make much sense in GN.
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, smut@google.com Link to the patchset: https://codereview.chromium.org/2127963003/#ps100001 (title: "fix goma flag syntax")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dpranke@chromium.org
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use goma on ios-device, ios-simulator builders. Now that these builders are using ninja, there isn't a compelling reason not to also be using goma. R=smut@chromium.org BUG=626005 ========== to ========== Use goma on ios-device, ios-simulator builders. Now that these builders are using ninja, there isn't a compelling reason not to also be using goma. R=smut@chromium.org NOTRY=true BUG=626005 ==========
The CQ bit was unchecked by dpranke@chromium.org
Marking as NOTRY=true to see if this'll unstick this. All the normal CQ jobs have actually passed but the CQ seems wedged on one unnecessary job.
The CQ bit was checked by dpranke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use goma on ios-device, ios-simulator builders. Now that these builders are using ninja, there isn't a compelling reason not to also be using goma. R=smut@chromium.org NOTRY=true BUG=626005 ========== to ========== Use goma on ios-device, ios-simulator builders. Now that these builders are using ninja, there isn't a compelling reason not to also be using goma. R=smut@chromium.org NOTRY=true BUG=626005 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Use goma on ios-device, ios-simulator builders. Now that these builders are using ninja, there isn't a compelling reason not to also be using goma. R=smut@chromium.org NOTRY=true BUG=626005 ========== to ========== Use goma on ios-device, ios-simulator builders. Now that these builders are using ninja, there isn't a compelling reason not to also be using goma. R=smut@chromium.org NOTRY=true BUG=626005 Committed: https://crrev.com/02339c42fda6687886b7d16be3fdb1cf2900813f Cr-Commit-Position: refs/heads/master@{#404494} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/02339c42fda6687886b7d16be3fdb1cf2900813f Cr-Commit-Position: refs/heads/master@{#404494} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
