New issue
Advanced search Search tips

Issue 909867 link

Starred by 1 user
Status: Fixed
Owner:
nicolaso@chromium.org
Closed: Dec 10
Cc:
meili...@chromium.org
rhalavati@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

{linux,win}-annotator-rel buildbot failures

Project Member Reported by nicolaso@chromium.org, Nov 28 
It looks like the network annotation introduced in [1] a couple weeks ago, caused the Linux and Windows annotator buildbots to start failing [2].

annotations.xml says the `journey_journey_info_json_request' is included on Windows and Linux. However, it looks like this code is only used on Android [3]. meiliang@, is this accurate?

If it's only on Android, we should clear the `os_list' and mark the annotation as deprecated. This will hopefully fix the waterfall bots.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1155719/29
[2] https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-annotator-rel?cursor=Ci0SJ2oQc35jci1idWlsZGJ1Y2tldHITCxIFQnVpbGQYsPaYnaTyoPZ7DBgAIAA%3D&limit=200
[3] https://cs.chromium.org/chromium/src/BUILD.gn?l=301&rcl=8104d670b52c6381a6157e60800268e7fc0d5a35

Log from running `check_annotations.py' locally:

> $ ./tools/traffic_annotation/scripts/check_annotations.py --build-path out/Debug --complete
> Starting traffic annotation auditor. This may take from a few minutes to a few hours based on the scope of the test.
> WARNING: Ignoring clang tool's returned errors.
> [Errors]
>   (1)   'tools/traffic_annotation/summary/annotations.xml' requires update. It is recommended to run traffic_annotation_auditor locally to do the updates automatically (please refer to tools/traffic_annotation/auditor/README.md), but you can also apply the following edit(s) to do it manually:
>         Update line: ' <item id="journey_journey_info_json_request" hash_code="62660788" type="0" content_hash_code="77009071" os_list="linux,windows" file_path="components/journey/journey_info_json_request.cc"/>' --> ' <item id="journey_journey_info_json_request" hash_code="62660788" type="0" content_hash_code="77009071" os_list="windows" file_path="components/journey/journey_info_json_request.cc"/>'

Comment 1 by rhalavati@google.com, Nov 29 

If the annotation is introduced only for a call that happens in Android, I think it's better to remove it from annotations.xml and use NO_TRAFFIC_ANNOTATION_YET in code instead of defining one. This tag is used for all code sites were we need an annotation only for a platform that is not supported yet.

Marking it deprecated gives the meaning that it was used on Windows and Linux for sometime, but not any more.
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 30 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/706b39eb53476938cb57be1e0c81928c747f0713

commit 706b39eb53476938cb57be1e0c81928c747f0713
Author: Nicolas Ouellet-payeur <nicolaso@chromium.org>
Date: Fri Nov 30 15:21:38 2018

Remove journey_journey_info_json_request network annotation

Bug:  909867 
Change-Id: I8d910acd2b5adf3686678ef186318cc4dc4fe118
Reviewed-on: https://chromium-review.googlesource.com/c/1355772
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612653}
[modify] https://crrev.com/706b39eb53476938cb57be1e0c81928c747f0713/components/journey/journey_info_json_request.cc
[modify] https://crrev.com/706b39eb53476938cb57be1e0c81928c747f0713/tools/traffic_annotation/summary/annotations.xml
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 30 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1679c39fbc1b0fd6b67331cf69e94f264c5c3de6

commit 1679c39fbc1b0fd6b67331cf69e94f264c5c3de6
Author: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Date: Fri Nov 30 20:01:42 2018

Revert "Remove journey_journey_info_json_request network annotation"

This reverts commit 706b39eb53476938cb57be1e0c81928c747f0713.

Reason for revert: broke check_network_annotations on CQ and didn't fix the underlying issue.

Original change's description:
> Remove journey_journey_info_json_request network annotation
> 
> Bug:  909867 
> Change-Id: I8d910acd2b5adf3686678ef186318cc4dc4fe118
> Reviewed-on: https://chromium-review.googlesource.com/c/1355772
> Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
> Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
> Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612653}

TBR=yusufo@chromium.org,rhalavati@chromium.org,meiliang@chromium.org,nicolaso@chromium.org

Change-Id: If75c9709ad6d81e9c4f5576b89f91b6af66fe4f7
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  909867 
Reviewed-on: https://chromium-review.googlesource.com/c/1357215
Reviewed-by: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612740}
[modify] https://crrev.com/1679c39fbc1b0fd6b67331cf69e94f264c5c3de6/components/journey/journey_info_json_request.cc
[modify] https://crrev.com/1679c39fbc1b0fd6b67331cf69e94f264c5c3de6/tools/traffic_annotation/summary/annotations.xml

Comment 4 by nicolaso@chromium.org, Dec 3 

Looks like that was the wrong approach, as my patch broke the `check_network_annotations' step on CQ (and didn't fix waterfall).

Looking into it more, it looks like this file is included on Linux/Windows after all, but only for unit tests [1]. This is probably what's confusing our buildbots.

meiliang@, it looks like `components/journey' is intended for Android only. Is that accurate? If so, is it OK to disable those unit tests on other platforms?

A patch like in comment 2 might fix the problem, if we also make `//components/journey:unit_tests' Android-only.

[1] https://cs.chromium.org/chromium/src/components/BUILD.gn?l=227&rcl=7c4d25d6e89687033e6234fcf8bb018dee54a193
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 5 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d715c04a0846f1955c2828fc33c0d9e55aebef9b

commit d715c04a0846f1955c2828fc33c0d9e55aebef9b
Author: Mei Liang <meiliang@chromium.org>
Date: Wed Dec 05 20:01:53 2018

Remove journey_journey_info_json_request network annotation

This CL also disables journey:unit_tests on other platforms, but
Android.

Bug:  909867 
Change-Id: I7bfe8c9464ec203b60d50679dda4c0a5b052f0e0
Reviewed-on: https://chromium-review.googlesource.com/c/1360276
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Reviewed-by: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Commit-Queue: Mei Liang <meiliang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614070}
[modify] https://crrev.com/d715c04a0846f1955c2828fc33c0d9e55aebef9b/components/BUILD.gn
[modify] https://crrev.com/d715c04a0846f1955c2828fc33c0d9e55aebef9b/components/journey/journey_info_json_request.cc
[modify] https://crrev.com/d715c04a0846f1955c2828fc33c0d9e55aebef9b/tools/traffic_annotation/summary/annotations.xml

Comment 6 by nicolaso@chromium.org, Dec 10

Status: Fixed (was: Untriaged)
This fixed linux-annotator-rel.

The bot for win-annotator-rel died, so I got a Trooper to respawn it recently. We'll know if it worked on Windows within a few hours. Keeping track of the bot's status.
►

Sign in to add a comment