Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2722113003:
Android Loading Tools: use computed adb path for cert install (Closed)
|
Created:
3 years, 9 months ago by mattcary Modified:
3 years, 9 months ago CC:
chromium-reviews, mikecase+watch_chromium.org, gabadie+watch_chromium.org, agrieve+watch_chromium.org, jbudorick+watch_chromium.org, lizeb+watch-android-loading_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid Loading Tools: use computed adb path for cert install
adb_install_cert will use the system adb path if an adb path isn't
given. This will cause an adb error if whatever version of adb is installed
outside of chromium doesn't match what's in third_party/android_tools, as that
version of adb is used elsewhere.
Review-Url: https://codereview.chromium.org/2722113003
Cr-Commit-Position: refs/heads/master@{#454224}
Committed: https://chromium.googlesource.com/chromium/src/+/7d02460714d56659a2901396bcf4e382d58fda2a
Patch Set 1 #
Total comments: 2
Patch Set 2 : remove stray edit #Messages
Total messages: 16 (5 generated)
mattcary@chromium.org changed reviewers: + alexilin@chromium.org, lizeb@chromium.org
On 2017/03/01 15:57:44, mattcary wrote: I'm not against the patch, but generally speaking having the system adb being something different from Chrome's adb is going to generate weird failures in semi-random places, from my experience. So even though this patch solves one issues, I'm not sure it solves the broader one. wdyt?
On 2017/03/01 16:01:18, Benoit L wrote: > On 2017/03/01 15:57:44, mattcary wrote: > > I'm not against the patch, but generally speaking having the system adb being > something different from Chrome's adb is going to generate weird failures in > semi-random places, from my experience. So even though this patch solves one > issues, I'm not sure it solves the broader one. > > wdyt? After running into the weird failures you mention, I removed my system adb entirely (no adb in my path anywhere!). This patch made the spec prefetch data-generation pipeline work in that case. I think it's unlikely that anything will use the Chrome adb only if the system adb doesn't exist, so I believe this fixes the weird failures for this pipeline at least. So I think it's getting pretty close to a good state.
On 2017/03/01 16:06:36, mattcary wrote: > On 2017/03/01 16:01:18, Benoit L wrote: > > On 2017/03/01 15:57:44, mattcary wrote: > > > > I'm not against the patch, but generally speaking having the system adb being > > something different from Chrome's adb is going to generate weird failures in > > semi-random places, from my experience. So even though this patch solves one > > issues, I'm not sure it solves the broader one. > > > > wdyt? > > After running into the weird failures you mention, I removed my system adb > entirely (no adb in my path anywhere!). This patch made the spec prefetch > data-generation pipeline work in that case. I think it's unlikely that anything > will use the Chrome adb only if the system adb doesn't exist, so I believe this > fixes the weird failures for this pipeline at least. So I think it's getting > pretty close to a good state. Thanks for the answer, lgtm.
On 2017/03/01 16:06:36, mattcary wrote: > On 2017/03/01 16:01:18, Benoit L wrote: > > On 2017/03/01 15:57:44, mattcary wrote: > > > > I'm not against the patch, but generally speaking having the system adb being > > something different from Chrome's adb is going to generate weird failures in > > semi-random places, from my experience. So even though this patch solves one > > issues, I'm not sure it solves the broader one. > > > > wdyt? > > After running into the weird failures you mention, I removed my system adb > entirely (no adb in my path anywhere!). This patch made the spec prefetch > data-generation pipeline work in that case. I think it's unlikely that anything > will use the Chrome adb only if the system adb doesn't exist, so I believe this > fixes the weird failures for this pipeline at least. So I think it's getting > pretty close to a good state. Thanks for the answer, lgtm.
lgtm Thanks for fixing this issue at least for spec prefetch pipeline. I'm wondering if there is any doc where we can put a warning for developers about a possible conflict between system adb and Chrome's adb.
https://codereview.chromium.org/2722113003/diff/1/tools/android/loading/devic... File tools/android/loading/device_setup.py (right): https://codereview.chromium.org/2722113003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:219: #Set up WPR server and device forwarder. nit: remove this from the diff.
https://codereview.chromium.org/2722113003/diff/1/tools/android/loading/devic... File tools/android/loading/device_setup.py (right): https://codereview.chromium.org/2722113003/diff/1/tools/android/loading/devic... tools/android/loading/device_setup.py:219: #Set up WPR server and device forwarder. On 2017/03/02 09:43:08, Benoit L wrote: > nit: remove this from the diff. oops, thanks
On 2017/03/01 17:10:24, alexilin wrote: > lgtm > > Thanks for fixing this issue at least for spec prefetch pipeline. > I'm wondering if there is any doc where we can put a warning for developers > about a possible conflict between system adb and Chrome's adb. I don't know. Probably it would be more useful to make the error message a little more verbose.
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org, alexilin@chromium.org Link to the patchset: https://codereview.chromium.org/2722113003/#ps20001 (title: "remove stray edit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488448587847850,
"parent_rev": "639e229373e9cb69c1259648cedc70a5841173e6", "commit_rev":
"7d02460714d56659a2901396bcf4e382d58fda2a"}
Message was sent while issue was closed.
Description was changed from ========== Android Loading Tools: use computed adb path for cert install adb_install_cert will use the system adb path if an adb path isn't given. This will cause an adb error if whatever version of adb is installed outside of chromium doesn't match what's in third_party/android_tools, as that version of adb is used elsewhere. ========== to ========== Android Loading Tools: use computed adb path for cert install adb_install_cert will use the system adb path if an adb path isn't given. This will cause an adb error if whatever version of adb is installed outside of chromium doesn't match what's in third_party/android_tools, as that version of adb is used elsewhere. Review-Url: https://codereview.chromium.org/2722113003 Cr-Commit-Position: refs/heads/master@{#454224} Committed: https://chromium.googlesource.com/chromium/src/+/7d02460714d56659a2901396bcf4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/7d02460714d56659a2901396bcf4... |
