New issue
Advanced search Search tips

Issue 631575 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Chrome,Blink and v8 trybots for ICU CL?

Project Member Reported by js...@chromium.org, Jul 26 2016

Issue description

third_party/icu is a critical component of chrome, blink and v8. Currently, it's not possible to run trybots (Chrome, Blink, v8) before rolling icu to a new version in src/DEPS. 

Because it's not possible atm, what I usually do is

1) for a critical change that needs to be tested on all platforms (e.g. major version update, say, from ICU 56.1 to ICU 57.1), I build Chrome/Blink (I haven't done this on V8, yet) on Linux/Mac/Windows/Android (and sometimes iOS) and test. 

2) for a simpler change, I test it on Linux (with Chrome/Blink/v8). 

It'd be handy if it's possible to send *optional* try jobs for an ICU CL (or a particular revision) to Chrome, Blink and v8 bots. 

CQ is certainly an overkill and can get in the way when I need to put ICU in an "intermeidate" state (as a part of a major update). 


machenbach@ may have already started this for v8 (see https://codereview.chromium.org/2174993002/ ). 

 

Comment 1 by js...@chromium.org, Jul 26 2016

Maybe, it's lot simpler than I thought.  It seems that it can be enabled by just adding a line to codereview.settings. machenbach@ just did it for icu ( https://codereview.chromium.org/2182943002 )

However, this does not work as expected:

git cl try -b ios-simulator-gn -m tryserver.chromium.mac

https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/builds/42061
Labels: -Type-Bug Type-Feature
Owner: machenb...@chromium.org
Status: Started (was: Untriaged)
CL for chromium:
https://codereview.chromium.org/2183393002/

V8 should already work after:
https://codereview.chromium.org/2186433003/
https://codereview.chromium.org/2182943002/

This will work for newly uploaded CLs that are rebased to the last change.
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 27 2016

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

commit aef7fb2d8b6212b1232fa5bd1768dbc8e290cf7c
Author: recipe-roller <recipe-roller@chromium.org>
Date: Wed Jul 27 11:45:49 2016

Roll recipe dependencies (trivial).

This is an automated CL created by the recipe roller. This CL rolls recipe
changes from upstream projects (e.g. depot_tools) into downstream projects
(e.g. tools/build).

More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug
(or complain)

depot_tools:
  https://crrev.com/1278dbd127a620c8bdc96f258bf5ead4031c444b Enable testing ICU CLs on chromium trybots (machenbach@chromium.org)

R=machenbach@chromium.org
BUG= 631575 

TBR=martiniss@chromium.org,phajdan.jr@chromium.org

Review-Url: https://codereview.chromium.org/2184093002

[modify] https://crrev.com/aef7fb2d8b6212b1232fa5bd1768dbc8e290cf7c/infra/config/recipes.cfg

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 27 2016

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra.git/+/48f70db6839be5b168d6745607f2f70f12b99abc

commit 48f70db6839be5b168d6745607f2f70f12b99abc
Author: recipe-roller <recipe-roller@chromium.org>
Date: Wed Jul 27 12:08:36 2016

Roll recipe dependencies (trivial).

This is an automated CL created by the recipe roller. This CL rolls recipe
changes from upstream projects (e.g. depot_tools) into downstream projects
(e.g. tools/build).

More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug
(or complain)

build:
  https://crrev.com/aef7fb2d8b6212b1232fa5bd1768dbc8e290cf7c Roll recipe dependencies (trivial). (recipe-roller@chromium.org)
depot_tools:
  https://crrev.com/1278dbd127a620c8bdc96f258bf5ead4031c444b Enable testing ICU CLs on chromium trybots (machenbach@chromium.org)

R=phajdan.jr@chromium.org,machenbach@chromium.org,martiniss@chromium.org,recipe-roller@chromium.org
BUG= 631575 

TBR=martiniss@chromium.org,phajdan.jr@chromium.org

Review-Url: https://codereview.chromium.org/2192443002

[modify] https://crrev.com/48f70db6839be5b168d6745607f2f70f12b99abc/infra/config/recipes.cfg

Status: Fixed (was: Started)
Test CL showing that chromium and v8 trybots work now (blink essentially uses the chromium recipe under the hood):
https://codereview.chromium.org/2183683004/

Comment 8 by js...@chromium.org, Jul 27 2016

Thanks a lot. 

Chromium trybots somehow succeeds for my test CL (that I broke on purpose). OTOH, v8 trybot failed as expected (compile should fail). 

https://codereview.chromium.org/2186133002


Comment 9 by js...@chromium.org, Jul 27 2016

Linux trybot failed, but Android, ios, windows and Mac succeeded. Locally, Mac and Android as well as Linux failed to get compiled (as they should because I broke one of header files used by virtually every ICU cc files  on purpose in the above CL). 

Comment 10 by js...@chromium.org, Jul 28 2016

This bug was indeed fixed, but 'gn refs <out dir> <file>' does not work for ICU headr files (most likely because those header files are not explicitly listed in BUILD.gn) 

See https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/263499/steps/analyze/logs/stdio

$ gn refs out/rel "third_party/icu/source/common/unicode/platform.h" --all
The input matches no targets, configs, or files.

Thank you, again for promptly taking care of this, Michael !
Yea - it looks like the patch is applied everywhere now, but analyzed decides to ignore the changes on some bots. I think that is a separate issue.
Cc: -machenb...@chromium.org tandrii@chromium.org
Status: Assigned (was: Fixed)
I was wrong. It's an infra problem. On the passing bots, the step "git diff to analyze patch" is executed in the wrong directory "src", while on the correctly failing bots it's executed in "src/third_party/icu".

@tandrii: Didn't you land a generic fix for this once? I have to check the details of  issue 491098 .
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra.git/+/162ccc443a2a7ac65d6cd3cf68a592eb6921c1a9

commit 162ccc443a2a7ac65d6cd3cf68a592eb6921c1a9
Author: recipe-roller <recipe-roller@chromium.org>
Date: Thu Jul 28 13:27:04 2016

Roll recipe dependencies (trivial).

This is an automated CL created by the recipe roller. This CL rolls recipe
changes from upstream projects (e.g. depot_tools) into downstream projects
(e.g. tools/build).

More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug
(or complain)

build:
  https://crrev.com/10d4c37a58a56a1edafc49ae71684203e960b1eb Fix applying non-chromium patches to ios trybots (machenbach@chromium.org)

R=machenbach@chromium.org
BUG= 631575 

TBR=martiniss@chromium.org,phajdan.jr@chromium.org

Review-Url: https://codereview.chromium.org/2186243002

[modify] https://crrev.com/162ccc443a2a7ac65d6cd3cf68a592eb6921c1a9/infra/config/recipes.cfg

Status: Fixed (was: Assigned)
Ios trybot now also fails as expected. Please reopen if there's a trybot that doesn't.
Project Member

Comment 17 by bugdroid1@chromium.org, Jul 28 2016

Comment 18 by js...@chromium.org, Jul 28 2016

Thanks a lot,  Michael !

With a cc file in ICU broken on purpose, all the trybots including ios bots failed except for one. Somehow, android_compile_mips_dbg still succeeds while android_compile_rel did fail (as it should). 

https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_mips_dbg/builds/28

If this target is a big endian mips (as opposed to mipsle), it's likely that it does not use ICU (until a few days ago, BE was not supported in our copy of ICU). But, I guess this target is little endian mips. In that case, this target should fail, too. 

analyzer output: https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_mips_dbg/builds/28/steps/analyze/logs/stdio

This may or may not be an infra issue. It could well be a BUILD.gn issue (either in ICU or android mips target), but I'm not sure. 
This looks indeed like a GN issue, as the input to analyze looks correct. Please file a separate bug for this.
Saw now you filed  http://crbug.com/632438 

Sign in to add a comment