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

Issue 757051 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug

Blocking:
issue 761066



Sign in to add a comment

make sure checkbins is running on all builders

Project Member Reported by wfh@chromium.org, Aug 18 2017

Issue description

checkbins is configured here:

https://cs.chromium.org/chromium/src/testing/buildbot/chromium.json?l=37

to run on 'Win' builder on 'chromium' waterfall:

https://build.chromium.org/p/chromium/builders/Win

This means it runs on 32-bit only and only on clobber builds (not trybots)

We should consider adding the checkbins step to:

chromium/Win x64
chromium.win/Win Builder
chromium.win/Win x64 Builder
 

Comment 1 by wfh@chromium.org, Aug 18 2017

Cc: dpranke@chromium.org
props to dpranke@ for explaining all this to me.

here is actual evidence it does at least get run on 32-bit clobber builds - https://build.chromium.org/p/chromium/builders/Win/builds/58119

Comment 2 by wfh@google.com, Aug 18 2017

Components: Security
Labels: -Pri-3 Pri-1

Comment 3 by wfh@chromium.org, Aug 18 2017

Owner: wfh@chromium.org
Status: Started (was: Untriaged)
I can tackle this.
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 25 2017

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

commit 913b6166ce7aded60fa153f019c189da1af36f9e
Author: Will Harris <wfh@chromium.org>
Date: Fri Aug 25 20:38:19 2017

Add checkbins to Clobber release Windows builders.

BUG=757051

Change-Id: Id3cff0b0efdab109ebc17a4b96d49c76d191fdb8
Reviewed-on: https://chromium-review.googlesource.com/621305
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497523}
[modify] https://crrev.com/913b6166ce7aded60fa153f019c189da1af36f9e/testing/buildbot/chromium.json

Comment 5 by wfh@chromium.org, Aug 25 2017

CL above adds to the two chromium clobber bots:

https://build.chromium.org/p/chromium/waterfall?builder=Win
https://build.chromium.org/p/chromium/waterfall?builder=Win%20x64

so these should go red if checkbins ever fails for either of these.

We haven't worked out a good solution for adding them to trybots that do not clobber, because it's possible this might cause trybot false positive and negatives e.g. two possible scenarios:

Scenario 1:
 - someone runs a tryjob that results in a binary that causes checkbins to fail
 - a subsequent tryjob runs on the same bot and doesn't modify that binary, the bot doesn't try to rebuild it, and checkbins fails for reasons unrelated to the CL in question

Scenario 2:

 - a new check is added to checkbins.py and then when it's run, it will run on old binaries and the try jobs will fail.
I don't know much about these "clobber" bots.  Could you clarify whether this script will be run during a CQ run to land a CL?  I'm getting the feeling 'no'... but that's kind of what I need to land https://chromium-review.googlesource.com/c/chromium/src/+/617817.

Regarding scenario 2: If we ensure the CQ coverage, then I'll see green or red when trying to land the above CL, and could ensure "green" then.

Scenario 1: Trickier.  Any way to "clobber"/delete just the build output directory if this script step fails (and the build process bails out)?

If CQ coverage is just not possible (any time soon), I'll move to land my original CFI unit test fix (as it is).

Thanks!

Comment 7 by wfh@chromium.org, Aug 28 2017

Cc: jbudorick@chromium.org
the script will not/is not run during CQ. If you want to run it as part of a trybot then just add the appropriate json into chromium.win in your CL, and upload it and the trybots will pick it up. This will ensure that the clobber bots aren't broken after your CL lands.

The file/edit you need is here -> https://chromium-review.googlesource.com/c/chromium/src/+/621305/1/testing/buildbot/chromium.win.json to chromium.win.json

I don't think your scenario 1 is possible. One option would be to make a presubmit that runs the checkbins script if edits are made to a set of files, on a separate trybot configured to always run checkbins...

Otherwise, just detecting the failure on the clobber bots - which should result in the tree closing, should be sufficient for now. It would mean any breakages result in a revert rather than a try failure, but given these flags change so infrequently that might be sufficient.

Perhaps jbudorick@ can chime in with some ideas too?

Comment 8 by wfh@chromium.org, Aug 28 2017

from the CL:

>I suppose another place we could put it would be to integrate it into the
>build steps, so the linking does a checkbins-type-thing to make sure it looks
>like we want?

I think this sounds like a sensible way forward.
#8: that sounds pretty reasonable to me, too.
From looking at the script, it looks like this scans *all* the DLLs and EXEs in the directory? Does it need to do that, or can it just look at a specific list of files? 

If the former, there's no good way to make this work properly either as part of the build or as a step that'd be run conditionally in the CQ.

Comment 11 by wfh@chromium.org, Aug 28 2017

I think if we were to go with plan in #8 then we'd change the script to keep the scanning guts but to take a path to a filename to scan, then let gn/ninja build steps run the script after each link stage, then control whether a specific target needs this step through some sort of gn magic :hand wave: /looks at scottmg.
Ah, yes, you could use a link wrapper to make this a post-link step, that would work fine.
Blocking: 761066

Sign in to add a comment