Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
157073002:
Bot update! (Closed)
|
Created:
6 years, 10 months ago by Ryan Tseng Modified:
6 years, 10 months ago CC:
chromium-reviews, stip+watch_chromium.org, cmp-cc_chromium.org, ilevy-cc_chromium.org, kjellander+cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/build.git@master Visibility:
Public. |
DescriptionThis implements all of bot_update.py except for applying patches.
This patch is dependent on https://codereview.chromium.org/150653004/
for proper deps2git.py invocation.
The chromium_commands.py bits are for disabling the update step when it sees a flag file emitted by bot_update.py
The flow essentially is:
1. Delete flag, if its there
2. Check if we're on a master/builder/slave that should be running this.
3. Parse and convert gclient specs to git
4. Delete build directory if .svn is present in one of the solutions
5. Fetch a git checkout using clone/reset/clean/pull/clean
6. Run deps2git
7. Gclient sync
8. Emit flag
9. ???
10. Profit
BUG=339168
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=250236
Patch Set 1 #Patch Set 2 : Removed unneeded deferreds #Patch Set 3 : Minor fixes #Patch Set 4 : moved gclient message around #Patch Set 5 : We dont' actually want to run on chromium.linux yet #
Total comments: 24
Patch Set 6 : Review fixews #Patch Set 7 : Chromium_commands pruning #Patch Set 8 : Change cache dir #Patch Set 9 : Fix PS8 #Patch Set 10 : Chromium commands fix #
Total comments: 11
Patch Set 11 : nit fix #Patch Set 12 : Add cache_dir to wanted directory #Patch Set 13 : Fixed cache dir from the build/.. dir to build/../.. dir #Patch Set 14 : Wait we got the directory right the first time #Patch Set 15 : Rebase #Patch Set 16 : pylint #Patch Set 17 : No need to print message in chromium_util #
Total comments: 29
Patch Set 18 : Review fix #
Total comments: 8
Patch Set 19 : Review fix #
Total comments: 1
Patch Set 20 : Remove environ #
Total comments: 12
Messages
Total messages: 30 (0 generated)
Hi Aaron and Robbie, can you guys review this? It has passed (compile) on my local machine.
https://codereview.chromium.org/157073002/diff/130001/scripts/common/chromium... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/157073002/diff/130001/scripts/common/chromium... scripts/common/chromium_utils.py:412: print 'a git checkout already. Skipping this step.' Not sure I like this phrasing. Maybe "update.flag file found: assuming bot_update has run and checkout is in a consistent state. No actions will be performed." https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:112: code = proc.wait() retcode https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:113: if not code: elapsed = ((time.time() - start_time) / 60.0) reused elapsed in prints below https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:129: sln['deps_file'] = '.DEPS.git' solutions_to_git already does this, I wouldn't do it here. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:145: if os.environ.get('TESTING_MASTER') == 'localhost': nope nope nope https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:219: scm_filename is expected to be either ['.svn', '.git'] scm_dirname? https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:229: os.chdir(path.dirname(os.getcwd())) Can't this be done without chdir? rmtree should only care about open files, not processes who think their pwd is build/. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:288: call('git', '-C', sln_dir, 'checkout', git_hash) does call('git', '-C', sln_dir, 'checkout', None) just checkout HEAD anyway? Not sure what subprocess.Popen's behavior is on 'None' args. Also, this is a really silent fallback, which I'm not sure is good. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:338: delete_flag(options.flag_file) I think we should only delete the flag if we're not supposed to be active. If this script somehow fails in the middle (between deleting and re-adding the flag file) we still don't want old processes to take over -- that'll just make a mess. It should fail, and fail hard. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/chromium_... File scripts/slave/chromium_commands.py (right): https://codereview.chromium.org/157073002/diff/130001/scripts/slave/chromium_... scripts/slave/chromium_commands.py:300: 'exiting\n'}) This message should match the messages in the other two scripts. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/chromium_... scripts/slave/chromium_commands.py:429: self.sendStatus({'header': "In VC Update\n"}) Not sure why you introduced these sendStatus calls. They're not consistent (e.g. with doClobber) and not directly related to this change afaict. Leave them out. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/gclient_s... File scripts/slave/gclient_safe_revert.py (right): https://codereview.chromium.org/157073002/diff/130001/scripts/slave/gclient_s... scripts/slave/gclient_safe_revert.py:32: print 'a git checkout already. Skipping this step.' See comment about phrasing in chromium_utils.
Fixes made, currently running here: http://oddish.mtv.corp.google.com:8019/builders/LinuxGit%20x64/ ptal. https://codereview.chromium.org/157073002/diff/130001/scripts/common/chromium... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/157073002/diff/130001/scripts/common/chromium... scripts/common/chromium_utils.py:412: print 'a git checkout already. Skipping this step.' On 2014/02/07 04:47:40, agable wrote: > Not sure I like this phrasing. Maybe > > "update.flag file found: assuming bot_update has run and checkout is in a > consistent state. No actions will be performed." Done. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:112: code = proc.wait() On 2014/02/07 04:47:40, agable wrote: > retcode I feel more comfortable waiting for the process, because the pipe could potentially close before the process actually completes. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:113: if not code: On 2014/02/07 04:47:40, agable wrote: > elapsed = ((time.time() - start_time) / 60.0) > > reused elapsed in prints below Done. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:129: sln['deps_file'] = '.DEPS.git' On 2014/02/07 04:47:40, agable wrote: > solutions_to_git already does this, I wouldn't do it here. oops you're right. Done. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:145: if os.environ.get('TESTING_MASTER') == 'localhost': On 2014/02/07 04:47:40, agable wrote: > nope nope nope herp de derp. Done. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:219: scm_filename is expected to be either ['.svn', '.git'] On 2014/02/07 04:47:40, agable wrote: > scm_dirname? Done. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:229: os.chdir(path.dirname(os.getcwd())) On 2014/02/07 04:47:40, agable wrote: > Can't this be done without chdir? rmtree should only care about open files, not > processes who think their pwd is build/. Discussed offline, but this needs to be here. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:288: call('git', '-C', sln_dir, 'checkout', git_hash) On 2014/02/07 04:47:40, agable wrote: > does call('git', '-C', sln_dir, 'checkout', None) just checkout HEAD anyway? Not > sure what subprocess.Popen's behavior is on 'None' args. Also, this is a really > silent fallback, which I'm not sure is good. oops fixed (by not even checking for that conditional) https://codereview.chromium.org/157073002/diff/130001/scripts/slave/bot_updat... scripts/slave/bot_update.py:338: delete_flag(options.flag_file) On 2014/02/07 04:47:40, agable wrote: > I think we should only delete the flag if we're not supposed to be active. If > this script somehow fails in the middle (between deleting and re-adding the flag > file) we still don't want old processes to take over -- that'll just make a > mess. It should fail, and fail hard. sgtm. Moved delete/emit flag down next to ensure_no_checkout on line 365 https://codereview.chromium.org/157073002/diff/130001/scripts/slave/chromium_... File scripts/slave/chromium_commands.py (right): https://codereview.chromium.org/157073002/diff/130001/scripts/slave/chromium_... scripts/slave/chromium_commands.py:300: 'exiting\n'}) On 2014/02/07 04:47:40, agable wrote: > This message should match the messages in the other two scripts. Done. https://codereview.chromium.org/157073002/diff/130001/scripts/slave/chromium_... scripts/slave/chromium_commands.py:429: self.sendStatus({'header': "In VC Update\n"}) On 2014/02/07 04:47:40, agable wrote: > Not sure why you introduced these sendStatus calls. They're not consistent (e.g. > with doClobber) and not directly related to this change afaict. Leave them out. Oh weird I totally thought i took these out, fixed https://codereview.chromium.org/157073002/diff/130001/scripts/slave/gclient_s... File scripts/slave/gclient_safe_revert.py (right): https://codereview.chromium.org/157073002/diff/130001/scripts/slave/gclient_s... scripts/slave/gclient_safe_revert.py:32: print 'a git checkout already. Skipping this step.' On 2014/02/07 04:47:40, agable wrote: > See comment about phrasing in chromium_utils. Done.
That link seems broken, try this one: http://oddish.mtv.corp.google.com:8019/builders/LinuxGit%20x64 On Fri, Feb 7, 2014 at 1:23 PM, <hinoka@google.com> wrote: > Fixes made, currently running here: > > http://oddish.mtv.corp.google.com:8019/builders/LinuxGit%20x64/ > > ptal. > > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/common/chromium_utils.py > File scripts/common/chromium_utils.py (right): > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/common/chromium_utils.py#newcode412 > scripts/common/chromium_utils.py:412: print 'a git checkout already. > Skipping this step.' > On 2014/02/07 04:47:40, agable wrote: > >> Not sure I like this phrasing. Maybe >> > > "update.flag file found: assuming bot_update has run and checkout is >> > in a > >> consistent state. No actions will be performed." >> > > Done. > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/bot_update.py > File scripts/slave/bot_update.py (right): > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/bot_update.py#newcode112 > scripts/slave/bot_update.py:112: code = proc.wait() > On 2014/02/07 04:47:40, agable wrote: > >> retcode >> > > I feel more comfortable waiting for the process, because the pipe could > potentially close before the process actually completes. > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/bot_update.py#newcode113 > scripts/slave/bot_update.py:113: if not code: > On 2014/02/07 04:47:40, agable wrote: > >> elapsed = ((time.time() - start_time) / 60.0) >> > > reused elapsed in prints below >> > > Done. > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/bot_update.py#newcode129 > scripts/slave/bot_update.py:129: sln['deps_file'] = '.DEPS.git' > On 2014/02/07 04:47:40, agable wrote: > >> solutions_to_git already does this, I wouldn't do it here. >> > > oops you're right. Done. > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/bot_update.py#newcode145 > scripts/slave/bot_update.py:145: if os.environ.get('TESTING_MASTER') == > 'localhost': > On 2014/02/07 04:47:40, agable wrote: > >> nope nope nope >> > > herp de derp. Done. > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/bot_update.py#newcode219 > scripts/slave/bot_update.py:219: scm_filename is expected to be either > ['.svn', '.git'] > On 2014/02/07 04:47:40, agable wrote: > >> scm_dirname? >> > > Done. > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/bot_update.py#newcode229 > scripts/slave/bot_update.py:229: os.chdir(path.dirname(os.getcwd())) > On 2014/02/07 04:47:40, agable wrote: > >> Can't this be done without chdir? rmtree should only care about open >> > files, not > >> processes who think their pwd is build/. >> > > Discussed offline, but this needs to be here. > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/bot_update.py#newcode288 > scripts/slave/bot_update.py:288: call('git', '-C', sln_dir, 'checkout', > git_hash) > On 2014/02/07 04:47:40, agable wrote: > >> does call('git', '-C', sln_dir, 'checkout', None) just checkout HEAD >> > anyway? Not > >> sure what subprocess.Popen's behavior is on 'None' args. Also, this is >> > a really > >> silent fallback, which I'm not sure is good. >> > oops fixed (by not even checking for that conditional) > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/bot_update.py#newcode338 > scripts/slave/bot_update.py:338: delete_flag(options.flag_file) > On 2014/02/07 04:47:40, agable wrote: > >> I think we should only delete the flag if we're not supposed to be >> > active. If > >> this script somehow fails in the middle (between deleting and >> > re-adding the flag > >> file) we still don't want old processes to take over -- that'll just >> > make a > >> mess. It should fail, and fail hard. >> > > sgtm. Moved delete/emit flag down next to ensure_no_checkout on line > 365 > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/chromium_commands.py > File scripts/slave/chromium_commands.py (right): > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/chromium_commands.py#newcode300 > scripts/slave/chromium_commands.py:300: 'exiting\n'}) > On 2014/02/07 04:47:40, agable wrote: > >> This message should match the messages in the other two scripts. >> > > Done. > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/chromium_commands.py#newcode429 > scripts/slave/chromium_commands.py:429: self.sendStatus({'header': "In > VC Update\n"}) > On 2014/02/07 04:47:40, agable wrote: > >> Not sure why you introduced these sendStatus calls. They're not >> > consistent (e.g. > >> with doClobber) and not directly related to this change afaict. Leave >> > them out. > > Oh weird I totally thought i took these out, fixed > > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/gclient_safe_revert.py > File scripts/slave/gclient_safe_revert.py (right): > > https://codereview.chromium.org/157073002/diff/130001/ > scripts/slave/gclient_safe_revert.py#newcode32 > scripts/slave/gclient_safe_revert.py:32: print 'a git checkout already. > Skipping this step.' > On 2014/02/07 04:47:40, agable wrote: > >> See comment about phrasing in chromium_utils. >> > > Done. > > https://codereview.chromium.org/157073002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM modulo some nits/questions. https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... scripts/slave/bot_update.py:89: CACHE_DIR = os.path.join( could do this relative to SCRIPTS_DIR. Probably should, actually. Another good idiom is THIS_DIR=path.abspath(__file__) SCRIPTS_DIR = stuff(THIS_DIR) CACHE_DIR = stuff(THIS_DIR) etc. https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... scripts/slave/bot_update.py:292: git_hash = revision Since we're willing to use refs (HEAD, master, my-funky-dev-branch, etc) this should probably be called git_ref, not git_hash. https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... scripts/slave/bot_update.py:298: call('git', '-C', sln_dir, 'clean', '-df') Only do this if you really think it's necessary. Shouldn't do any extra work since this step will already take long enough as it is. https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... scripts/slave/bot_update.py:333: # TODO(hinoka): We don't actually use this Then can we remove it? Or does that require a master side change too?
https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... scripts/slave/bot_update.py:89: CACHE_DIR = os.path.join( On 2014/02/07 22:19:06, agable wrote: > could do this relative to SCRIPTS_DIR. Probably should, actually. > > Another good idiom is > > THIS_DIR=path.abspath(__file__) > SCRIPTS_DIR = stuff(THIS_DIR) > CACHE_DIR = stuff(THIS_DIR) > > etc. * THIS_DIR = path.abspath(path.dirname(__file__)) https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... scripts/slave/bot_update.py:298: call('git', '-C', sln_dir, 'clean', '-df') On 2014/02/07 22:19:06, agable wrote: > Only do this if you really think it's necessary. Shouldn't do any extra work > since this step will already take long enough as it is. Isn't this necessary because there could be a patch applied? Though apply_issue currently actually commits the patch... Oh, yeah, since apply_issue commits the patch, we should also make sure to set user.name and user.email on the root repo. That said, if apply_issue really commits it, then we won't need clean because there will never be any untracked stuff. https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... scripts/slave/bot_update.py:333: # TODO(hinoka): We don't actually use this On 2014/02/07 22:19:06, agable wrote: > Then can we remove it? Or does that require a master side change too? We probably shouldn't rely on this being a parameter. In fact, I think MANY of these parameters are not needed/wanted, and should be derived slaveside from mastername/buildername instead. Otherwise we require the master configuration to pass them correctly.......
https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... scripts/slave/bot_update.py:89: CACHE_DIR = os.path.join( On 2014/02/07 22:19:06, agable wrote: > could do this relative to SCRIPTS_DIR. Probably should, actually. > > Another good idiom is > > THIS_DIR=path.abspath(__file__) > SCRIPTS_DIR = stuff(THIS_DIR) > CACHE_DIR = stuff(THIS_DIR) > > etc. Done. https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... scripts/slave/bot_update.py:292: git_hash = revision On 2014/02/07 22:19:06, agable wrote: > Since we're willing to use refs (HEAD, master, my-funky-dev-branch, etc) this > should probably be called git_ref, not git_hash. Done. https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... scripts/slave/bot_update.py:298: call('git', '-C', sln_dir, 'clean', '-df') On 2014/02/07 22:19:06, agable wrote: > Only do this if you really think it's necessary. Shouldn't do any extra work > since this step will already take long enough as it is. The one on line 283 is probably enough. I'll remove this. https://codereview.chromium.org/157073002/diff/290001/scripts/slave/bot_updat... scripts/slave/bot_update.py:333: # TODO(hinoka): We don't actually use this On 2014/02/07 22:19:06, agable wrote: > Then can we remove it? Or does that require a master side change too? Rephrased. I mean we need to use this at some point, but it is yet to be implemented. Before its implemented anything that depends on the behavior of revision mapping will be incorrect.
Will commit once http://oddish.mtv.corp.google.com:8019/builders/LinuxGit%20x64/builds/3 goes green
Made 2 more changes: 1. PS11 has SLAVE_DIR and subsequently cache_dir in the wrong place, moved it back to where its supposed to be now 2. Let run_slave.py know that we do want to keep cache_dir around
I'm going crazy and losing track of where directories are. I think its right this time
The CQ bit was checked by hinoka@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hinoka@google.com/157073002/520001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for build/scripts/slave/chromium_commands.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file build/scripts/slave/chromium_commands.py
Hunk #1 FAILED at 269.
Hunk #2 succeeded at 289 (offset 2 lines).
1 out of 2 hunks FAILED -- saving rejects to file
build/scripts/slave/chromium_commands.py.rej
Patch: build/scripts/slave/chromium_commands.py
Index: scripts/slave/chromium_commands.py
diff --git build/scripts/slave/chromium_commands.py
build/scripts/slave/chromium_commands.py
index
9c8b82e023311b74aac87c0e6558e0d6e898d511..c44efa82211d3bd005525f7d97a86878d79447bd
100644
--- a/build/scripts/slave/chromium_commands.py
+++ b/build/scripts/slave/chromium_commands.py
@@ -269,6 +269,7 @@ class GClient(SourceBaseCommand):
self.no_gclient_revision = args.get('no_gclient_revision', False)
self.gclient_transitive = args.get('gclient_transitive')
self.gclient_jobs = args.get('gclient_jobs')
+ self.do_nothing = False
def start(self):
"""Start the update process.
@@ -287,8 +288,21 @@ class GClient(SourceBaseCommand):
self.sourcedatafile = os.path.join(self.builder.basedir,
self.srcdir,
".buildbot-sourcedata")
+ self.do_nothing = os.path.isfile(os.path.join(self.builder.basedir,
+ self.srcdir,
+ 'update.flag'))
+
+ d = defer.succeed(0)
+
+ if self.do_nothing:
+ # If bot update is run, we don't need to run the traditional update step.
+ msg = 'update.flag file found: bot_update has run and checkout is \n'
+ msg += 'already in a consistent state.\n'
+ msg += 'No actions will be performed in this step.'
+ self.sendStatus({'header': msg})
+ d.addCallback(self._sendRC)
+ return d
- d = defer.succeed(None)
# Do we need to clobber anything?
if self.mode in ("copy", "clobber", "export"):
d.addCallback(self.doClobber, self.workdir)
The CQ bit was checked by hinoka@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hinoka@google.com/157073002/600001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 157073002-600001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Pylint (424 files) (39.47s) failed
************* Module slave.bot_update
W0621:200,4:solutions_to_git: Redefining name 'path' from outer scope (line 19)
W0611: 8,0: Unused import json
************* Module slave.chromium_commands
W0201:293,4:GClient.start: Attribute 'do_nothing' defined outside __init__
Missing LGTM from an OWNER for these files:
build/slave/run_slave.py
Presubmit checks took 41.2s to calculate.
stuffs https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/master/f... File scripts/master/factory/commands.py (right): https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/master/f... scripts/master/factory/commands.py:938: 'root': '%(root:~src)s', I'm not sure if the default of 'src' is really good to have here, since it masks the fact that root was empty, and may not always be appropriate. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/master/f... scripts/master/factory/commands.py:942: 'revision': '%(revision:-)s', this should also pass through patch_url if it's defined https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... File scripts/slave/bot_update.py (right): https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:101: while tries < RETRIES: could also do for try in xrange(RETRIES): ... https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:110: buf = proc.stdout.read(1) can't we just do readline() calls? https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:119: break just do a return here https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:124: if code: then you don't need this check here https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:126: (' '.join(args), code, os.getcwd())) ... after XX tries ? https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:127: return code don't need this here either https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:228: print '%s detected in checking, deleting %s...' % (scm_dirname, build_dir), checking? checkout? https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:324: parse.add_option('-c', '--server', help='Rietveld server.') --server isn't really descriptive. Should be --rietveld_server or something like that. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:334: parse.add_option('-g', '--flag_file', default=path.join(os.getcwd(), These short options aren't super-meaningful... wdyt about just removing them all so that nothing starts depending on them? https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:344: slave = os.environ.get('BUILDBOT_SLAVENAME', socket.getfqdn().split('.')[0]) urk.... We need CLI options for these so we can use this in recipes without setting the env vars. why don't we just pass these as normal options anyway? they should just be %(mastername:-)s and %(buildername:-)s and %(slavename:-)s right? https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/ch... File scripts/slave/chromium_commands.py (right): https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/ch... scripts/slave/chromium_commands.py:294: self.srcdir, I'm assuming srcdir is actually 'build'?
https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/master/f... File scripts/master/factory/commands.py (right): https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/master/f... scripts/master/factory/commands.py:938: 'root': '%(root:~src)s', On 2014/02/08 01:31:31, iannucci wrote: > I'm not sure if the default of 'src' is really good to have here, since it masks > the fact that root was empty, and may not always be appropriate. Done. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/master/f... scripts/master/factory/commands.py:942: 'revision': '%(revision:-)s', On 2014/02/08 01:31:31, iannucci wrote: > this should also pass through patch_url if it's defined Done. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... File scripts/slave/bot_update.py (right): https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:101: while tries < RETRIES: On 2014/02/08 01:31:31, iannucci wrote: > could also do > > for try in xrange(RETRIES): > ... Done. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:110: buf = proc.stdout.read(1) On 2014/02/08 01:31:31, iannucci wrote: > can't we just do readline() calls? And rely on the system native newline semantics playing nicely with python? :/ I'd rather have as little things in the python stdout pipe as i can. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:119: break On 2014/02/08 01:31:31, iannucci wrote: > just do a return here Done. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:124: if code: On 2014/02/08 01:31:31, iannucci wrote: > then you don't need this check here Done. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:126: (' '.join(args), code, os.getcwd())) On 2014/02/08 01:31:31, iannucci wrote: > ... after XX tries > > ? Done. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:127: return code On 2014/02/08 01:31:31, iannucci wrote: > don't need this here either Done. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:228: print '%s detected in checking, deleting %s...' % (scm_dirname, build_dir), On 2014/02/08 01:31:31, iannucci wrote: > checking? checkout? Done. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:324: parse.add_option('-c', '--server', help='Rietveld server.') On 2014/02/08 01:31:31, iannucci wrote: > --server isn't really descriptive. Should be --rietveld_server or something like > that. Done. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:334: parse.add_option('-g', '--flag_file', default=path.join(os.getcwd(), On 2014/02/08 01:31:31, iannucci wrote: > These short options aren't super-meaningful... wdyt about just removing them all > so that nothing starts depending on them? But I like short option names :( I guess thats okay if bot_update.py should only ever be used by bots. https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/bo... scripts/slave/bot_update.py:344: slave = os.environ.get('BUILDBOT_SLAVENAME', socket.getfqdn().split('.')[0]) On 2014/02/08 01:31:31, iannucci wrote: > urk.... We need CLI options for these so we can use this in recipes without > setting the env vars. > > why don't we just pass these as normal options anyway? they should just be > %(mastername:-)s and %(buildername:-)s and %(slavename:-)s right? We get them as free always in the environ as part of annotated command, I don't see the need for the duplication? https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/ch... File scripts/slave/chromium_commands.py (right): https://chromiumcodereview.appspot.com/157073002/diff/730001/scripts/slave/ch... scripts/slave/chromium_commands.py:294: self.srcdir, On 2014/02/08 01:31:31, iannucci wrote: > I'm assuming srcdir is actually 'build'? Presumably, its used that way in all the other part of the script.
https://codereview.chromium.org/157073002/diff/730001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/730001/scripts/slave/bot_updat... scripts/slave/bot_update.py:110: buf = proc.stdout.read(1) On 2014/02/08 01:52:21, Ryan T. wrote: > On 2014/02/08 01:31:31, iannucci wrote: > > can't we just do readline() calls? > > And rely on the system native newline semantics playing nicely with python? :/ > I'd rather have as little things in the python stdout pipe as i can. oh, I see you're just copying bytes... how does that prevent weird interleaving? https://codereview.chromium.org/157073002/diff/730001/scripts/slave/bot_updat... scripts/slave/bot_update.py:344: slave = os.environ.get('BUILDBOT_SLAVENAME', socket.getfqdn().split('.')[0]) On 2014/02/08 01:52:21, Ryan T. wrote: > On 2014/02/08 01:31:31, iannucci wrote: > > urk.... We need CLI options for these so we can use this in recipes without > > setting the env vars. > > > > why don't we just pass these as normal options anyway? they should just be > > %(mastername:-)s and %(buildername:-)s and %(slavename:-)s right? > > We get them as free always in the environ as part of annotated command, I don't > see the need for the duplication? 1) They assume buildbot 2) Passing data via env vars when it's avoidable is a great way to shoot yourself in various sensitive parts of your anatomy. https://codereview.chromium.org/157073002/diff/770001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/770001/scripts/slave/bot_updat... scripts/slave/bot_update.py:100: tries = 0 don't need this var any more https://codereview.chromium.org/157073002/diff/770001/scripts/slave/bot_updat... scripts/slave/bot_update.py:101: for tries in xrange(RETRIES): this should be a singular word like 'attempt' https://codereview.chromium.org/157073002/diff/770001/scripts/slave/bot_updat... scripts/slave/bot_update.py:122: tries += 1 don't need this https://codereview.chromium.org/157073002/diff/770001/scripts/slave/bot_updat... scripts/slave/bot_update.py:321: parse.add_option('--patch_url') help text? "Optional URL to SVN patch"
Added fixes, passed green locally https://codereview.chromium.org/157073002/diff/730001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/730001/scripts/slave/bot_updat... scripts/slave/bot_update.py:110: buf = proc.stdout.read(1) On 2014/02/08 03:37:21, iannucci wrote: > On 2014/02/08 01:52:21, Ryan T. wrote: > > On 2014/02/08 01:31:31, iannucci wrote: > > > can't we just do readline() calls? > > > > And rely on the system native newline semantics playing nicely with python? :/ > > I'd rather have as little things in the python stdout pipe as i can. > > oh, I see you're just copying bytes... how does that prevent weird interleaving? It doesn't but unless we do multiple calls at a time there shouldn't be any weird interleaving...? https://codereview.chromium.org/157073002/diff/770001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/770001/scripts/slave/bot_updat... scripts/slave/bot_update.py:100: tries = 0 On 2014/02/08 03:37:21, iannucci wrote: > don't need this var any more Done. https://codereview.chromium.org/157073002/diff/770001/scripts/slave/bot_updat... scripts/slave/bot_update.py:101: for tries in xrange(RETRIES): On 2014/02/08 03:37:21, iannucci wrote: > this should be a singular word like 'attempt' Done. https://codereview.chromium.org/157073002/diff/770001/scripts/slave/bot_updat... scripts/slave/bot_update.py:122: tries += 1 On 2014/02/08 03:37:21, iannucci wrote: > don't need this Done. https://codereview.chromium.org/157073002/diff/770001/scripts/slave/bot_updat... scripts/slave/bot_update.py:321: parse.add_option('--patch_url') On 2014/02/08 03:37:21, iannucci wrote: > help text? > > "Optional URL to SVN patch" Done.
lgtm % env var https://codereview.chromium.org/157073002/diff/850001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/850001/scripts/slave/bot_updat... scripts/slave/bot_update.py:349: options.revision = os.environ.get('BUILDBOT_REVISION') nope?
The CQ bit was checked by hinoka@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hinoka@google.com/157073002/920001
Message was sent while issue was closed.
Change committed as 250236
Message was sent while issue was closed.
Passing by, added some comments. https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:100: for attempt in xrange(RETRIES): Passing RETRIES as a global variable could be avoided by changing the function signature like this: def call(cmd, retries=3) This is more consistent with the Popen signature and cleaner (*args arguments tend to be hard to document). https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:112: sys.stdout.write(buf) Isn't that horribly inefficient? I trust that you checked that stdout=sys.stdout and stderr=subprocess.STDOUT does produce ooo output, but I'm still surprised. https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:116: print '===Succeeded in %.1f mins===' % elapsed_time nit: units have no plural. Use 'min' instead of 'mins' https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:120: print You could have added \n in the previous line.
Message was sent while issue was closed.
https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:100: for attempt in xrange(RETRIES): On 2014/02/12 17:46:26, pgervais wrote: > Passing RETRIES as a global variable could be avoided by changing the function > signature like this: > > def call(cmd, retries=3) > > This is more consistent with the Popen signature and cleaner (*args arguments > tend to be hard to document). We just overloaded **kwargs in this function to pass directly to subprocess.Popen() in another CL, so that won't work as well :( The *arg construct was intentional (despite going against pythonic conventions) because our calls are generally short and call('a', 'b', 'c', 'd') looks more readable than call(['a', 'b', 'c', 'd']) by a tiny margin. https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:112: sys.stdout.write(buf) On 2014/02/12 17:46:26, pgervais wrote: > Isn't that horribly inefficient? I trust that you checked that stdout=sys.stdout > and stderr=subprocess.STDOUT does produce ooo output, but I'm still surprised. You'd think so, but its a pretty simple loop and our machines have at least 8 cores nominally, so this is pretty negligible, despite how inefficient it looks. Also its the only thing that works all the time, since stdout=sys.stdout will produce pretty undesireable results, where the "print" statements in main() gets interleaved/printed after the subprocess because the subprocess outputs to sys.stdout unbuffered but the python process itself buffers it outputs whenever it feels like it. We already do this in many places in various slave scripts because of the very issue. In fact it generally looks even more complicated, with threads and stuff. Basically pipes are weird. https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:116: print '===Succeeded in %.1f mins===' % elapsed_time On 2014/02/12 17:46:26, pgervais wrote: > nit: units have no plural. Use 'min' instead of 'mins' I intentionally said "mins" because everything except 1.0min should be plural (0.0 mins, 0.5 mins, 2.0mins), so it would be right most of the time. https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:120: print On 2014/02/12 17:46:26, pgervais wrote: > You could have added \n in the previous line. Noted, will fix in another CL
Message was sent while issue was closed.
https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:100: for attempt in xrange(RETRIES): On 2014/02/12 18:56:53, Ryan T. wrote: > On 2014/02/12 17:46:26, pgervais wrote: > > Passing RETRIES as a global variable could be avoided by changing the function > > signature like this: > > > > def call(cmd, retries=3) > > > > This is more consistent with the Popen signature and cleaner (*args arguments > > tend to be hard to document). > > We just overloaded **kwargs in this function to pass directly to > subprocess.Popen() in another CL, so that won't work as well :( There are at least two solutions to that problem: - def call(*args, retries=3, **kwargs): OR - def call(*args, **kwargs): and use kwargs.pop('retries') to remove the extra key prior to calling Popen. Plus Popen doesn't have that many keyword parameters, and they're not likely to change. Listing all of them in the function definition is thus possible. > The *arg construct was intentional (despite going against pythonic conventions) > because our calls are generally short and call('a', 'b', 'c', 'd') looks more > readable than call(['a', 'b', 'c', 'd']) by a tiny margin. I have often noticed that this tiny margin is not worth it, but it's my personal experience. You inevitably end up writing things like cmd = ['call', 'me'] print 'command: %s ' % ' '.join(cmd) call(*cmd) Thus you're forced to add an extra *, which I don't like (but again, this is me) https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:116: print '===Succeeded in %.1f mins===' % elapsed_time On 2014/02/12 18:56:53, Ryan T. wrote: > On 2014/02/12 17:46:26, pgervais wrote: > > nit: units have no plural. Use 'min' instead of 'mins' > > I intentionally said "mins" because everything except 1.0min should be plural > (0.0 mins, 0.5 mins, 2.0mins), so it would be right most of the time. My point was that you should write '2 min' not '2 mins', because 'min' is a unit, not an abbreviation. It's the same with other units: 2 m, 2 mph.
Message was sent while issue was closed.
https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... File scripts/slave/bot_update.py (right): https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:100: for attempt in xrange(RETRIES): FWIW, the construct being used here (pass all **kwargs straight through to subprocess.Popen) is the same system used throughout recipes. We're definitely not ever going to list all the arguments to Popen (there are 13 of them). We do use kwargs.pop() in a bunch of places, so we could/should probably use that here too. I don't feel strongly either way. As for call(1, 2, 3) vs call([1, 2, 3])... recipes uses both. I think we ended up standardizing on the latter (since it was effectively necessary for api.python) but again i don't feel strongly either way. https://codereview.chromium.org/157073002/diff/920001/scripts/slave/bot_updat... scripts/slave/bot_update.py:116: print '===Succeeded in %.1f mins===' % elapsed_time On 2014/02/12 19:17:22, pgervais wrote: > On 2014/02/12 18:56:53, Ryan T. wrote: > > On 2014/02/12 17:46:26, pgervais wrote: > > > nit: units have no plural. Use 'min' instead of 'mins' > > > > I intentionally said "mins" because everything except 1.0min should be plural > > (0.0 mins, 0.5 mins, 2.0mins), so it would be right most of the time. > > My point was that you should write '2 min' not '2 mins', because 'min' is a > unit, not an abbreviation. It's the same with other units: 2 m, 2 mph. +1. Any of 'min' or 'minutes' or 'minute(s)' are correct, 'mins' isn't really. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
