New issue
Advanced search Search tips

Issue 878760 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug



Sign in to add a comment

Traffic annotation auditor is not sensitive to unsupported platform names.

Project Member Reported by rhalavati@chromium.org, Aug 29

Issue description

If someone adds an unsupported platform name to the os_list of an annotation in annotations.xml, such as 'mac', traffic_annotation_auditor does not catch this as an error.

 
Is "mac" unsupported, though? Because the exporter [1] spews "mac" in its os_list, if you build your own traffic_annotation_auditor. If we don't want to put it in the output, that looks like a bug as well.

FWIW we have 2 occurences of "mac" already in annotations.xml.

[1] https://cs.chromium.org/chromium/src/tools/traffic_annotation/auditor/traffic_annotation_exporter.cc?l=80&rcl=e8fc42d59a3be80e09a1abb03fb483d337f6f2ed
You are right, and I reviewed that change, but now I think it was a mistake to add it while mac is not supported yet.

The |os_list| is initiated with all supported platforms, and once the auditor finds that an annotation is no more available on one platform, it removes the platform from the |os_list|. When |os_list| is emptied, the annotation is marked as deprecated. With 'mac' here, and as we do not run auditor on mac, when an annotation is removed from both windows and linux, |os_list| doesn't get empty an annotation stays as active.
Hence, I think removing 'mac' from exporter and all current mentions in annotations.xml would be the good move.
Labels: Enterprise-Triaged OS-Mac
Owner: georgesak@chromium.org
Status: Assigned (was: Available)
Cc: -nicolaso@chromium.org
Owner: nicolaso@chromium.org
Cc: nicolaso@chromium.org
Owner: syoussefi@chromium.org
Owner: nicolaso@chromium.org
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 19

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

commit 0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330
Author: Nicolas Ouellet-payeur <nicolaso@chromium.org>
Date: Fri Oct 19 14:53:07 2018

Remove MacOS from traffic annotations

- Don't build traffic_annotation_auditor on MacOS.
- Remove "mac" from the "os_list" everywhere in annotations.xml.
- Add a warning and fail on buildbots when an unsupported platform is
  added in annotations.xml.

Bug:  878760 
Change-Id: I7329e8ca2940880cdc4aa357acd42b2976d9a80e
Reviewed-on: https://chromium-review.googlesource.com/c/1287097
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601147}
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/BUILD.gn
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/testing/buildbot/chromium.clang.json
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/testing/buildbot/chromium.mac.json
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/testing/buildbot/test_suites.pyl
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/tools/traffic_annotation/auditor/BUILD.gn
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/tools/traffic_annotation/auditor/auditor_result.cc
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/tools/traffic_annotation/auditor/auditor_result.h
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/tools/traffic_annotation/auditor/traffic_annotation_auditor.cc
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/tools/traffic_annotation/auditor/traffic_annotation_auditor.h
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/tools/traffic_annotation/auditor/traffic_annotation_exporter.cc
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/tools/traffic_annotation/auditor/traffic_annotation_exporter.h
[modify] https://crrev.com/0d23b5cf0da7631e2d9d4bf53ea4baf48a51c330/tools/traffic_annotation/summary/annotations.xml

Status: Fixed (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 22

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

commit 416ac8f01bb6db626209475cdebbeaf942911264
Author: Ramin Halavati <rhalavati@chromium.org>
Date: Mon Oct 22 09:48:27 2018

Roll traffic_annotation_auditor binaries.

Traffic annotation auditor binaries for Linux and Windows are updates
after the change crrev.com/c/1287097.

Bug:  878760 
Change-Id: I92e7f632cc9343fee93e16f0d6b48f099189d6da
TBR: nicolaso@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/1292877
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601509}
[modify] https://crrev.com/416ac8f01bb6db626209475cdebbeaf942911264/tools/traffic_annotation/bin/README.md
[modify] https://crrev.com/416ac8f01bb6db626209475cdebbeaf942911264/tools/traffic_annotation/bin/linux64/traffic_annotation_auditor.sha1
[modify] https://crrev.com/416ac8f01bb6db626209475cdebbeaf942911264/tools/traffic_annotation/bin/win32/traffic_annotation_auditor.exe.sha1

Sign in to add a comment