Bug 36599 – test-webkitpy: Should remove orphaned *.pyc files in webkitpy/ prior to importing from webkitpy/

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

This should increase the robustness of test-webkitpy since it eliminates the possibility of test-webkitpy passing because of *.pyc files left-over from a batch of file moves.

[Eric Seidel (no email)]

Huh?  To test-webkitpy would always cause a full python recompile?  That seems like a bad idea.

[Chris Jerdonek]

(In reply to comment #1)
> Huh?  To test-webkitpy would always cause a full python recompile?  That seems
> like a bad idea.

I noticed no perceptible difference between running test-webkitpy with and without  deleting all *.pyc files before-hand.  It seems like recompiling might be an order of magnitude or two less than running the tests themselves.  (I tried to time the difference, but unittest.main() exits before you can execute a final line.)

Do you have a suggestion for an alternative?  We could always put this behind a --clean option, but I think it would be good for the bots to do whatever we come up with by default.

One alternative might be to delete any .pyc file that doesn't have a corresponding .py, but I don't think that covers all the cases we want to deal with.  Or maybe delete any *.pyc file that doesn't correspond to a source-controlled (tracked) *.py file, but I'm also not sure that covers all the scenarios.  Given that the compile is so fast, it might not be worth the trouble.

[Eric Seidel (no email)]

We can certainly try it.  I wonder how it will interact with things like the commit-queue which are written in python and run test-webkitpy before committing. :)

[Chris Jerdonek]

(In reply to comment #2)
> One alternative might be to delete any .pyc file that doesn't have a
> corresponding .py, but I don't think that covers all the cases we want to deal
> with.

After thinking about this, the above may be the correct alternative.  This should address the main case I had in mind which was moving .py files in source control and leaving behind orphaned .pyc files that were still being referenced by import statements.  (This has occasionally caused problems in practice.)   If there are untracked .py files still being referenced, though, I'm not sure how much we can really do about that (or in practice need to) since in problem scenarios those would get recompiled anyways.

> Or maybe delete any *.pyc file that doesn't correspond to a
> source-controlled (tracked) *.py file, but I'm also not sure that covers all
> the scenarios.


(In reply to comment #3)
> We can certainly try it.  I wonder how it will interact with things like the
> commit-queue which are written in python and run test-webkitpy before
> committing. :)

A related issue to consider is that we may want other scripts (e.g. update-webkit) to trigger a *.pyc "clean" action ("clean" in the sense of the first alternative above).  This way if there are orphaned *.pyc files in someone's webkitpy, the Python scripts will never pick up the wrong ones (e.g. if import finds multiple potential matches in sys.path).

[Chris Jerdonek]

Created attachment 51730 [details]
Proposed patch

This would have come in handy during the re-org. :)

[Chris Jerdonek]

Created attachment 51732 [details]
Proposed patch 2

Reduced the maximum indent level by one.

[Eric Seidel (no email)]

Are we sure this is actually a bug?  Do we know how to reproduce it?  If so, we should file a bug with python before working around it here.

[Chris Jerdonek]

(In reply to comment #7)
> Are we sure this is actually a bug?  Do we know how to reproduce it?

Yes, we can reproduce it.  Just "svn move" any python module (without touching any of the code), and then run test-webkitpy.  It still succeeds.  With this patch, it correctly fails with an error:

ImportError: No module named ...

> If so, we
> should file a bug with python before working around it here.

It's a Python feature, not a bug. :)

"It is possible to have a file called spam.pyc (or spam.pyo when -O is used) without a file spam.py for the same module. This can be used to distribute a library of Python code in a form that is moderately hard to reverse engineer."

(from http://docs.python.org/tutorial/modules.html#compiled-python-files )

[Chris Jerdonek]

(In reply to comment #7)
> Are we sure this is actually a bug?  Do we know how to reproduce it?  If so, we
> should file a bug with python before working around it here.

The problem is also stated here, for instance:

http://bugs.python.org/issue1180193#msg24989

[Eric Seidel (no email)]

Comment on attachment 51732 [details]
Proposed patch 2

OK.  I think it needs a unit test though.

[Chris Jerdonek]

So (In reply to comment #10)
> (From update of attachment 51732 [details])
> OK.  I think it needs a unit test though.

This seems a bit excessive to me since we'd be unit testing unit-test code.  We don't normally do that, but I could do it.

In this case, since the method needs to be defined in test-webkitpy, the unit test would either need to go in test-webkitpy itself, or we would need a module somewhere in webkitpy (say webkitpy/test/main_unittest.py) that would need to import this method from test-webkitpy.  (In particular, the method would be getting unit-tested after it has already executed.)

Clarify for me by r-'ing the patch if this is required.  Thanks.

[Chris Jerdonek]

(In reply to comment #11)
> In this case, since the method needs to be defined in test-webkitpy, the unit
> test would either need to go in test-webkitpy itself, or we would need a module
> somewhere in webkitpy (say webkitpy/test/main_unittest.py) that would need to
> import this method from test-webkitpy.

Maybe the simplest approach to test that this method is doing what it's supposed to do would be to insert a mock .pyc file somewhere in webkitpy before the method call and then raise an error afterwards if that particular file hasn't been deleted.  I.e. the call and the test would be one and the same.

[Eric Seidel (no email)]

The problem is that this code is "complicated" and difficult to tell when it's broken.  Which is why I suggested testing.  I agree, it seems kinda silly to test the test code.   Your solution of dropping a some sort of testing .pyc file makes sense.

[Chris Jerdonek]

Created attachment 51812 [details]
Proposed patch

[Chris Jerdonek]

Created attachment 51814 [details]
Proposed patch 4

Added link to Python documentation.

[Eric Seidel (no email)]

Comment on attachment 51814 [details]
Proposed patch 4

Looks sane.

[WebKit Commit Bot]

Comment on attachment 51814 [details]
Proposed patch 4

Rejecting patch 51814 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel', '--force']" exit_code: 1
patching file WebKitTools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKitTools/Scripts/test-webkitpy
Hunk #1 FAILED at 29.
Hunk #2 succeeded at 79 (offset 1 line).
1 out of 2 hunks FAILED -- saving rejects to file WebKitTools/Scripts/test-webkitpy.rej

Full output: http://webkit-commit-queue.appspot.com/results/1605082

[Chris Jerdonek]

Manually committed after rebasing:

http://trac.webkit.org/changeset/56794