Remove stale .pyc files automatically

RESOLVED FIXED in Firefox 42

Status

Firefox Build System
General
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: roc, Assigned: glandium)

Tracking

Trunk
mozilla42

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
(Assignee)

Comment 1

3 years ago
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?
Flags: needinfo?(gps)

Comment 2

3 years ago
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.
Flags: needinfo?(gps)
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8641591 [details]
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?
Assignee: nobody → mh+mozilla
Attachment #8641591 - Flags: feedback?(gps)

Comment 5

3 years ago
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.)
Attachment #8641591 - Flags: feedback?(gps) → feedback+
(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).
(Assignee)

Comment 7

3 years ago
Created attachment 8642903 [details] [diff] [review]
Remove stale .pyc files from the source directory at import time
Attachment #8641591 - Attachment is obsolete: true
Attachment #8642903 - Flags: review?(gps)
(Assignee)

Comment 8

3 years ago
Created attachment 8642904 [details] [diff] [review]
Remove stale .pyc files from the source directory at import time

Err, forgot to remove a debug print.
Attachment #8642903 - Attachment is obsolete: true
Attachment #8642903 - Flags: review?(gps)
Attachment #8642904 - Flags: review?(gps)

Comment 9

3 years ago
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.
Attachment #8642904 - Flags: review?(gps)
(Assignee)

Comment 10

3 years ago
(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)
Flags: needinfo?(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.
Flags: needinfo?(gps)

Updated

3 years ago
Attachment #8642904 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/78ce50ffa083
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Updated

3 years ago
Duplicate of this bug: 729752

Updated

3 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.