| msg121467 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2010-11-18 16:07 |
| It would be useful if “python -m compileall -h” was possible. Right now it fails with “option -h not recognized” and prints its help text, which is a bit silly :) Bug week-end candidate! |
|
|
| msg121470 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-11-18 16:12 |
| One neat way to solve this is to make compileall use argparse. |
|
|
| msg121673 - (view) |
Author: Michele Orrù (maker) * |
Date: 2010-11-20 16:23 |
| I'm still working on this task; the attachment shows how I'm solving the bug. The patch is NOT yet completed, there are some problems with the unittests. Hoping that Eric will give me a help soon. |
|
|
| msg121682 - (view) |
Author: Michele Orrù (maker) * |
Date: 2010-11-20 16:43 |
| The new attached patch passes the unittest. |
|
|
| msg121683 - (view) |
Author: Daniel Albeseder (Kotan) |
Date: 2010-11-20 17:02 |
| I did a very simple addition to the CommandLineTest, to check that "-h" really returns the "usage:". I am not sure, if this is useful since, it rather tests argparse now with the proposed patch... |
|
|
| msg121684 - (view) |
Author: Michele Orrù (maker) * |
Date: 2010-11-20 17:06 |
| Eric, the unittests in Lib/test/test_compileall.py seems quite consistent to me, so for now I won't add anything. About adding a method for testing the '-h' argument, now that Lib/compileall.py uses argparse, it sounds trivial. EDIT: Kotan, I'm still of the same opinion. Anyway, I think you should use as template test.test_compileall.CommandLineTests.test_legacy_paths: so, check for returncode and use subprocess.call instead of subprocess.Popen. |
|
|
| msg121686 - (view) |
Author: Daniel Albeseder (Kotan) |
Date: 2010-11-20 17:11 |
| I wanted to test, that no unwanted output is given before the usage. I just added the test before I started to try to fix this bug, as I recognized Michele already was already fixing the bug. This was just my test-driven approach, where the unit test failed before the fix, and should pass afterwards. As the unwanted output was the main reason for the issue, I just did put this into the test. However I agree that the test at all is silly, and I fully agree if it is not used. |
|
|
| msg121689 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2010-11-20 17:26 |
| On the other hand, the test case in test_compileall says "test some aspects of compileall's CLI". Since the patch completely changes the logic of CLI parsing, having tests that cover as much as practical of the CLI would greatly increase the confidence that the patch is correct. |
|
|
| msg121740 - (view) |
Author: Michele Orrù (maker) * |
Date: 2010-11-20 19:41 |
| Unittest added; should be enough. |
|
|
| msg121763 - (view) |
Author: Bruno Gola (brunogola) |
Date: 2010-11-20 20:42 |
| applied the patch on an ubuntu 10.04 64bits, py3k (trunk) test_force fails as following: ====================================================================== FAIL: test_force (__main__.CommandLineTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "Lib/test/test_compileall.py", line 202, in test_force self.assertNotEqual(access, access2) AssertionError: 1290285399.744327 == 1290285399.744327 ---------------------------------------------------------------------- |
|
|
| msg121772 - (view) |
Author: Michele Orrù (maker) * |
Date: 2010-11-20 20:57 |
| Yes, I was discussing about that on IRC. That's a matter of platform -on mine for example works-. He gave me a hand in solving this failure; now -I think- he's gonna apply that. |
|
|
| msg121774 - (view) |
Author: Michele Orrù (maker) * |
Date: 2010-11-20 20:58 |
| *discussing that on IRC with R. David Murray |
|
|
| msg121782 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2010-11-20 21:22 |
| Patch committed with minor formatting changes and one fixed test (test_force) in r86611. Thanks, Michele! |
|
|
| msg121808 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-11-20 22:58 |
| Invocation without arguments does not work. I have a fix, I’ll add a test and commit. |
|
|
| msg121809 - (view) |
Author: Michele Orrù (maker) * |
Date: 2010-11-20 23:04 |
| Sorry. |
|
|
| msg121858 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-11-21 03:56 |
| One buildbot also shows a bug in quiet or test_quiet: Traceback (most recent call last): File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows\build\lib\test\test_compileall.py", line 227, in test_quiet self.assertTrue(len(noise) > len(quiet)) AssertionError: False is not True I changed that to assertGreater to get more information in the next failure. |
|
|
| msg121859 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-11-21 03:59 |
| I hadn’t seen your patch Michele. I find mine more readable: http://pastealacon.com/26257 . That was just the easy part though; do you want to write a test? |
|
|
| msg121880 - (view) |
Author: Michele Orrù (maker) * |
Date: 2010-11-21 07:50 |
| Yeah, maybe your is more readable. I suppose that failure was due to some missing arguments when calling compileall (line 225). The attached patch should fix this issue, but currently I have no Windows machines where to test. |
|
|
| msg122017 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-11-21 23:23 |
| Patch for test_quiet looks good, thanks. Do you want to write test_noargs? |
|
|
| msg122073 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-11-22 02:43 |
| test_quiet hopefully fixed in r86668. |
|
|
| msg122085 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-11-22 03:43 |
| Not fixed: http://www.python.org/dev/buildbot/all/builders/AMD64%20Windows%20Server%202008%203.x/builds/54/steps/test/logs/stdio |
|
|
| msg122132 - (view) |
Author: Stefan Krah (skrah) * |
Date: 2010-11-22 14:57 |
| On Windows, test_compileall fails due to #10197. The patch uses subprocess.check_output() instead. Technically, now byte strings are compared instead of strings, but that should not matter for the outcome. |
|
|
| msg122397 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-11-25 19:25 |
| Thanks for the patch Stefan. I can’t test on Windows now; can you/have you? |
|
|
| msg122414 - (view) |
Author: Stefan Krah (skrah) * |
Date: 2010-11-25 22:31 |
| Yes, the patch is tested on Windows. Feel free to commit it if you have a chance. |
|
|
| msg122417 - (view) |
Author: Michele Orrù (maker) * |
Date: 2010-11-25 23:03 |
| Thank you Stefan, these days I was a little busy and I hadn't the time to review my patch. I really appreciate you help. |
|
|
| msg122422 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-11-26 00:45 |
| Revision 86758, thanks. |
|
|
| msg123492 - (view) |
Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) *  |
Date: 2010-12-06 19:03 |
| r86611 has introduced a regression: $ mkdir dir1 dir2 $ python3.1 -m compileall dir1 dir2 Listing dir1 ... Listing dir2 ... $ python3.2 -m compileall dir1 dir2 usage: compileall.py [-h] [-l] [-f] [-q] [-b] [-d DESTDIR] [-x REGEXP] [-i FILE] [FILE|DIR] compileall.py: error: unrecognized arguments: dir2 |
|
|
| msg123495 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-12-06 19:13 |
| Whoops, a nargs='?' should have been '*'. Who wants to write the test? |
|
|
| msg123496 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2010-12-06 19:15 |
| I'm working on it. |
|
|
| msg123497 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-12-06 19:19 |
| Let me be more helpful, just in case. This is the offending line: parser.add_argument('compile_dest', metavar='FILE|DIR', nargs='?') |
|
|
| msg123498 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2010-12-06 19:21 |
| Here's the test. The fix isn't as simple as making it nargs='*', though. |
|
|
| msg123500 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2010-12-06 19:45 |
| Here is a fix. This is not finished, though, because I see that I did not do an adequate review of the original patch. There are still bugs in the -d and -i handling that need both tests and fixes. |
|
|
| msg123621 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2010-12-08 17:39 |
| I'm still working on this, making sure the remaining options that aren't currently tested have tests and work. |
|
|
| msg123630 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-12-08 18:42 |
| Thank you for stepping up. I plead guilty too for letting bugs slip. I’ll be here for pre- or post-commit review. |
|
|
| msg123694 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2010-12-09 18:45 |
| OK, here is what I hope is a comprehensive set of CLI tests, and fixes for the bugs revealed thereby. Except for the new test added by Georg after the original patch here was committed, all of the tests either pass using the old compileall module or fail only because stderr has resource warnings in it. I did some refactoring on the tests, and since there were few enough original tests I went through and refactored them all. Hopefully that won't make the review more difficult. Note that I have not tested this patch on windows :) |
|
|
| msg123875 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2010-12-13 14:28 |
| Updating patch because the assertTestRegexMatches name was updated. |
|
|
| msg123877 - (view) |
Author: Ismail Donmez (donmez) * |
Date: 2010-12-13 14:38 |
| Patch works fine on Mac OSX 10.6.5 |
|
|
| msg123982 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-12-14 22:19 |
| Works under Windows 7. |
|
|
| msg123984 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2010-12-14 22:33 |
| committed in r87248. |
|
|
| msg123986 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-12-14 22:38 |
| Thanks for going through. Nice patch! |
|
|