New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 773182 link

Starred by 1 user
Status: Assigned
Owner:
thakis@chromium.org
Cc:
█ phajdan.jr@chromium.org
dpranke@chromium.org
█ mbjorge@chromium.org
jbudorick@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment

It is difficult to change some scripts in depot_tools

Project Member Reported by thakis@chromium.org, Oct 10 2017 
I landed some change to download_from_google_storage.py in depot_tools (ttps://chromium-review.googlesource.com/706915). Among other things, it removed a now-unused parameter from a function.

This apparently broke syncing on chrome/android, because some scripts (mostly in src/build/android, but also one in src/tools) import that file and call a function therein, instead of calling it as a subprocess like the rest of the world (https://cs.chromium.org/search/?q=download_from_google_storage%5C(+-file:third_party/depot_tools+&sq=package:chromium&type=cs).

So now devs had broken builds (because `gclient` autoupdates, and they got the autoupdated download_from_google_storage), but bots apparently use the pinned devtools so I didn't know that I had broken this until someone pinged me hours later.

I then tried to update these scripts here https://chromium-review.googlesource.com/c/chromium/src/+/708234 but that didn't pass presubmit because presubmit used pinned depot_tools. So I tried updating pinned depot_tools in the same CL, which made local presubmits happy, but chromium_presubmit still failed.

At this point I gave up and restored the parameter I had removed (https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/707937/).


This is not a good situation. I'm not 100% sure what exactly went wrong, though. My list:

1. Files outside of depot_tools shouldn't call stuff in depot_tools except as subprocesses. That way, only argparse / optparse is API, and not every single function. (src/build/android owners: Please remove direct calls to stuff in depot_tools)

2. Bots and devs shouldn't use different depot_tools versions – gclient runhooks should probably try to call functions in third_party/depot_tools instead of just from PATH first? (Not sure). Similarly, either `gclient` should only autoupdate to the pinned version on bots, instead of to HEAD? (This sounds like a pretty good idea actually?)

3. Having both a "global" depot_tools and third_party/depot_tools is as confusing as I expected it to be, and I knew about it. issue 730686 started as P1 but was downgraded to P2 and has seen zero action. It should probably have higher priority.

Comment 1 by mbjorge@chromium.org, Oct 10 2017 

At least on the chromecast side, I believe what happened is that our bots put our global, auto-updated-to-master depot_tools on the PYTHONPATH explicitly all the time

So even though the new find_depot_tools helper looks for the DEPS'd version first, since it appends it to the sys.path, the scripts calling into the download_from_google_storage.py APIs directly were getting the master version instead of the pinned version

Comment 2 by dpranke@chromium.org, Oct 10 2017

Cc: phajdan.jr@chromium.org
Status: Available (was: Untriaged)
your find_depot_tools should put the directory at the front of the path, not at the end; that might fix things.

Otherwise, I think thakis' diagnosis is right and we should fix the issues we find.
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 10 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5064a49489a271975179a5341025ec2dafc9d8fd

commit 5064a49489a271975179a5341025ec2dafc9d8fd
Author: Mike Bjorge <mbjorge@chromium.org>
Date: Tue Oct 10 21:36:39 2017

find_depot_tools: put DEPS'd depot_tools first

Update find_depot_tools to put the pinned depot_tools from the DEPS file
first on the sys.path.

If a user/bot has depot_tools on their path already (likely pointing to
a global, auto-updated-to-master version), the scripts will still want
to find the pinned version, in case APIs change.

Bug: 773182
Change-Id: I26a55a5a463462911cd3324ed8c31dc9685144c0
Reviewed-on: https://chromium-review.googlesource.com/710315
Reviewed-by: Daniel Jacques <dnj@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Mike Bjorge <mbjorge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507779}
[modify] https://crrev.com/5064a49489a271975179a5341025ec2dafc9d8fd/build/find_depot_tools.py
Project Member

Comment 4 by sheriffbot@chromium.org, Oct 11

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by pkl@chromium.org, Oct 15

Owner: mbjorge@chromium.org
Status: Assigned (was: Untriaged)
mbjorge: Did your last submit fix this issue completely? If not, what else needs to happen?

Comment 6 by mbjorge@chromium.org, Oct 15 

I think my last submit fixed the immediate term issue that had caused issue on the chromecast side, but I think 1, 2, & 3 from the original description are likely still valid.

(Though I haven't been on a relevant team for ~1 year now, so it's possible things have gotten fixed that I'm not aware of)

Comment 7 by mbjorge@chromium.org, Oct 15

Owner: thakis@chromium.org
Going to pass back to thakis@ since he'll have a better idea of the state of things than I do
►

Sign in to add a comment