1188224 - Remove stale .pyc files automatically

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See also bug 1010603, bug 685957.

glandium says "file a bug to have stale .pyc files removed (that is, .pyc files with no corresponding .py file)". That would help with some of the stale .pyc problems. This is that bug.

Comment 1

[Mike Hommey [:glandium]]

So, thinking about it, I think we can get around this with a simple hack around the knee example python module. The question is how to force it to be loaded everywhere. Greg, ideas? Maybe replace $PYTHON with something that invokes a wrapper script kind of like how pythonpath.py works?

Comment 2

[Gregory Szorc [:gps]]

When `python` processes start, they implicitly import the "site" module (https://docs.python.org/2/library/site.html). This makes an attempt to import a "sitecustomize" module, which commonly doesn't exist. But if you throw a sitecustomize.py file in <virtualenv>/bin/, you can have Python execute custom code very early during interpreter startup. I've leveraged sitecustomize.py files to do things like automatically enable code coverage or profiling when certain environment variables are present. It works really nice. Just watch out for performance overhead.

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

My colleague encountered a python import error in xpidl.py while building gecko. I told him to remove those .pyc files, which fixed his issue. We should fix this bug to save developers' time.

Comment 4

[Mike Hommey [:glandium]]

Attachment: PoC sitecustomize.py

Here is a PoC.

What this does is add a wrapper for import that:
- imports the module
- if the imported module was a .pyc, and there is no corresponding .py, remove the .pyc
- if the imported module was removed above, reimport it.

This needs refinement to handle .pyo files (that's why there's an endswith(('c', 'c')), because one of those 'c' was a 'o', but that removed .so modules too), and to avoid removing files outside virtualenv-managed stuff, but it works.

There is one downside, though: if the loaded .pyc has side effects (creating files, printing stuff, etc.) at import time, they will still happen (and rehappen when importing the new module if those things haven't changed).

I think it's a fine-ish downside, but it's also possible to do something more involved that would actually lookup the module first with imp.find_module but that would add more overhead.

Greg, what do you think?

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8641591 [details]
PoC sitecustomize.py

I'm not thrilled about the extra stat() on module import. But Python is already likely incurring over a dozen stat() during each import as it traverses sys.path, so I don't think this is a huge concern.

We'll definitely want to limit this to paths in the source directory. But I'm not sure how we'll do that, since sitecustomize.py is loaded *very* early. We may have to write out a dynamic sitecustomize.py containing a variable defining directories that are allowed to be pruned. Hacky.

Also, we'll need something similar for mach, since it doesn't use the virtualenv. (Although I would love for mach to bootstrap itself into a virtualenv as the first thing it does. It would solve a lot of problems to have everything running from the same virtualenv.)

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #3)
> My colleague encountered a python import error in xpidl.py while building
> gecko. I told him to remove those .pyc files, which fixed his issue. We
> should fix this bug to save developers' time.

I had a similar problem building Aurora after having built Central (presumably because of the changes to idl-parser in bug 1183291).

Comment 7

[Mike Hommey [:glandium]]

Attachment: Remove stale .pyc files from the source directory at import time

Comment 8

[Mike Hommey [:glandium]]

Attachment: Remove stale .pyc files from the source directory at import time

Err, forgot to remove a debug print.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8642904 [details] [diff] [review]
Remove stale .pyc files from the source directory at import time

Review of attachment 8642904 [details] [diff] [review]:
-----------------------------------------------------------------

Import hook looks good. sitecustomize, not so much.

::: python/mozbuild/mozbuild/virtualenv.py
@@ +332,5 @@
>              for package in packages:
>                  handle_package(package)
> +
> +            sitecustomize = os.path.join(
> +                os.path.dirname(os.__file__), 'sitecustomize.py')

In many environments, the system modules directory is shared between virtualenvs. So os.py will typically be <prefix>/lib/python2.7/os.py and this will write out <prefix>/lib/python2.7/sitecustomize.py, which if /usr/lib/python2.7/sitecustomize.py makes it global to all Python running on the machine. That is certainly not intended!

I'm pretty sure that putting a sitecustomize.py in the same directory as the Python interpreter executable will work. This file has code for resolving that path. Try that instead.

Please run this through Try before next review.

Comment 10

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 8642904 [details] [diff] [review]
> Remove stale .pyc files from the source directory at import time
> 
> Review of attachment 8642904 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Import hook looks good. sitecustomize, not so much.
> 
> ::: python/mozbuild/mozbuild/virtualenv.py
> @@ +332,5 @@
> >              for package in packages:
> >                  handle_package(package)
> > +
> > +            sitecustomize = os.path.join(
> > +                os.path.dirname(os.__file__), 'sitecustomize.py')
> 
> In many environments, the system modules directory is shared between
> virtualenvs.

We're talking about *our* virtualenvs. Do we ever do that?

> So os.py will typically be <prefix>/lib/python2.7/os.py and
> this will write out <prefix>/lib/python2.7/sitecustomize.py, which if
> /usr/lib/python2.7/sitecustomize.py makes it global to all Python running on
> the machine. That is certainly not intended!

Except that part of the script is run from within the virtualenv we've set up,
so what is going to be written is objdir/_virtualenv/lib/python2.7/sitecustomize.py.

> I'm pretty sure that putting a sitecustomize.py in the same directory as the
> Python interpreter executable will work. This file has code for resolving
> that path. Try that instead.

Running the virtualenv python never tries bin/sitecustomize.py:

28174 open("/home/glandium/build/obj-x86_64-unknown-linux-gnu/_virtualenv/lib/python2.7/sitecustomize.py", O_RDONLY) = -1 ENOENT (No such file or directory)
28174 open("/home/glandium/build/obj-x86_64-unknown-linux-gnu/_virtualenv/lib/python2.7/sitecustomize.pyc", O_RDONLY) = -1 ENOENT (No such file or directory)
28174 open("/home/glandium/build/obj-x86_64-unknown-linux-gnu/_virtualenv/lib/python2.7/lib-dynload/sitecustomize.py", O_RDONLY) = -1 ENOENT (No such file or directory)
28174 open("/home/glandium/build/obj-x86_64-unknown-linux-gnu/_virtualenv/lib/python2.7/lib-dynload/sitecustomize.pyc", O_RDONLY) = -1 ENOENT (No such file or directory)

Comment 11

[Gregory Szorc [:gps]]

Oh, hmm. I didn't realize virtualenv symlinks individual .py files from lib/python2.7 to the canonical system location. I always thought it just put the original system location on sys.path directly. In that case, this patch looks good.

Strangely, version-control-tools's virtualenv has sitecustomize.py in bin/sitecustomize.py and I think it "just works." Not sure why your system isn't searching there. I wouldn't rule out something hacky in version-control-tools.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/684252f11061

Comment 13

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/125b87d65303

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78ce50ffa083

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/78ce50ffa083