Don't issue try jobs for OWNERS-only changes.

Don't issue try jobs for OWNERS-only changes.

BUG=None
R=maruel@chromium.org
TEST=PRESUBMIT_test.py, less needless try jobs.
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176192

[Dan Beam, 2013-01-10 21:53:44]

see https://chromiumcodereview.appspot.com/11820044/ for my motivation

[M-A Ruel, 2013-01-10 22:42:59]

https://codereview.chromium.org/11830057/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11830057/diff/1/PRESUBMIT.py#newcode832
PRESUBMIT.py:832: if not files or all(re.search(r'[\\/]OWNERS$', f) for f in
files):
What about doing a whitelist instead?

We currently have one in presubmit_support.py as InputApi.DEFAULT_WHITE_LIST but
it's not available in this context. The list also has a glarring ommision: .gyp
and .gypi. On the other side, I don't think it's worth sending a try job for an
.html or .css only change.

What do you think?

[Dan Beam, 2013-01-10 22:44:36]

On 2013/01/10 22:42:59, Marc-Antoine Ruel wrote:
> https://codereview.chromium.org/11830057/diff/1/PRESUBMIT.py
> File PRESUBMIT.py (right):
> 
> https://codereview.chromium.org/11830057/diff/1/PRESUBMIT.py#newcode832
> PRESUBMIT.py:832: if not files or all(re.search(r'[\\/]OWNERS$', f) for f in
> files):
> What about doing a whitelist instead?
> 
> We currently have one in presubmit_support.py as InputApi.DEFAULT_WHITE_LIST
but
> it's not available in this context. The list also has a glarring ommision:
.gyp
> and .gypi. On the other side, I don't think it's worth sending a try job for
an
> .html or .css only change.

.gypi, .gyp, .html, .css, and .js all have potential to break the build.  OWNERS
doesn't in any scenario, AFAIK...

> 
> What do you think?

[Dan Beam, 2013-01-10 22:46:47]

also, I don't think a whitelist would be particularly useful unless it's just
the same regex, essentially, as there's a bajillion OWNERS files

  $ find -type f -name OWNERS | wc -l
  532

[Dan Beam, 2013-01-10 22:47:54]

I should mention that this obviously doesn't change the review/approval process,
just the try jobs sent for OWNERS-only changes.

[M-A Ruel, 2013-01-10 22:51:20]

On 2013/01/10 22:44:36, Dan Beam wrote:
> .gypi, .gyp, .html, .css, and .js all have potential to break the build. 
OWNERS
> doesn't in any scenario, AFAIK...

Dan, the change itself LGTM.

Scott/Nico; do you think it'd be possible to ask ninja to return a yes/no value
based on the list of files that are in the change list? To clarify, don't scan
the file system, just return true if any build rule would be triggered by the
list of the files given as input argument.

While this wouldn't be usable directly here as we can't assume the ninja files
were generated or up to date, I could make use of this on the CQ or on the Try
Server on the slave side.

[Nico, 2013-01-10 22:54:08]

On 2013/01/10 22:51:20, Marc-Antoine Ruel wrote:
> On 2013/01/10 22:44:36, Dan Beam wrote:
> > .gypi, .gyp, .html, .css, and .js all have potential to break the build. 
> OWNERS
> > doesn't in any scenario, AFAIK...
> 
> Dan, the change itself LGTM.
> 
> Scott/Nico; do you think it'd be possible to ask ninja to return a yes/no
value
> based on the list of files that are in the change list? To clarify, don't scan
> the file system, just return true if any build rule would be triggered by the
> list of the files given as input argument.
> 
> While this wouldn't be usable directly here as we can't assume the ninja files
> were generated or up to date, I could make use of this on the CQ or on the Try
> Server on the slave side.

If there was  a check like that, would you want to run this on every bot type
(e.g. currently win,mac,linux32,linux64,android,ios,chromeos) before deciding to
run a real try job? Is that worth it?

[commit-bot: I haz the power, 2013-01-10 23:10:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/11830057/1

[commit-bot: I haz the power, 2013-01-10 23:12:22]

Change committed as 176192

[Dirk Pranke, 2013-01-10 23:16:23]

Also it would be hard for such a check to accurately capture changes
to data files or test files needed during testing for which you would
still want try runs.

Maybe another strategy would be if the file was needed in an isolate,
or was part of (compiled into) a file needed in an isolate?

-- Dirk

On Thu, Jan 10, 2013 at 2:54 PM,  <thakis@chromium.org> wrote:
> On 2013/01/10 22:51:20, Marc-Antoine Ruel wrote:
>>
>> On 2013/01/10 22:44:36, Dan Beam wrote:
>> > .gypi, .gyp, .html, .css, and .js all have potential to break the build.
>> OWNERS
>> > doesn't in any scenario, AFAIK...
>
>
>> Dan, the change itself LGTM.
>
>
>> Scott/Nico; do you think it'd be possible to ask ninja to return a yes/no
>
> value
>>
>> based on the list of files that are in the change list? To clarify, don't
>> scan
>> the file system, just return true if any build rule would be triggered by
>> the
>> list of the files given as input argument.
>
>
>> While this wouldn't be usable directly here as we can't assume the ninja
>> files
>> were generated or up to date, I could make use of this on the CQ or on the
>> Try
>> Server on the slave side.
>
>
> If there was  a check like that, would you want to run this on every bot
> type
> (e.g. currently win,mac,linux32,linux64,android,ios,chromeos) before
> deciding to
> run a real try job? Is that worth it?
>
> https://codereview.chromium.org/11830057/

[M-A Ruel, 2013-01-10 23:16:42]

On 2013/01/10 22:54:08, Nico wrote:
> On 2013/01/10 22:51:20, Marc-Antoine Ruel wrote:
> > Scott/Nico; do you think it'd be possible to ask ninja to return a yes/no
> value
> > based on the list of files that are in the change list? To clarify, don't
scan
> > the file system, just return true if any build rule would be triggered by
the
> > list of the files given as input argument.
> > 
> > While this wouldn't be usable directly here as we can't assume the ninja
files
> > were generated or up to date, I could make use of this on the CQ or on the
Try
> > Server on the slave side.
> 
> If there was  a check like that, would you want to run this on every bot type
> (e.g. currently win,mac,linux32,linux64,android,ios,chromeos) before deciding
to
> run a real try job? Is that worth it?

If the time to query is calculated in seconds, I think it is. The time would be
dominated by gyp generation as the CQ will have to generate all the
configurations it has whitelisted. Still, even if we talk about 2 minutes, it's
still better than running tests unneededlessly.

[M-A Ruel, 2013-01-10 23:43:40]

On 2013/01/10 23:16:23, Dirk Pranke wrote:
> Also it would be hard for such a check to accurately capture changes
> to data files or test files needed during testing for which you would
> still want try runs.
> 
> Maybe another strategy would be if the file was needed in an isolate,
> or was part of (compiled into) a file needed in an isolate?

.isolate are .gypi files that are going to be included in the dependency list
analysis. So it's a non-issue.