Remove over 100 lines from checkperms.py.

Make the design api-based to make it cleaner and only process the SCM controlled files.

Add check for shebang.

Add check for windows executable.

Add --bare option to ease mass processing of file with incorrect bit.

R=thestig@chromium.org
BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135495

[M-A Ruel, 2012-04-24 16:20:17]

Depends on a few in-flight CLs.

[Lei Zhang, 2012-04-24 19:34:01]

https://chromiumcodereview.appspot.com/10088002/diff/1002/tools/checkperms/ch...
File tools/checkperms/checkperms.py (right):

https://chromiumcodereview.appspot.com/10088002/diff/1002/tools/checkperms/ch...
tools/checkperms/checkperms.py:35: 'in',
Why is the '.in' extension whitelisted? Also, 'in' goes after 'exe'.

https://chromiumcodereview.appspot.com/10088002/diff/1002/tools/checkperms/ch...
tools/checkperms/checkperms.py:114: return bool(permission &
os.stat(full_path).st_mode)
Are you expecting os.stat() to not throw any exceptions?

https://chromiumcodereview.appspot.com/10088002/diff/1002/tools/checkperms/ch...
tools/checkperms/checkperms.py:136: shebang = self.has_shebang(full_path)
I think this is going to slow down checkperms.py significantly, since you have
to read every file now.

[M-A Ruel, 2012-04-25 01:30:11]

https://chromiumcodereview.appspot.com/10088002/diff/1002/tools/checkperms/ch...
File tools/checkperms/checkperms.py (right):

https://chromiumcodereview.appspot.com/10088002/diff/1002/tools/checkperms/ch...
tools/checkperms/checkperms.py:35: 'in',
On 2012/04/24 19:34:01, Lei Zhang wrote:
> Why is the '.in' extension whitelisted? Also, 'in' goes after 'exe'.

Replace 'in' with the 2 equivalent exceptions.

https://chromiumcodereview.appspot.com/10088002/diff/1002/tools/checkperms/ch...
tools/checkperms/checkperms.py:114: return bool(permission &
os.stat(full_path).st_mode)
On 2012/04/24 19:34:01, Lei Zhang wrote:
> Are you expecting os.stat() to not throw any exceptions?

Why would a file in the checkout not be readable by the user?

https://chromiumcodereview.appspot.com/10088002/diff/1002/tools/checkperms/ch...
tools/checkperms/checkperms.py:136: shebang = self.has_shebang(full_path)
On 2012/04/24 19:34:01, Lei Zhang wrote:
> I think this is going to slow down checkperms.py significantly, since you have
> to read every file now.

All the following is on a standard 500gb HD.

linux / git checkout / cold
real    0m30.113s
user    0m1.660s
sys     0m1.340s

linux / git checkout / warm
real    0m1.854s
user    0m0.950s
sys     0m0.630s

Yes, this is less than 2 seconds, checking over 35k files.


linux / svn checkout / relatively warm
real    2m48.827s
user    0m1.570s
sys     0m0.840s

linux / svn checkout / warm
real    2m45.645s
user    0m1.540s
sys     0m0.840s

All the "real" time is spent in the "svn ls --recursive" call.


linux / no checkout / cold
This is the total worst case, when the svn detection failed and it iterate
inside all the directories, including .svn :)
real    12m37.555s
user    0m17.540s
sys     0m16.940s

linux / no checkout / warm
real    0m18.165s
user    0m8.250s
sys     0m5.640s


