Today I was hacking on a patch that changes the marshaling format for Code objects. When building the patch I was met with:

ValueError: bad marshal data (unknown type code)
make: *** [Lib/_sysconfigdata.py] Abort trap: 6

This is because the frozen importlib was not rebuilt with the new marshaling format.

Patch attached.

Looks good to me. It probably won't cover all cases (such as e.g. changing the bytecode format), but it's a good step forward.

FYI: the .pyc magic number is now built/kept in Lib/importlib/_bootstrap.py, so updates to it will trigger a rebuild of frozen importlib during make.

What I mean is, shouldn't changes to marshal have an accompanying bump to the pyc magic number?

Eric, that is a good point, but if someone forgets (like I did) or just hasn't gotten around to bumping the number yet, then the build breaks because the interpreter crashes. I think we should always try to avoid building an interpreter that is in an inconsistent state.

Anyway, I am hitting another problem now -- _freeze_importlib is *not* idempotent. Thus adding this rule might cause some noise in the builds because importlib.h is different when nothing has actually changed. I am still investigating that problem.

Assuming I can fix the idempotency problem, then maybe _freeze_importlib should just be always run.

Hmmm, I guess the idempotency issue is no worse than it already is -- the
same thing can still happen with trivial changes to the other prerequisites
for importlib.h.

Consider this small example (you might have to run sample program multiple
times to see a difference):

$ cat dis-closure.py
import dis

def adder(a, b):
    def add():
        return a + b
    return add

print(dis.dis(adder(1, 2).__code__))

$  ./python.exe dis-closure.py
  5           0 LOAD_DEREF               0 (a) 
              3 LOAD_DEREF               1 (b) 
              6 BINARY_ADD           
              7 RETURN_VALUE         
None
$  ./python.exe dis-closure.py
  5           0 LOAD_DEREF               1 (a) 
              3 LOAD_DEREF               0 (b) 
              6 BINARY_ADD           
              7 RETURN_VALUE         
None

The order of 'co_cellvars', 'co_varnames', and 'co_freevars' can be
different from compile to compile, thus the bytecode can be different
from compile to compile (I am not sure if this is worth fixing).

Thus there may be times where importlib.h is regenerated, but the changes
in the bytecode aren't significant.

I will just commit this patch as is.

Anyway, I am hitting another problem now -- _freeze_importlib is *not* idempotent.

What do you mean with "idempotent"? Deterministic?

Le lundi 16 juillet 2012 à 05:42 +0000, Meador Inge a écrit :

The order of 'co_cellvars', 'co_varnames', and 'co_freevars' can be
different from compile to compile, thus the bytecode can be different
from compile to compile (I am not sure if this is worth fixing).

I don't know, but you could open an issue just for the record.
People may be surprised if bytecode generation isn't deterministic.

On Mon, Jul 16, 2012 at 5:20 AM, Antoine Pitrou <report@bugs.python.org> wrote:

> Anyway, I am hitting another problem now -- _freeze_importlib is *not* idempotent.

What do you mean with "idempotent"? Deterministic?

Whoops, yeah, deterministic. I got mixed up with my terminology.

On Mon, Jul 16, 2012 at 7:13 AM, Antoine Pitrou <report@bugs.python.org> wrote:

I don't know, but you could open an issue just for the record.
People may be surprised if bytecode generation isn't deterministic.

Yeah, I was somewhat surprised and it took some digging to figure
out exactly what was changing. Opened bpo-15368 to track this.

Meador, is this still a problem? There was a flurry of activity in this area.

Yup, it is still an issue. The recent activity was mostly related to Windows builds (bpo-15431).

FWIW, the patch looks good to me. This is probably the last week to get this in for 3.3.0.

New changeset 344f44bd793f by Eric Snow in branch 'default':
Issue bpo-15352: Rebuild frozen modules when marshal.c is changed.
https://hg.python.org/cpython/rev/344f44bd793f

Thanks for the patch Meador.

Hmmm, not sure why I forgot to apply this myself. Thanks for committing it Eric.