Log error message if os.rename fails

recipe_modules/bot_update/resources/bot_update.py

 #!/usr/bin/env python
 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 # TODO(hinoka): Use logging.
 
 import cStringIO
 import codecs
 import copy
@@ skipping 281 matching lines @@
   return solutions
 
 
 def remove(target):
   """Remove a target by moving it into build.dead."""
   dead_folder = path.join(BUILDER_DIR, 'build.dead')
   if not path.exists(dead_folder):
     os.makedirs(dead_folder)
   dest = path.join(dead_folder, uuid.uuid4().hex)
   print 'Marking for removal %s => %s' % (target, dest)
-  os.rename(target, dest)
+  try:
+    os.rename(target, dest)
+  except Exception as e:
+    print 'Error renaming %s to %s: %s' % (target, dest, str(e))

[ddoman1, 2016/10/25 21:33:08]

This would eat the exception and the execution flow would continue as if this
remove() call succeeded.

I believe that it would be more reasonable to interrupt the build with raising
the exception when an error occurred from os.rename() since os-level errors are
often critical and need someone to take a look at asap.

Also, I would be surprised if
#1. stderr from bot_update.py is not logged, and, therefore,
#2. bot_update.py needs to catch exceptions and print the message in stdout in
order for stderr messages to get logged.

https://build.chromium.org/p/chromium.win/builders/Win%20x64%20Builder/builds...

I expect that all stdout/stderr FDs should be connected to cloud tail or logdog
by remote_run (or at execution-level) such that each step doesn't have to stream
stderr to stdout by itself.
 
[stderr] link appears in step 'remote_run_result' only, and I haven't digged
into the recipe code to figure out how the stdout/stderr of each step are
logged. I would take a look at remote_run to find how it redirects the
stderr/stdout streams of the steps executed.

[agable, 2016/10/28 16:48:40]

You're absolutely right that we should be using the python logging library, and
reasonably sending things to stdout/stderr based on content. See the TODO at the
top of the file :)

However, for now, this `print` is in-line with the rest of the output in this
file. We should file a bug to convert the whole file over, but I wouldn't block
this CL on adopting best practices when it would be out-of-sync with the rest of
the file.

For what it's worth, all stdout and stderr from all recipe scripts is logged,
and is annotated according to stream (in buildbot logs, stdout is black while
stderr is red). So this will show up as black, which is unfortunate, but it will
show up and get logged and get streamed to logdog.

(But also yes, this should probably re-raise the exception too.)

[katthomas, 2016/10/28 18:41:13]

My bad on the raise. Done now.

 
 
 def ensure_no_checkout(dir_names):
   """Ensure that there is no undesired checkout under build/."""
   build_dir = os.getcwd()
   has_checkout = any(path.exists(path.join(build_dir, dir_name, '.git'))
                      for dir_name in dir_names)
   if has_checkout:
     for filename in os.listdir(build_dir):
       deletion_target = path.join(build_dir, filename)
@@ skipping 783 matching lines @@
     # download patch failure is still an infra problem.
     if e.code == 3:
       # Patch download problem.
       return 87
     # Genuine patch problem.
     return 88
 
 
 if __name__ == '__main__':
   sys.exit(main())