Remove use of GYP_DEFINES, LLVM_FORCE_HEAD_REVISION from tools/clang/scripts/update.py |
|||||||||||||
Issue descriptionNow that gclient supports conditionals and args properly, we can replace our use of GYP_DEFINES and LLVM_FORCE_HEAD_REVISION in update.py and the bot_configs with proper flags that allow us to select either the HEAD revision for ClangToT builds or the pinned version for normal builds. We can also generalize this to a third revision for the angle test bots if needed.
,
Aug 18 2017
,
Aug 27 2017
Any progress on this? In Issue 727437 all of the work is complete; waiting for a fix for this issue in order to be able to pin Clang and the compiler options. Thanks.
,
Aug 29 2017
We're making some progress on this, but it's not really ready to be deployed yet and probably won't be for at least another week or two. Given that we can't pin //build files, I've forgotten what we decided to do there. I have a vague memory that Nico thought that pinning clang for deqp separately wouldn't be viable, as he didn't want to maintain separate //build flags as well. It seems like the options were/are: 1) Pin just the version of clang for deqp, and trust that that'll be sufficient most of the time (i.e., don't support branches in the build configs for the two versions). 2) Pin a deqp version of clang and also support branches in the build configs. 3) Do neither, and just make sure the main version of clang works for deqp. It would also help to clarify what we want to do in the short and long terms. In the short term, I'm fine with any of these, though 3) is obviously preferable for both the short and long term to me.
,
Aug 29 2017
In our last meeting I thought Nico said it wouldn't be too difficult to maintain a few versions of the base build flags and choose among them with a GN variable. So I would like to have (2) as an option for the situation where the dEQP bots break deeply after an update to both Clang and the build flags. Thanks.
,
Aug 29 2017
Okay, that's fine.
,
Aug 29 2017
comment 5 is what I remember too, fwiw :-) (with "a few versions" being "up to three": pinned regular clang, trunk clang (we already have these 2 today) and every now and then a deqp clang)
,
Aug 29 2017
Yup, "a few" == "three".
,
Sep 16 2017
Hi, is this mechanism ready to be deployed yet? We've done all the work we can on Issue 727437 (splitting off the GPU team's drawElements tests onto their own bots) and would like to close it out by making those bots build with a pinned version of Clang. Thanks.
,
Sep 21 2017
Uploaded https://chromium-review.googlesource.com/c/chromium/src/+/677286 . More CLs likely to follow for remaining issues. Ken, I'm not sure if I fully understand what you're trying to do, so more context - a doc, or VC, might help.
,
Sep 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8549a3a5fd5e76ac1e5443dde72eb4f00f1ed155 commit 8549a3a5fd5e76ac1e5443dde72eb4f00f1ed155 Author: Paweł Hajdan, Jr <phajdan.jr@chromium.org> Date: Fri Sep 22 08:30:45 2017 Remove already no-op --if-needed code from clang's update.py Bug: 756686 Change-Id: I2d003007c92c9f5cb7f7720bc8c1248104f549c9 Reviewed-on: https://chromium-review.googlesource.com/677286 Reviewed-by: Hans Wennborg <hans@chromium.org> Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org> Cr-Commit-Position: refs/heads/master@{#503688} [modify] https://crrev.com/8549a3a5fd5e76ac1e5443dde72eb4f00f1ed155/tools/clang/scripts/update.py
,
Sep 22 2017
re: comment #10, let me try to provide the context: We currently have two different configurations of clang builders in Chromium: building using clang with the normal pinned version, and building using tip-of-tree Clang, which is gated by the LLVM_FORCE_HEAD_REVISION env var. The dEQP test suites use c++ exceptions, and after a long discussion w/ kbr@ and thakis@ over whether we wanted to require clang rolls to make sure they don't break the dEQP suites, we agreed to support a mechanism by which we'd allow a second pinned revision for clang for the bots or users that might want to run that test suite; since we need to support two settings, a third wasn't much additional work. So, we need some way to specify via gclient conditionals which of the three configurations we want, such that the right things then get passed into the update.py script, and so we can get rid of the LLVM_FORCE_HEAD_REVISION env var, and make sure which configuration we have propagates into the GN args as well so that we can (optionally) pass different build flags as needed. It looks like the update script still has a dependency on GYP_DEFINES to look for "OS=android" that further (optionally?) controls how to build clang at ToT. We need to get rid of that as well. Does that answer your question(s)?
,
Oct 20 2017
thakis@ just mentioned that the switch is going to be made soon to use Clang on Windows again. Before that happens we *must* have a way to pin both Clang, and the default compiler arguments, to an earlier version on some of the Windows bots -- see Issue 727437 . Otherwise a future Clang roll is likely to break the ANGLE project's trybots, which compile the dEQP test suite with exception support turned on. This is the only test suite on the Chromium waterfall which uses exception support, and we've had agreement that we won't block Clang rolls due to breakage of this suite. This means we have to have a way to pin Clang. Could I please ask for a status update on this bug? Upgrading to P1 to indicate the urgency, but recategorizing as a feature.
,
Oct 20 2017
I think the GPU FYI bots are currently pinned to msvc, so they should be not affected by the switch for now. So we should definitely figure this out, but it's also not "must do by Monday" :-)
,
Oct 20 2017
Ah, right, I'd forgotten that the dEQP bots were switched to use MSVC in Issue 727437 . The rest of the chromium.gpu.fyi bots were switched back to use the standard Chromium build configuration in that bug.
,
Oct 20 2017
It's also only "must do before we try to land a change that breaks exceptions and don't want to roll it back" ;).
,
Nov 17 2017
,
Nov 17 2017
,
Nov 17 2017
Dirk, can you please find an owner for this? We're ready to put it to use on the ANGLE tree.
,
Nov 21 2017
I don't really have anyone who can work on this who isn't already working on something more urgent, so I can't assign this to someone with a reasonable ETA. That said, there are a number of uses of gclient conditions in DEPS now, and so it probably wouldn't be hard for someone not in Ops to take a stab at this.
,
Nov 21 2017
let me know your thoughts?
,
Feb 2 2018
,
Apr 13 2018
I'm disappointed that Issue 786460 was resolved by switching over the ANGLE dEQP bots to Clang without fixing this issue first. Dirk, since you reviewed https://chromium-review.googlesource.com/1012740 , please find someone to implement this pinning mechanism so we can use it before a Clang roll completely breaks these bots. Thanks.
,
Apr 14 2018
Given that thakis did the change, I'm guessing he's okay with us not actually having the pinning feature in the meantime. AFAIK, we haven't yet had a clang roll that's broken things, so I'm not convinced that we need this mechanism, but of course I didn't want this mechanism in the first place so I'm not the best judge of the importance of this. @thakis - what are your thoughts on this?
,
Apr 14 2018
We already had a clang roll today that broke the linux and mac deqp bots, so there's 0 benefits on keeping the win bots on msvc and making everyone support msvc. Having said that, I'll work on issue 786460 starting monday. I agree both bugs are the same.
,
Oct 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b98e2bc4fa7968ff4aacd00ff183d20866254acf commit b98e2bc4fa7968ff4aacd00ff183d20866254acf Author: Stephen Martinis <martiniss@chromium.org> Date: Wed Oct 31 21:58:40 2018 Remove GYP_DEFINE reference in clang update.py Bug: 756686, 873373 Change-Id: I22adc1b01841e97a81e230f4fa8b94e96fdf623a Reviewed-on: https://chromium-review.googlesource.com/c/1309229 Commit-Queue: Stephen Martinis <martiniss@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#604387} [modify] https://crrev.com/b98e2bc4fa7968ff4aacd00ff183d20866254acf/DEPS [modify] https://crrev.com/b98e2bc4fa7968ff4aacd00ff183d20866254acf/tools/clang/scripts/update.py
,
Nov 16
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by dpranke@chromium.org
, Aug 18 2017