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

Issue 757293 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Investigate WebRTC Windows compile coverage.

Project Member Reported by kjellander@chromium.org, Aug 21 2017

Issue description

We're lacking proper coverage on our Windows compilation trybots, which has been demonstrated by  bug 756840  (GPU bots building with MSVC, catching errors we couldn't) and https://bugs.chromium.org/p/webrtc/issues/detail?id=8108 (size_t to int warning didn't fail our build).

I suspect it's caused by our bots building with different flags than Chromium and/or change default for Clang for Windows.
 
I remembered correctly: Clang was enabled by default on Windows in May and it appears some GPU bots are still using MSVC (see  bug 727437 ). 

I'm still looking to see if setting is_clang=false on some of our bots is the right thing to do.
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/8b844b16eb8d71313d6b5a06e9469f968fcb0446

commit 8b844b16eb8d71313d6b5a06e9469f968fcb0446
Author: kjellander <kjellander@webrtc.org>
Date: Mon Aug 21 04:37:21 2017

Only disable -Wthread-safety-attributes warning for Clang.

Clang is enabled by default and we currently lack coverage for building on Windows
with MSVC (see  crbug.com/757293 ). This should unblock rolling WebRTC into
Chromium DEPS. We need to improve our trybot coverage for standalone WebRTC
to prevent things like this in the future though ( crbug.com/756840 ).

BUG= webrtc:8119 , chromium:756840 , chromium:757293 
TBR=hbos@webrtc.org
NOTRY=True

Review-Url: https://codereview.webrtc.org/3003473002
Cr-Commit-Position: refs/heads/master@{#19420}

[modify] https://crrev.com/8b844b16eb8d71313d6b5a06e9469f968fcb0446/webrtc/modules/video_coding/BUILD.gn

Cc: mbonadei@chromium.org
Status: Started (was: Assigned)
I read some more in 82385 and it appears Windows was flipped to Clang at Jul 28 in https://chromium.googlesource.com/chromium/src/+/d2c91228a51bdf37ae3b2e501fb53c0528f1629c but then recently flipped back 2 days ago in https://chromium.googlesource.com/chromium/src.git/+/7cd2f6bd73c6a9c6200704e995b18ebb3168ead6

Our bots are using these same configurations, so our coverage should be fine, and we should probably keep our Win Clang bots until this flip sticks. What we should probably do is to add att least one bot that explicitly sets is_clang=false so we can catch errors like  bug 756840  next time Clang is flipped on Windows. I'll set that up.
In the two flip-CLs mentioned above, Chromium flips their Clang-specific bots to build with MSVC instead, so they don't lose coverage. As we didn't do the same (we didn't notice), we were bitten by a compile regression on MSVC during their first flip-attempt.

I'll CC some of us to  bug 82385  so we can stay informed when the next attempt is going to be made.
Cc: charujain@chromium.org
+charujain as trooper, since the above explains some of the errors blocking the autoroller
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/c2d4d577552c1cac19155a2a36796cd7435554e7

commit c2d4d577552c1cac19155a2a36796cd7435554e7
Author: kjellander <kjellander@webrtc.org>
Date: Mon Aug 21 09:57:53 2017

MB: Add configs for Windows bots enforcing MSVC compilation.

BUG= chromium:757293 
NOTRY=True

Review-Url: https://codereview.webrtc.org/2996343002
Cr-Commit-Position: refs/heads/master@{#19422}

[modify] https://crrev.com/c2d4d577552c1cac19155a2a36796cd7435554e7/tools_webrtc/mb/mb_config.pyl

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/0534141056df48e999bde31c88605ff66147aa7d

commit 0534141056df48e999bde31c88605ff66147aa7d
Author: Henrik Kjellander <kjellander@chromium.org>
Date: Mon Aug 21 10:02:15 2017

WebRTC: Add Windows bots building explicitly with MSVC enabled.

This should protect us from breakages during the transition to
using Clang as the default compiler on Windows. See  http://crbug.com/82385 
for details.

Required MB changes are done in https://codereview.webrtc.org/2996343002/

BUG= 757293 
TBR=ehmaldonado@chromium.org

Change-Id: Iad73a157121e72ac087b16237ac931d32b8e4850
Reviewed-on: https://chromium-review.googlesource.com/622791
Reviewed-by: Henrik Kjellander <kjellander@chromium.org>
Commit-Queue: Henrik Kjellander <kjellander@chromium.org>

[modify] https://crrev.com/0534141056df48e999bde31c88605ff66147aa7d/masters/master.tryserver.webrtc/slaves.cfg
[add] https://crrev.com/0534141056df48e999bde31c88605ff66147aa7d/scripts/slave/recipes/webrtc/standalone.expected/tryserver_webrtc_win_msvc_rel.json
[modify] https://crrev.com/0534141056df48e999bde31c88605ff66147aa7d/masters/master.client.webrtc/master_win_cfg.py
[modify] https://crrev.com/0534141056df48e999bde31c88605ff66147aa7d/masters/master.tryserver.webrtc/master.cfg
[add] https://crrev.com/0534141056df48e999bde31c88605ff66147aa7d/scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_win32_release__msvc_.json
[modify] https://crrev.com/0534141056df48e999bde31c88605ff66147aa7d/scripts/slave/recipe_modules/webrtc/builders.py
[modify] https://crrev.com/0534141056df48e999bde31c88605ff66147aa7d/masters/master.client.webrtc/slaves.cfg

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/7542bf80e2ec4cce6ccdcd70843521e59a62101c

commit 7542bf80e2ec4cce6ccdcd70843521e59a62101c
Author: kjellander <kjellander@webrtc.org>
Date: Mon Aug 21 16:51:47 2017

CQ: Add win_msvc_rel to default trybot set.

BUG= chromium:757293 
TBR=mbonadei@webrtc.org
NOTRY=True

Review-Url: https://codereview.webrtc.org/3002063002
Cr-Commit-Position: refs/heads/master@{#19434}

[modify] https://crrev.com/7542bf80e2ec4cce6ccdcd70843521e59a62101c/infra/config/cq.cfg

Remaining task for this bug is to investigate why we couldn't catch the error that was fixed in https://chromium-review.googlesource.com/c/external/webrtc/+/623949 on our bots. I will do some tests once we've unblocked rolling deps (tracked in https://bugs.chromium.org/p/webrtc/issues/detail?id=8109).
Status: Fixed (was: Started)
I think we're good here now as rolling is unblocked.

Sign in to add a comment