Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2448983004:
Log error message if os.rename fails (Closed)
|
Created:
4 years, 1 month ago by katthomas Modified:
4 years, 1 month ago Reviewers:
agable CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionLog error message if os.rename fails
R=agable@google.com
BUG=659178
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/b7c80888a6f74eca501c1dfa78a7f6074bd94c70
Patch Set 1 #
Total comments: 3
Patch Set 2 : Log error message if os.rename fails #Messages
Total messages: 12 (6 generated)
Description was changed from ========== Log error message if os.rename fails R=agable@google.com BUG=659178 ========== to ========== Log error message if os.rename fails R=agable@google.com BUG=659178 ==========
katthomas@google.com changed reviewers: + agable@chromium.org - agable@google.com
ddoman@google.com changed reviewers: + ddoman@google.com
https://codereview.chromium.org/2448983004/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2448983004/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:305: print 'Error renaming %s to %s: %s' % (target, dest, str(e)) 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.
ddoman@google.com changed reviewers: - ddoman@google.com
https://codereview.chromium.org/2448983004/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2448983004/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:305: print 'Error renaming %s to %s: %s' % (target, dest, str(e)) On 2016/10/25 at 21:33:08, ddoman1 wrote: > 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. 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.)
PTAL https://codereview.chromium.org/2448983004/diff/1/recipe_modules/bot_update/r... File recipe_modules/bot_update/resources/bot_update.py (right): https://codereview.chromium.org/2448983004/diff/1/recipe_modules/bot_update/r... recipe_modules/bot_update/resources/bot_update.py:305: print 'Error renaming %s to %s: %s' % (target, dest, str(e)) On 2016/10/28 16:48:40, agable wrote: > On 2016/10/25 at 21:33:08, ddoman1 wrote: > > 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. > > 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.) My bad on the raise. Done now.
lgtm
The CQ bit was checked by katthomas@google.com
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 ========== Log error message if os.rename fails R=agable@google.com BUG=659178 ========== to ========== Log error message if os.rename fails R=agable@google.com BUG=659178 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/b7c80888a6f74e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/b7c80888a6f74e... |
