Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1757733004:
clang upload.sh: don't download Clang binaries from clang_upload bots. (Closed)
|
Created:
4 years, 9 months ago by krasin Modified:
4 years, 9 months ago CC:
chromium-reviews, yunlian, eugenis+clang_chromium.org, glider+clang_chromium.org, dmikurube+clang_chromium.org, ukai+watch_chromium.org, kcc2, pcc1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionclang upload.sh: don't download Clang binaries from clang_upload bots.
Clang Upload trybots are used to build the new Clang
binaries. Right now, they fail, because they try to download
the not-yet-built clang revision during runhooks.
In this CL a check into update.py is added to skip the
download if it's running from Clang Upload trybot. LLVM_FORCE_LOCAL_BUILD env var is used for that.
BUG=578306
Committed: https://crrev.com/9321234d5ec76fadeacefd38591b93420dc62b67
Cr-Commit-Position: refs/heads/master@{#379989}
Patch Set 1 #Patch Set 2 : use env #
Total comments: 1
Patch Set 3 : LLVM_FORCE_LOCAL_BUILD #
Total comments: 2
Messages
Total messages: 33 (8 generated)
Description was changed from ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-build clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. A known slave directory is used for the detection. BUG=578306 ========== to ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-built clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. A known slave directory is used for the detection. BUG=578306 ==========
Description was changed from ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-built clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. A known slave directory is used for the detection. BUG=578306 ========== to ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-built clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. A known slave directory is used for the detection. BUG=578306 ==========
krasin@chromium.org changed reviewers: + hans@chromium.org, krasin@chromium.org, thakis@chromium.org
Nico, Hans, please, review this fix for Clang Upload bots. That fixed the Linux trybot, see https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... and exposes new issues in Mac bot, see https://bugs.chromium.org/p/chromium/issues/detail?id=578306#c49
> In this CL a check into update.py is added to skip the > download if it's running from Clang Upload trybot. A known > slave directory is used for the detection. Looking at the dir names seems pretty hacky, and also makes it inconvenient to test the behaviour locally. An environment variable or flag would be better, I think. For our ToT bots we set the LLVM_FORCE_HEAD_REVISION environment variable, for example.
On 2016/03/02 21:41:42, hans wrote: > > In this CL a check into update.py is added to skip the > > download if it's running from Clang Upload trybot. A known > > slave directory is used for the detection. > > Looking at the dir names seems pretty hacky, and also makes it inconvenient to > test the behaviour locally. > > An environment variable or flag would be better, I think. For our ToT bots we > set the LLVM_FORCE_HEAD_REVISION environment variable, for example. Relying on an env var was my first call, but then I opened https://crbug.com/582737 and reconsidered. Let me know if you want to go with an env var. In this case, I will need to convert the bot back to GYP (not that it does anything for us, it's just for gclient sync / runhooks).
On 2016/03/02 21:44:13, krasin1 wrote: > Relying on an env var was my first call, but then I opened > https://crbug.com/582737 and reconsidered. In the one, it seems the problem is that update.py gives inconsistent results because the environment is different in different build steps. But for this upload functionality, we will only ever need the environment variable in one step - the one the uploads, so maybe it's not really an issue?
On 2016/03/02 21:48:12, hans wrote: > On 2016/03/02 21:44:13, krasin1 wrote: > > Relying on an env var was my first call, but then I opened > > https://crbug.com/582737 and reconsidered. > > In the one, it seems the problem is that update.py gives inconsistent results > because the environment is different in different build steps. > > But for this upload functionality, we will only ever need the environment > variable in one step - the one the uploads, so maybe it's not really an issue? The problem is that GN-based bots don't pass an env var into update.py. And GYP bots are deprecated (not strongly, but still). I really don't have a strong opinion here, just a slight preference to reduce the number of back-and-forth transitions between GYP and GN buildbot types.
On 2016/03/02 21:50:35, krasin1 wrote: > On 2016/03/02 21:48:12, hans wrote: > > On 2016/03/02 21:44:13, krasin1 wrote: > > > Relying on an env var was my first call, but then I opened > > > https://crbug.com/582737 and reconsidered. > > > > In the one, it seems the problem is that update.py gives inconsistent results > > because the environment is different in different build steps. > > > > But for this upload functionality, we will only ever need the environment > > variable in one step - the one the uploads, so maybe it's not really an issue? > > The problem is that GN-based bots don't pass an env var into update.py. :-( I thought they would still need to run gclient runhooks and we could pass an env var there? > And GYP bots are deprecated (not strongly, but still). Calling gyp deprecated sounds a bit premature, I think :-) > I really don't have a strong opinion here, just a slight preference to reduce > the number of back-and-forth transitions between GYP and GN buildbot types. I think checking the path name is not a good enough solution. It needs to be an environment variable or a flag. If it's really impossible to pass that into the step that runs update.py, I suppose we'd have to have a separate step for running update.py that makes it possible.
do we need any of the hooks on the upload bots? can we change the bot config to not run runhooks?
On 2016/03/02 22:16:11, Nico wrote: > do we need any of the hooks on the upload bots? can we change the bot config to > not run runhooks? the linux bot needs runhooks to download binutils which is used for building the gold plugin, IIRC
Description was changed from ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-built clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. A known slave directory is used for the detection. BUG=578306 ========== to ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-built clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. LLVM_DOWNLOAD_NOT_NEEDED env var is used for that. BUG=578306 ==========
It seems that nobody has a better idea than using GYP + env var, so I have changed update.py to rely on LLVM_DOWNLOAD_NOT_NEEDED. Please, take a look. I will also send a corresponding change to the build side to convert the buildbots to GYP and to insert the env var.
https://codereview.chromium.org/1757733004/diff/20001/tools/clang/scripts/upd... File tools/clang/scripts/update.py (right): https://codereview.chromium.org/1757733004/diff/20001/tools/clang/scripts/upd... tools/clang/scripts/update.py:728: 'LLVM_DOWNLOAD_NOT_NEEDED is found in the environment') can we call it LLVM_FORCE_LOCAL_BUILD instead and tie it in with the corresponding flag?
On 2016/03/04 19:52:28, hans wrote: > https://codereview.chromium.org/1757733004/diff/20001/tools/clang/scripts/upd... > File tools/clang/scripts/update.py (right): > > https://codereview.chromium.org/1757733004/diff/20001/tools/clang/scripts/upd... > tools/clang/scripts/update.py:728: 'LLVM_DOWNLOAD_NOT_NEEDED is found in the > environment') > can we call it LLVM_FORCE_LOCAL_BUILD instead and tie it in with the > corresponding flag? Done.
Description was changed from ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-built clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. LLVM_DOWNLOAD_NOT_NEEDED env var is used for that. BUG=578306 ========== to ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-built clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. LLVM_FORCE_LOCAL_BUILD env var is used for that. BUG=578306 ==========
The build/ part of the change is published: https://codereview.chromium.org/1766073002/ Please, review both.
https://codereview.chromium.org/1757733004/diff/40001/tools/clang/scripts/upd... File tools/clang/scripts/update.py (right): https://codereview.chromium.org/1757733004/diff/40001/tools/clang/scripts/upd... tools/clang/scripts/update.py:728: 'LLVM_FORCE_LOCAL_BUILD is found in the environment') doesn't this mean the tot bots will stop building clang since they now exit here?
https://codereview.chromium.org/1757733004/diff/40001/tools/clang/scripts/upd... File tools/clang/scripts/update.py (right): https://codereview.chromium.org/1757733004/diff/40001/tools/clang/scripts/upd... tools/clang/scripts/update.py:728: 'LLVM_FORCE_LOCAL_BUILD is found in the environment') On 2016/03/05 00:11:12, Nico wrote: > doesn't this mean the tot bots will stop building clang since they now exit > here? ToT bots use LLVM_FORCE_HEAD_REVISION. And LLVM_FORCE_LOCAL_BUILD is not currently used.
Hans, Nico, are there any more objections to the CL?
Friendly ping. Nico, please, take a look.
Sorry, I'm a bit behind on reviews, as you can tell. I'm catching up, though, and things should be faster now. If this is the approach you want to go with, I'm fine with it. I want to make one last attempt to sell my suggestion here (which might not be practicable, but I haven't seen why yet). The current approach to me seems to match "we do a bunch of work, some of which we don't want, so do more work at the end to undo some of the work we did at the start" pattern. When I see these, I generally feel "do less work in the first place" is a better fix most of the time. In this case, instead of running all the hooks and then adding a flag that lets one of the hooks that we don't want skip some of what it normally does, not running the hooks except for the one we need might be better. Then update.py won't have to grow this flag, and the bot-side config work is about the same in both cases (either change the config to not run hooks but to run that one script, or add code to add an env var). As I said, in the end I leave it to you, and if you think that wasn't convincing, then the current CL lgtm. Re: > the linux bot needs runhooks to download binutils which is used for building the > gold plugin, IIRC The bot could have a build step that downloads just binutils and doesn't call the other hooks maybe?
On 2016/03/08 21:14:54, Nico wrote:
> Sorry, I'm a bit behind on reviews, as you can tell. I'm catching up, though,
> and things should be faster now.
>
> If this is the approach you want to go with, I'm fine with it. I want to make
> one last attempt to sell my suggestion here (which might not be practicable,
but
> I haven't seen why yet).
>
> The current approach to me seems to match "we do a bunch of work, some of
which
> we don't want, so do more work at the end to undo some of the work we did at
the
> start" pattern. When I see these, I generally feel "do less work in the first
> place" is a better fix most of the time. In this case, instead of running all
> the hooks and then adding a flag that lets one of the hooks that we don't want
> skip some of what it normally does, not running the hooks except for the one
we
> need might be better. Then update.py won't have to grow this flag, and the
> bot-side config work is about the same in both cases (either change the config
> to not run hooks but to run that one script, or add code to add an env var).
>
> As I said, in the end I leave it to you, and if you think that wasn't
> convincing, then the current CL lgtm.
I agree with you in general ("do less" is better), but I would like to have the
basic solution (even if it requires a small number of complications) to be
working first, and iterate / cleanup later.
In particular, it's due to my internal accounting books, which should that I
spend a bit too much on Chromium infrastructure cleanups. While I plan to
continue doing so, keeping some time budget is probably wise in the medium /
long term. I have filed https://crbug.com/593155 to track that, but it's a low
priority to me.
>
>
> Re:
>
> > the linux bot needs runhooks to download binutils which is used for building
> the
> > gold plugin, IIRC
>
> The bot could have a build step that downloads just binutils and doesn't call
> the other hooks maybe?
The CQ bit was checked by krasin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1757733004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757733004/40001
Message was sent while issue was closed.
Description was changed from ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-built clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. LLVM_FORCE_LOCAL_BUILD env var is used for that. BUG=578306 ========== to ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-built clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. LLVM_FORCE_LOCAL_BUILD env var is used for that. BUG=578306 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-built clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. LLVM_FORCE_LOCAL_BUILD env var is used for that. BUG=578306 ========== to ========== clang upload.sh: don't download Clang binaries from clang_upload bots. Clang Upload trybots are used to build the new Clang binaries. Right now, they fail, because they try to download the not-yet-built clang revision during runhooks. In this CL a check into update.py is added to skip the download if it's running from Clang Upload trybot. LLVM_FORCE_LOCAL_BUILD env var is used for that. BUG=578306 Committed: https://crrev.com/9321234d5ec76fadeacefd38591b93420dc62b67 Cr-Commit-Position: refs/heads/master@{#379989} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9321234d5ec76fadeacefd38591b93420dc62b67 Cr-Commit-Position: refs/heads/master@{#379989}
Message was sent while issue was closed.
(this can be reverted now, right?)
Message was sent while issue was closed.
On 2016/03/09 05:00:32, Nico wrote: > (this can be reverted now, right?) I plan to, pending on https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... results (I will know the answer in 3-4 mins)
Message was sent while issue was closed.
It seems to work. Reverting this one...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1777793003/ by krasin@chromium.org. The reason for reverting is: Not needed anymore. https://codereview.chromium.org/1776893002/ was a better approach.. |