As you can see, the very vast majority is spent in "svn ls". It is so ridiculous
that on a warm disk cache, it's faster to check all the files, including
everything under .svn, than calling "svn ls". :(

I'm not sure how to help with that. A quick sampling on "Linux" builder shows it
currently takes ~50 seconds to complete. I'm not sure why it takes almost 3
minutes here. Since it is vastly bound by subversion, I do not expect a
significant slow down on the continuous integration server.

On my workstation, the previous version of checkperms.py takes:


This is on svn 1.6.12.

I tried without --recursive but it is even slower, I cancelled it after 19
minutes...


As a comparison, the old version of checkperms.py with warm cache:
src> time tools/checkperms/checkperms.py
Using base directory: /mnt/seagate1.5tb/maruel/src/svn_stuff/chrome/src
Checking directory: /mnt/seagate1.5tb/maruel/src/svn_stuff/chrome/src

SUCCESS


real    0m32.547s
user    0m1.700s
sys     0m1.880s

It is faster because it never calls "svn ls" and just checks everything.

Note that the old version of checkperms.py has issue with non-English LANGUAGE
variable.

https://chromiumcodereview.appspot.com/10088002/diff/9001/tools/checkperms/ch...
File tools/checkperms/checkperms.py (right):

https://chromiumcodereview.appspot.com/10088002/diff/9001/tools/checkperms/ch...
tools/checkperms/checkperms.py:216: class ApiSvnQuick(ApiBase):
With this implementation, linux/svn/warm gets down to:
real    0m42.968s
user    0m24.070s
sys     0m17.220s

Still an order of magnitude slower than with git but still better than crawling
everything.

[Lei Zhang, 2012-04-25 04:01:02]

https://chromiumcodereview.appspot.com/10088002/diff/1002/tools/checkperms/ch...
File tools/checkperms/checkperms.py (right):

https://chromiumcodereview.appspot.com/10088002/diff/1002/tools/checkperms/ch...
tools/checkperms/checkperms.py:114: return bool(permission &
os.stat(full_path).st_mode)
On 2012/04/25 01:30:11, Marc-Antoine Ruel wrote:
> On 2012/04/24 19:34:01, Lei Zhang wrote:
> > Are you expecting os.stat() to not throw any exceptions?
> 
> Why would a file in the checkout not be readable by the user?

Ideally that would be the case, but the fact that this script exists seems to
indicate people can't set their file permissions correctly sometimes. Well, we
can just leave it as is and see if we get any complaints.

https://chromiumcodereview.appspot.com/10088002/diff/1002/tools/checkperms/ch...
tools/checkperms/checkperms.py:136: shebang = self.has_shebang(full_path)
How about we only check files with the executable bit set and see if they have a
shebang? If the file is not set executable, then do not worry about it?

On my machine, the existing script takes 165 seconds with a cold cache, and 90
seconds with a hot cache.
The new script takes 310 seconds with a cold cache, and 190 seconds with a hot
cache.
The new script minus the shebang check takes 140 seconds with a cold cache, and
100 seconds with a hot cache.

So even without the "svn ls" slowness, the new script with the shebang check is
quite a bit slower. 

Below is a micro-benchmark to demonstrate the issue:

----
#!/usr/bin/python

import os

def has_shebang(full_path):
  with open(full_path, 'rb') as f:
    f.read(3) == '#!/'

def list_dir(start_dir):
  return sorted(x for x in os.listdir(start_dir) if not x.startswith('.'))

def check_dir(start_dir):
  items = list_dir(start_dir)
  for item in items:
    full_path = os.path.join(start_dir, item)
    if os.path.isdir(full_path):
      check_dir(full_path)
    else:
      os.stat(full_path)
      has_shebang(full_path)

check_dir('.')
----

With a hot cache, it takes < 1 second whether you call has_shebang() or not.
With a cold cache, if you comment out has_shebang(), it takes 15 seconds. With
has_shebang(), it jumps to 100 seconds.

https://chromiumcodereview.appspot.com/10088002/diff/9001/tools/checkperms/ch...
File tools/checkperms/checkperms.py (right):

https://chromiumcodereview.appspot.com/10088002/diff/9001/tools/checkperms/ch...
tools/checkperms/checkperms.py:92: env['LANGUAGE'] = 'en_US.UTF-8'
I thought $LANG is the right variable to use here. Which command were you
running that depends on $LANGUAGE being set correctly?

https://chromiumcodereview.appspot.com/10088002/diff/9001/tools/checkperms/ch...
tools/checkperms/checkperms.py:216: class ApiSvnQuick(ApiBase):
On 2012/04/25 01:30:11, Marc-Antoine Ruel wrote:
> With this implementation, linux/svn/warm gets down to:
> real    0m42.968s
> user    0m24.070s
> sys     0m17.220s
> 
> Still an order of magnitude slower than with git but still better than
crawling
> everything.

I'm fine with this.

[M-A Ruel, 2012-04-27 16:49:17]

I modified the script heavily so the shebang check is skipped on most files.

From 35665 SCM controlled files, 30014 files are processed. The others are
ignored. From that number, 2532 files where tested for shebang. So from
non-ignored files, 8% are tested for shebang. Does that make more sense?

I also updated the CL description.

https://chromiumcodereview.appspot.com/10088002/diff/9001/tools/checkperms/ch...
File tools/checkperms/checkperms.py (right):

https://chromiumcodereview.appspot.com/10088002/diff/9001/tools/checkperms/ch...
tools/checkperms/checkperms.py:92: env['LANGUAGE'] = 'en_US.UTF-8'
On 2012/04/25 04:01:02, Lei Zhang wrote:
> I thought $LANG is the right variable to use here. Which command were you
> running that depends on $LANGUAGE being set correctly?

svn ls output is localized. :(

[Lei Zhang, 2012-04-27 20:51:40]

On 2012/04/27 16:49:17, Marc-Antoine Ruel wrote:
> I modified the script heavily so the shebang check is skipped on most files.
> 
> From 35665 SCM controlled files, 30014 files are processed. The others are
> ignored. From that number, 2532 files where tested for shebang. So from
> non-ignored files, 8% are tested for shebang. Does that make more sense?

Ya, this works for me. Looks like a good interview question. ;)

https://chromiumcodereview.appspot.com/10088002/diff/14001/tools/checkperms/c...
File tools/checkperms/checkperms.py (right):

https://chromiumcodereview.appspot.com/10088002/diff/14001/tools/checkperms/c...
tools/checkperms/checkperms.py:127: IGNORED_FILENAMES = set((
Probably does not need to be a set.

https://chromiumcodereview.appspot.com/10088002/diff/14001/tools/checkperms/c...
tools/checkperms/checkperms.py:226: os.path.basename(rel_path) in
IGNORED_FILENAMES or
nit: this fits on the line above.

[M-A Ruel, 2012-04-28 16:12:45]

https://chromiumcodereview.appspot.com/10088002/diff/14001/tools/checkperms/c...
File tools/checkperms/checkperms.py (right):

https://chromiumcodereview.appspot.com/10088002/diff/14001/tools/checkperms/c...
tools/checkperms/checkperms.py:127: IGNORED_FILENAMES = set((
On 2012/04/27 20:51:40, Lei Zhang wrote:
> Probably does not need to be a set.

Indeed, fixed.

https://chromiumcodereview.appspot.com/10088002/diff/14001/tools/checkperms/c...
tools/checkperms/checkperms.py:226: os.path.basename(rel_path) in
IGNORED_FILENAMES or
On 2012/04/27 20:51:40, Lei Zhang wrote:
> nit: this fits on the line above.

That's on purpose, +4 alignment makes refactoring much easier. I can save the
line if you prefer.

[M-A Ruel, 2012-05-01 01:09:19]

Any issue with this CL?

[Lei Zhang, 2012-05-02 00:32:45]

On 2012/05/01 01:09:19, Marc-Antoine Ruel wrote:
> Any issue with this CL?

Whoops, I knew I forgot something. LGTM!

[commit-bot: I haz the power, 2012-05-02 12:44:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/10088002/17002

[commit-bot: I haz the power, 2012-05-04 22:24:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/10088002/26002

[commit-bot: I haz the power, 2012-05-05 00:45:13]

Change committed as 135495