When trying to decide whether or not a given file needs to be
recompiled, the inode creation time should be taken into account along
with the file modification time.

Scenario: Suppose you have three times, A < B < C < D. At time A, you
package version 1 of foo.py. At time B, you package version 2. At time C
you install version 1 onto some system, and byte-compile it for all
users. Therefore foo.py has mtime A and ctime C, and foo.pyc has both
mtime C. At time D you delete foo.py from version 1 and install version
2. Then you byte-compile it without force. At that time, foo.py has
mtime B (because that was when it was packaged) but ctime D (because
that was when the file was created). foo.pyc has mtime C (because that
was when it was last modified). The current comparison compares only
mtimes, sees C > B, and skips foo.py. With this patch in place, it will
check for C > max(B, D), which is not the case, and thus recompile foo.c.

In other words, max(st_mtime, st_ctime) can be interpreted as the last
time a file at a given path was changed, either through modification of
its contents (mtime) or deletion followed by recreation (ctime).
Installation systems that overwrite files without deleting them before
still are in the same trouble as before, but all others will benefit
from this.

See also: http://bugs.gentoo.org/256953

The problem is not that ctime should be taken into account, but that the
.pyc file should be read for its timestamp and that should be used.
Otherwise you are still deviating from what Python uses internally to
decide whether bytecode should be regenerated.

Like this? Should the magic number be checked as well, and if so,
against what value? I couldn't find that constant in any structure
accessible from python, and jumping through hoops seems too much, as
updating the python version should probably result in all system-wide
modules being updated as well, at least on a well-maintained system.

On Mon, Feb 2, 2009 at 12:22, Martin von Gagern <report@bugs.python.org> wrote:
>
> Martin von Gagern <Martin.vGagern@gmx.net> added the comment:
>
> Like this?

Don't have the time right now to do a code review right now, but
hopefully I can get to this soon.

> Should the magic number be checked as well,

If one is already reading the file you might as well.

> and if so,
> against what value?

imp.get_magic().

If you want an easy way to see how bytecode is checked, look at
importlib._bootstrap in Python 3.1:
http://svn.python.org/view/python/branches/py3k/Lib/importlib/_bootstrap.py?view=markup
. Specifically, look at the get_code() method for _PyFileLoader.

Next iteration. With magic number, and now also closing the file again.
I changed from unpack and number comparison to pack and string
comparison, makes things a bit easier, as there is only one comparison,
and as underflow of the packed data isn't an issue any more.

Would you prefer the use of private marshal functions, as bootstrap
does, over the struct packing as I do here?

Looking forward to your review.

On Mon, Feb 2, 2009 at 12:42, Martin von Gagern <report@bugs.python.org> wrote:
>
> Martin von Gagern <Martin.vGagern@gmx.net> added the comment:
>
> Next iteration. With magic number, and now also closing the file again.
> I changed from unpack and number comparison to pack and string
> comparison, makes things a bit easier, as there is only one comparison,
> and as underflow of the packed data isn't an issue any more.
>
> Would you prefer the use of private marshal functions, as bootstrap
> does, over the struct packing as I do here?
>

No since importlib only does it that way for bootstrapping
considerations (might end up at the implementation of import itself so
it has special requirements).

> Looking forward to your review.

I have not looked, Martin, but are proper tests in place already in
the standard library? If not I would appreciate tests.

On Mon, Feb 2, 2009 at 13:50, Brett Cannon <brett@python.org> wrote:
> On Mon, Feb 2, 2009 at 12:42, Martin von Gagern <report@bugs.python.org> wrote:
>>
>> Martin von Gagern <Martin.vGagern@gmx.net> added the comment:
>>
>> Next iteration. With magic number, and now also closing the file again.
>> I changed from unpack and number comparison to pack and string
>> comparison, makes things a bit easier, as there is only one comparison,
>> and as underflow of the packed data isn't an issue any more.
>>
>> Would you prefer the use of private marshal functions, as bootstrap
>> does, over the struct packing as I do here?
>>
>
> No since importlib only does it that way for bootstrapping
> considerations (might end up at the implementation of import itself so
> it has special requirements).
>
>> Looking forward to your review.
>
> I have not looked, Martin, but are proper tests in place already in
> the standard library? If not I would appreciate tests.
>

Although, now that I said that, I have had testing issues myself when
it comes to mtime on bytecode as Python tends to execute faster than
the granularity the file system supports.

Patch is really close. Can you use a context manager for the file
management? That way the file is guaranteed to be closed without issue.

Not being a regular Python programmer myself, I've never even heard of a
context manager before, but this latest patch should fit the bill.

As for tests, I believe that a few two-second sleeps (as FAT has only
two second resolution iirc) should avoid the kind of problem you
describe. This latest patch includes unit tests, where the test_backdate
fails for current implementation. The others are there to express even
more basic assumptions, and can be dropped if you worry about those
seconds spent running obvious tests. The tests are based on
test_dircache, from which I copied and adapted the infrastructure for
dealing with a temporary directory.

Any progress with the review?

By the way, what branch are we aiming for? I'm using 2.5 here, so I
reported this issue against that version, and wrote the patch against
that branch. I guess it should work for trunk as well, but the imports
of with_statement from __future__ should probably be omitted there.

On Sun, Feb 8, 2009 at 09:15, Martin von Gagern <report@bugs.python.org> wrote:
>
> Martin von Gagern <Martin.vGagern@gmx.net> added the comment:
>
> Any progress with the review?
>

I am planning to get to it on Tuesday.

> By the way, what branch are we aiming for?

2.7/3.1. It's a backwards-incompatible change so I am not planning on
backporting it.

Committed in 69481 and 69482 for trunk and py3k, respectively. Had to
rewrite the test code but the compileall patch went in fine. Thanks, Martin!

This patch introduces a slight change in behaviour.

Now compilation will only happen if there is a difference in the number of seconds in the timestamp of files, before this patch any difference in mtime will trigger a rebuild. This is because the timestamp in the pyc file is an integer number of seconds, while mtime from stat is a float time.

There is no patch, Djoume, but honestly that's fine since if you want to submit a change to something it should go in a new issue.

But honestly compileall needs to be rewritten in Python 3.4 to use importlib and have it control when source code should be rebuilt since the .pyc format changed in Python 3.3 to include source file size.