Would this also make  issue 762359  obsolete?

I think this issue is different from 762359. This is largely meant for desktop, while the other appears to be for Android. Also, this relates more to moving away from parsing stdout (in favor of outputting JSON directly) and moving things src-side, while 762359 is about deduplicating code.

Any update here? There's a bug in sizes.py (see  issue 831951  where it let two regressions sneak in), and I don't know which, or both, of the sizes.py scripts to fix.

Nope, I haven't done any work on this and AFAIK no one else has, either.

I *think* you would need to change both the one in infra/scripts/legacy (https://chromium.googlesource.com/chromium/src/+/6c74cdcf786ee105487747af0bd2183e4b30b555/infra/scripts/legacy/scripts/slave/chromium/sizes.py) and the one in build/scripts (https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave/chromium/sizes.py)? Both seem to use dump-static-initializers.py, though, so you might be able to just modify that and get the fix in both places?

Both have code for dealing with SIs.

Thanks.

And a follow-on question: why should check_static_initializers.py live in chromium/build instead of chromium/src? Things like checkbins and checkperms live in chromium/src/tools/. So it seems like a plan of action could be:

- Reconcile the differences between chromium/infra/scripts/legacy/scripts/slave/chromium/sizes.py and chromium/build/scripts/slave/chromium/sizes.py into a chromium/src/tools/checksi/check_static_initializers.py
- Update chromium/src/testing/scripts/sizes.py to point to that new checksi/ script
- Update any infra config files that point at the old ones (?) 
- Delete the old copies of the script

Unfortunately the bug is not in the dependent script but in sizes.py itself (it's not checking the retcode of the otool command it runs).

I don't remember the exact reason or person (I think dpranke@?), and the old email thread seems to have disappeared for me. However, at some point, it was pointed out that it would be preferable to keep the src-side stuff in infra/scripts/legacy/scripts/slave/chromium.

However, I think you misread/misunderstood something - we wouldn't have anything in chromium/build after all this is done. We would have the following files:

//src/infra/scripts/legacy/scripts/slave/chromium/check_static_initializers.py - only measures the # of static initializers
//src/testing/scripts/check_static_initializers.py - wrapper to run the above script on the bots as a build step
//src/infra/scripts/legacy/scripts/slave/chromium/sizes.py - provides (non-static-initializer-related) binary size information in JSON format.

Ah, sorry, I missed that you were referring to the infra directory within chromium/src not the chromium/infra repo.

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

commit 15bbc3bf959efe5264443bea08f99a77b44f7921
Author: Robert Sesek <rsesek@chromium.org>
Date: Wed Apr 18 22:35:28 2018

Manually merge the two divergent sizes.py scripts.

The two scripts are:
//chromium/src/infra/scripts/legacy/scripts/slave/chromium/sizes.py
//chromium/tools/build/scripts/slave/chromium/sizes.py

The script in //chromium/tools/build will be removed in favor of the new
//chromium/src merged one.

Bug:  783855 
Change-Id: I6a359bf8137319231bc530889a276fd076665ca0
Reviewed-on: https://chromium-review.googlesource.com/1017061
Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551861}
[modify] https://crrev.com/15bbc3bf959efe5264443bea08f99a77b44f7921/infra/scripts/legacy/scripts/slave/chromium/sizes.py

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

commit 1abd5963a9602c79749964fa89289f9430d7fcf9
Author: Robert Sesek <rsesek@chromium.org>
Date: Thu Apr 19 00:28:03 2018

Remove the sizes.py script and point ChromiumFactory and recipes to the src-side copy.

Bug:  783855 
Change-Id: Ia2c16c3abcf130a807d5f7786f37aabdf133fe33
Reviewed-on: https://chromium-review.googlesource.com/1017563
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>

[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipes/cronet.expected/android_cronet_builder__dbg_.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipe_modules/chromium/tests/sizes.expected/basic.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipes/cronet.expected/android_cronet_x86_builder.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipe_modules/cronet/examples/full.expected/gn_test.json
[delete] https://crrev.com/2923501e25b973de9039ec35975de43bb3d50829/scripts/slave/chromium/sizes.py
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipes/cronet.expected/android_cronet_arm64_builder.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipes/cronet.expected/android_cronet_marshmallow_64bit_builder.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipe_modules/cronet/examples/full.expected/local_test.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipes/cronet.expected/android_cronet_x86_builder__dbg_.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipe_modules/chromium_tests/tests/steps/sizes_step.expected/basic.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipes/cronet.expected/android_cronet_kitkat_builder.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipe_modules/chromium/tests/sizes.expected/platform.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipe_modules/chromium/api.py
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipes/cronet.expected/local_test.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/master/factory/chromium_commands.py
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipes/cronet.expected/android_cronet_arm64_builder__dbg_.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipes/cronet.expected/android_cronet_lollipop_builder.json
[modify] https://crrev.com/1abd5963a9602c79749964fa89289f9430d7fcf9/scripts/slave/recipe_modules/cronet/examples/full.expected/mb_test.json

+dpranke: not sure if you wanted to own this bug too in addition to bug 572393.

We've now de-duped the sizes.py script, but there's more cleanup to be done (renaming the script, updating the expectation infrastructure, etc.).

It's pretty much the same bug, so, sure.

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

commit 70c56b8ff35427244ef484c2b176421a80d76da9
Author: Robert Sesek <rsesek@chromium.org>
Date: Thu Apr 19 19:58:34 2018

Fix a bug in sizes.py on Windows where main_win() was returning non-zero.

This was caused by an erroneous manual merge in
15bbc3bf959efe5264443bea08f99a77b44f7921.

Bug:  783855 ,  834604 
Change-Id: I1f821b5c86d7c11478f4430f68bce58238c1d9bc
Reviewed-on: https://chromium-review.googlesource.com/1019980
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552132}
[modify] https://crrev.com/70c56b8ff35427244ef484c2b176421a80d76da9/infra/scripts/legacy/scripts/slave/chromium/sizes.py

I think thomasanderson@ has completed doing this elsewhere ...