Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
66953010:
Added an iOS Device ninja build. (Closed)
|
Created:
7 years, 1 month ago by pgervais Modified:
7 years ago CC:
chromium-reviews, cmp-cc_chromium.org, ilevy-cc_chromium.org, Vadim Sh. Visibility:
Public. |
DescriptionAdded an iOS Device ninja build.
At the same time, the buildbot URL definition has been moved it from
master.cfg to master_site_config.py See crbug.com/319980.
BUG=310682
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=239064
Patch Set 1 #
Total comments: 3
Patch Set 2 : Reactivating svn polling #
Total comments: 4
Patch Set 3 : Removed commented line #
Total comments: 1
Patch Set 4 : Moved ios/clang builder to chromium.fyi #
Total comments: 4
Patch Set 5 : Reverted buildbot_url definition displacement #
Total comments: 1
Messages
Total messages: 26 (0 generated)
Could you review that new bot definition?
lgtm https://codereview.chromium.org/66953010/diff/1/masters/master.chromium.mac/m... File masters/master.chromium.mac/master_mac_cfg.py (right): https://codereview.chromium.org/66953010/diff/1/masters/master.chromium.mac/m... masters/master.chromium.mac/master_mac_cfg.py:314: # 'iphoneos', '-target' , 'All'], # for xcode 5 Is upstream still on Xcode 4.6? Just remove this line altogether if it's going to be commented out anyways.
On 2013/11/19 23:49:20, smut wrote: > lgtm > > https://codereview.chromium.org/66953010/diff/1/masters/master.chromium.mac/m... > File masters/master.chromium.mac/master_mac_cfg.py (right): > > https://codereview.chromium.org/66953010/diff/1/masters/master.chromium.mac/m... > masters/master.chromium.mac/master_mac_cfg.py:314: # 'iphoneos', '-target' , > 'All'], # for xcode 5 > Is upstream still on Xcode 4.6? Just remove this line altogether if it's going > to be commented out anyways. Yes, mini43-a1 is still running Xcode 4.6. I suppose it'll ported to xcode 5 at some point, so I left the commented line for the next person no to spend any time looking for a fix.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
rubber stamp LGTM, I don't know much about bot configs.
https://codereview.chromium.org/66953010/diff/1/masters/master.chromium.mac/m... File masters/master.chromium.mac/master_source_cfg.py (right): https://codereview.chromium.org/66953010/diff/1/masters/master.chromium.mac/m... masters/master.chromium.mac/master_source_cfg.py:33: # c['change_source'].append(poller) Just double-checking: is this intentional?
https://codereview.chromium.org/66953010/diff/1/masters/master.chromium.mac/m... File masters/master.chromium.mac/master_source_cfg.py (right): https://codereview.chromium.org/66953010/diff/1/masters/master.chromium.mac/m... masters/master.chromium.mac/master_source_cfg.py:33: # c['change_source'].append(poller) On 2013/11/21 22:33:02, Vadim Sh. wrote: > Just double-checking: is this intentional? Good catch. No, this is not intentional. It slipped through. Fixing that.
Fix uploaded. Could you check again?
lgtm (though I'm just driving by)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pgervais@chromium.org/66953010/120001
Presubmit check for 66953010-120001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
build/masters/master.chromium.mac/master.cfg
build/masters/master.chromium.mac/master_mac_cfg.py
build/masters/master.chromium.mac/master_site_config.py
build/masters/master.chromium.mac/slaves.cfg
Presubmit checks took 40.9s to calculate.
On 2013/11/21 23:16:43, I haz the power (commit-bot) wrote: > Presubmit check for 66953010-120001 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for these files: > build/masters/master.chromium.mac/master.cfg > build/masters/master.chromium.mac/master_mac_cfg.py > build/masters/master.chromium.mac/master_site_config.py > build/masters/master.chromium.mac/slaves.cfg > > Presubmit checks took 40.9s to calculate. Adding agable and stip to reviewers.
https://codereview.chromium.org/66953010/diff/120001/masters/master.chromium.... File masters/master.chromium.mac/master_mac_cfg.py (right): https://codereview.chromium.org/66953010/diff/120001/masters/master.chromium.... masters/master.chromium.mac/master_mac_cfg.py:314: # 'iphoneos', '-target' , 'All'], # for xcode 5 Don't commit commented out code :) https://codereview.chromium.org/66953010/diff/120001/masters/master.chromium.... masters/master.chromium.mac/master_mac_cfg.py:325: B('iOS Device ninja', 'ios_rel_ninja', gatekeeper='ios_rel_ninja', Just confirming that this configuration has been really tested. If it hasn't, putting this up on chromium.mac is going to make the whole tree red and then everyone will be sad.
https://codereview.chromium.org/66953010/diff/120001/masters/master.chromium.... File masters/master.chromium.mac/master_mac_cfg.py (right): https://codereview.chromium.org/66953010/diff/120001/masters/master.chromium.... masters/master.chromium.mac/master_mac_cfg.py:314: # 'iphoneos', '-target' , 'All'], # for xcode 5 On 2013/11/22 00:22:40, Aaron Gable wrote: > Don't commit commented out code :) As you which, but the next person will spend some time figuring out what to write here. https://codereview.chromium.org/66953010/diff/120001/masters/master.chromium.... masters/master.chromium.mac/master_mac_cfg.py:325: B('iOS Device ninja', 'ios_rel_ninja', gatekeeper='ios_rel_ninja', On 2013/11/22 00:22:40, Aaron Gable wrote: > Just confirming that this configuration has been really tested. If it hasn't, > putting this up on chromium.mac is going to make the whole tree red and then > everyone will be sad. I ran it locally on my mac pro, on a real iPad and the output was green. It took a long time to get there, mainly because the ninja compilation itself failed.
Why is it failing?
On 2013/11/22 14:01:57, justincohen wrote: > Why is it failing? If you're talking about ninja compilation, you were the one who solved that problem :-)
https://chromiumcodereview.appspot.com/66953010/diff/220001/masters/master.ch... File masters/master.chromium.mac/master_mac_cfg.py (right): https://chromiumcodereview.appspot.com/66953010/diff/220001/masters/master.ch... masters/master.chromium.mac/master_mac_cfg.py:324: B('iOS Device ninja', 'ios_rel_ninja', gatekeeper='ios_rel_ninja', we'll need to add a config to master_gatekeeper_cfg.py for this to take effect in order to put this on the main waterfall, we'll need trybot coverage and to add this to CQ via projects.py
I moved the builder to the chromium.fyi master. The configuration has been tested on my desktop mac, and ran flawlessly. What do you think of this new version?
lgtm with comment https://chromiumcodereview.appspot.com/66953010/diff/280001/masters/master.ch... File masters/master.chromium.fyi/master.cfg (right): https://chromiumcodereview.appspot.com/66953010/diff/280001/masters/master.ch... masters/master.chromium.fyi/master.cfg:331: def mac_out(): return F('src/out', 'darwin') I don't think you need this? https://chromiumcodereview.appspot.com/66953010/diff/280001/masters/master.ch... masters/master.chromium.fyi/master.cfg:2359: c['buildbotURL'] = ActiveMaster.buildbot_url in general a good idea to make this change in a separate CL, but might as well keep it here https://chromiumcodereview.appspot.com/66953010/diff/280001/masters/master.ch... File masters/master.chromium.mac/master_mac_cfg.py (right): https://chromiumcodereview.appspot.com/66953010/diff/280001/masters/master.ch... masters/master.chromium.mac/master_mac_cfg.py:314: 'iphoneos6.1', '-target' , 'All'], # for xcode 4.6. Use 'iphoneos' for 5. capitalize 'for' might be a better idea to put this on a separate line: 'iphoneos6.1 for xcode 4.6, iphoneos for xcode 5'
Thanks for your comments. A last question as a line comment below. https://chromiumcodereview.appspot.com/66953010/diff/280001/masters/master.ch... File masters/master.chromium.fyi/master.cfg (right): https://chromiumcodereview.appspot.com/66953010/diff/280001/masters/master.ch... masters/master.chromium.fyi/master.cfg:2359: c['buildbotURL'] = ActiveMaster.buildbot_url On 2013/12/05 21:04:25, stip wrote: > in general a good idea to make this change in a separate CL, but might as well > keep it here There are actually two of these modifications in that CL (another for master.chromium.mac). What is the best way to do these modifications: do a separate CL for each master (so it's easier to revert), or can we do several at once?
On 2013/12/05 21:44:13, pgervais wrote: > Thanks for your comments. A last question as a line comment below. > > https://chromiumcodereview.appspot.com/66953010/diff/280001/masters/master.ch... > File masters/master.chromium.fyi/master.cfg (right): > > https://chromiumcodereview.appspot.com/66953010/diff/280001/masters/master.ch... > masters/master.chromium.fyi/master.cfg:2359: c['buildbotURL'] = > ActiveMaster.buildbot_url > On 2013/12/05 21:04:25, stip wrote: > > in general a good idea to make this change in a separate CL, but might as well > > keep it here > > There are actually two of these modifications in that CL (another for > master.chromium.mac). What is the best way to do these modifications: do a > separate CL for each master (so it's easier to revert), or can we do several at > once? depends on how risky the change is. in this case not very risky, and you're probably going to restart FYI first so any breakage should be obvious. I'd be fine with one CL for both masters.
On 2013/12/05 21:52:57, stip wrote: > > depends on how risky the change is. in this case not very risky, and you're > probably going to restart FYI first so any breakage should be obvious. I'd be > fine with one CL for both masters. I reverted back the displacement of the c['buildbotURL'] definition, and will do it in a separate CL. Should be good to go now, one final review and you're done!
🚢🚢🚢 lgtm ship it 🚢🚢🚢
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pgervais@chromium.org/66953010/300001
Message was sent while issue was closed.
Change committed as 239064
Message was sent while issue was closed.
https://codereview.chromium.org/66953010/diff/300001/masters/master.chromium.... File masters/master.chromium.fyi/master.cfg (right): https://codereview.chromium.org/66953010/diff/300001/masters/master.chromium.... masters/master.chromium.fyi/master.cfg:141: 'Chromium iOS Device', Add 'Chromium iOS Device (ninja)' here so commits will trigger this builder. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
