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

Issue 756686 link

Starred by 0 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Feature

Blocked on:
issue 875037

Blocking:
issue 718157
issue 786460



Sign in to add a comment

Remove use of GYP_DEFINES, LLVM_FORCE_HEAD_REVISION from tools/clang/scripts/update.py

Project Member Reported by dpranke@chromium.org, Aug 18 2017

Issue description

Now 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.
 
Blocking: 718157
Blocking: 727437

Comment 3 by kbr@chromium.org, Aug 27 2017

Status: Assigned (was: Available)
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.

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.

Comment 5 by kbr@chromium.org, 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.

Okay, that's fine. 

Comment 7 by thakis@chromium.org, 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)
Yup, "a few" == "three". 

Comment 9 by kbr@chromium.org, 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.

Cc: phajdan@google.com
Summary: Remove use of GYP_DEFINES, LLVM_FORCE_HEAD_REVISION from tools/clang/scripts/update.py (was: Remove use of GYP_DEFINES, LLVM_FORCE_HEAD_REVISION from tools/clang/update.py)
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.
Project Member

Comment 11 by bugdroid1@chromium.org, 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

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)?

Comment 13 by kbr@chromium.org, Oct 20 2017

Cc: dpranke@chromium.org
Labels: -Type-Bug -Pri-2 Pri-1 Type-Feature
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.

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" :-)

Comment 15 by kbr@chromium.org, 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.

It's also only "must do before we try to land a change that breaks exceptions and don't want to roll it back" ;).
Blocking: 786460
Blocking: -727437

Comment 19 by kbr@chromium.org, Nov 17 2017

Cc: jmad...@chromium.org
Owner: dpranke@chromium.org
Dirk, can you please find an owner for this? We're ready to put it to use on the ANGLE tree.

Cc: jbudorick@chromium.org
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.
let me know your thoughts?
Components: -Infra>SDK Infra>Client>Chrome
Owner: ----
Status: Available (was: Assigned)

Comment 23 by kbr@chromium.org, Apr 13 2018

Owner: dpranke@chromium.org
Status: Assigned (was: Available)
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.

Owner: thakis@chromium.org
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?
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.
Project Member

Comment 26 by bugdroid1@chromium.org, 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

Blockedon: 875037

Sign in to add a comment