Fix _CheckFilePermissions to actually flag presubmit errors

Fix _CheckFilePermissions to actually flag presubmit errors

Previously it was misusing subprocess.Popen such that no output
was ever captured from checkperms.py.

Also, changed the check to use input_api.subprocess instead of importing subprocess.

R=maruel@chromium.org
NOTRY=true

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

[M-A Ruel, 2014-01-29 21:28:19]

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py#newcode12
PRESUBMIT.py:12: import re
Remove

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py#newcode13
PRESUBMIT.py:13: import subprocess
Remove

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py#newcode529
PRESUBMIT.py:529: checkperms = subprocess.Popen(args, stdout=subprocess.PIPE)
What you want is
errors = input_api.subprocess.check_output(args)

unless checkperms.py normally return non-zero.

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py#newcode796
PRESUBMIT.py:796: pattern = re.compile(
Replwith with: input_api.re.compile(

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py#newcode914
PRESUBMIT.py:914: if re.search(r"\bD?LOG\s*\(\s*INFO\s*\)", contents):
Here too, and up to line 922.

[adamk, 2014-01-29 21:39:54]

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py#newcode12
PRESUBMIT.py:12: import re
On 2014/01/29 21:28:19, M-A Ruel wrote:
> Remove

This and the other re comments below seem like they should be done in a separate
change.

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py#newcode13
PRESUBMIT.py:13: import subprocess
On 2014/01/29 21:28:19, M-A Ruel wrote:
> Remove

See below, I don't think check_output does what I want (though my python and
PRESUBMIT knowledge is rudimentary).

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py#newcode529
PRESUBMIT.py:529: checkperms = subprocess.Popen(args, stdout=subprocess.PIPE)
On 2014/01/29 21:28:19, M-A Ruel wrote:
> What you want is
> errors = input_api.subprocess.check_output(args)
> 
> unless checkperms.py normally return non-zero.

Not sure what you mean by "normally", but it returns non-zero when there are bad
permissions, so it seems like check_output's not a good match here.

[M-A Ruel, 2014-01-29 21:47:04]

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py#newcode529
PRESUBMIT.py:529: checkperms = subprocess.Popen(args, stdout=subprocess.PIPE)
On 2014/01/29 21:39:54, adamk wrote:
> On 2014/01/29 21:28:19, M-A Ruel wrote:
> > What you want is
> > errors = input_api.subprocess.check_output(args)
> > 
> > unless checkperms.py normally return non-zero.
> 
> Not sure what you mean by "normally", but it returns non-zero when there are
bad
> permissions, so it seems like check_output's not a good match here.

Normally would be in absence of catastrophic failure. Still, do not add
reference to global subprocess import, it was unwarranted. Use
input_api.subprocess instead.

[adamk, 2014-01-29 21:52:54]

Now using input_api.subprocess.

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/143013004/diff/20001/PRESUBMIT.py#newcode529
PRESUBMIT.py:529: checkperms = subprocess.Popen(args, stdout=subprocess.PIPE)
On 2014/01/29 21:47:05, M-A Ruel wrote:
> On 2014/01/29 21:39:54, adamk wrote:
> > On 2014/01/29 21:28:19, M-A Ruel wrote:
> > > What you want is
> > > errors = input_api.subprocess.check_output(args)
> > > 
> > > unless checkperms.py normally return non-zero.
> > 
> > Not sure what you mean by "normally", but it returns non-zero when there are
> bad
> > permissions, so it seems like check_output's not a good match here.
> 
> Normally would be in absence of catastrophic failure. Still, do not add
> reference to global subprocess import, it was unwarranted. Use
> input_api.subprocess instead.

Ok, will do, but note that I didn't add this reference, it was here already.

[M-A Ruel, 2014-01-30 01:36:17]

lgtm

[commit-bot: I haz the power, 2014-01-30 01:40:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/143013004/30001

[commit-bot: I haz the power, 2014-01-30 01:44:35]

Change committed as 247805

[scottmg, 2014-02-03 20:05:48]

This makes it impossible to upload .py files from Windows:

...
** Presubmit ERRORS **
checkperms.py failed.
  d:\src\cr3\src\build\gyp_chromium: Has shebang but not executable bit

Presubmit checks took 2.0s to calculate.
...

Can we revert this or make that test non-Windows please?

[adamk, 2014-02-03 20:34:45]

On 2014/02/03 20:05:48, scottmg wrote:
> This makes it impossible to upload .py files from Windows:
> 
> ...
> ** Presubmit ERRORS **
> checkperms.py failed.
>   d:\src\cr3\src\build\gyp_chromium: Has shebang but not executable bit
> 
> Presubmit checks took 2.0s to calculate.
> ...
> 
> Can we revert this or make that test non-Windows please?

Sorry for the trouble, I (incorrectly, apparently) assumed this script knew
something about Windows. I defer to maruel and thestig for the best way to
handle this on Windows.

[Nico, 2014-02-03 20:47:51]

On Mon, Feb 3, 2014 at 12:34 PM, <adamk@chromium.org> wrote:

> On 2014/02/03 20:05:48, scottmg wrote:
>
>> This makes it impossible to upload .py files from Windows:
>>
>
>  ...
>> ** Presubmit ERRORS **
>> checkperms.py failed.
>>    d:\src\cr3\src\build\gyp_chromium: Has shebang but not executable bit
>>
>
>  Presubmit checks took 2.0s to calculate.
>> ...
>>
>
>  Can we revert this or make that test non-Windows please?
>>
>
> Sorry for the trouble, I (incorrectly, apparently) assumed this script knew
> something about Windows. I defer to maruel and thestig for the best way to
> handle this on Windows.
>

Probably best to not run this test on windows. (Reverting this change
doesn't make sense to me as it really just makes the code that was there
before this change have an effect.)


>
> https://codereview.chromium.org/143013004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[adamk, 2014-02-04 00:24:52]

For those following along at home,
https://src.chromium.org/viewvc/chrome?revision=248605&view=revision skips the
check on Windows.