Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2471023002:
Make isolate.py fail if Go isolate fails (Closed)
|
Created:
4 years, 1 month ago by djd-OOO-Apr2017 Modified:
4 years, 1 month ago CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionMake isolate.py fail if Go isolate fails
This change removes the fallback to python if the Go isolate client is
missing or fails to execute correctly (or if its version is wrong:
isolate has been at 3.3 since Nov 2015).
This is the first step in attempting to remove this script altogether.
R=tansell@chromium.org, vadimsh@chromium.org
BUG=
Committed: https://chromium.googlesource.com/chromium/tools/build/+/30a5ba05fb2522d5458fb05e9d5353298d3cd807
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove call to os.Access #
Total comments: 1
Messages
Total messages: 19 (11 generated)
The CQ bit was checked by djd@chromium.org to run a CQ dry run
Dry run: 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 commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
lgtm
The CQ bit was checked by djd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2471023002/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/isolate/resources/isolate.py (right): https://codereview.chromium.org/2471023002/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/isolate/resources/isolate.py:27: return None actually, return something !=0 here, otherwise it becomes zero exit code for the process :-/ Or remove os.access check completely and let subprocess.call to fail itself.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/01 at 23:40:02, vadimsh wrote: > https://codereview.chromium.org/2471023002/diff/1/scripts/slave/recipe_module... > File scripts/slave/recipe_modules/isolate/resources/isolate.py (right): > > https://codereview.chromium.org/2471023002/diff/1/scripts/slave/recipe_module... > scripts/slave/recipe_modules/isolate/resources/isolate.py:27: return None > actually, return something !=0 here, otherwise it becomes zero exit code for the process :-/ > > Or remove os.access check completely and let subprocess.call to fail itself. Good call. I'll do the second, since it's less code.
LGTM, one small suggestion. https://codereview.chromium.org/2471023002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/isolate/resources/isolate.py (right): https://codereview.chromium.org/2471023002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/isolate/resources/isolate.py:27: return subprocess.call([exe] + args) Maybe you want to use subprocess.check_call ?
On 2016/11/02 at 07:13:11, tansell wrote: > LGTM, one small suggestion. > > https://codereview.chromium.org/2471023002/diff/20001/scripts/slave/recipe_mo... > File scripts/slave/recipe_modules/isolate/resources/isolate.py (right): > > https://codereview.chromium.org/2471023002/diff/20001/scripts/slave/recipe_mo... > scripts/slave/recipe_modules/isolate/resources/isolate.py:27: return subprocess.call([exe] + args) > Maybe you want to use subprocess.check_call ? I think not, but correct if my understanding is wrong: with check_call, if the isolate command returned a non-zero status code (for example, an upload failed) then an error is raised. That would cause this script to exit with an ugly stack trace which will distract from what actually happened.
The CQ bit was checked by djd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org Link to the patchset: https://codereview.chromium.org/2471023002/#ps20001 (title: "Remove call to os.Access")
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 ========== Make isolate.py fail if Go isolate fails This change removes the fallback to python if the Go isolate client is missing or fails to execute correctly (or if its version is wrong: isolate has been at 3.3 since Nov 2015). This is the first step in attempting to remove this script altogether. R=tansell@chromium.org, vadimsh@chromium.org BUG= ========== to ========== Make isolate.py fail if Go isolate fails This change removes the fallback to python if the Go isolate client is missing or fails to execute correctly (or if its version is wrong: isolate has been at 3.3 since Nov 2015). This is the first step in attempting to remove this script altogether. R=tansell@chromium.org, vadimsh@chromium.org BUG= Committed: https://chromium.googlesource.com/chromium/tools/build/+/30a5ba05fb2522d5458f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/tools/build/+/30a5ba05fb2522d5458f... |
