classification
Title: Add -h/--help option to compileall
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: r.david.murray Nosy List: Arfrever, Kotan, brunogola, donmez, eric.araujo, ezio.melotti, maker, pitrou, r.david.murray, skrah
Priority: high Keywords: easy, patch

Created on 2010-11-18 16:07 by eric.araujo, last changed 2010-12-14 22:38 by eric.araujo. This issue is now closed.

Files
File name Uploaded Description Edit
test_compileall.patch Kotan, 2010-11-20 17:02 unit test, to test that -h returns usage and returns with error code 0
issue10453.patch maker, 2010-11-20 19:41
issue10453_tests.patch maker, 2010-11-20 19:41
issue10453_final.patch maker, 2010-11-20 20:27
issue10453_noargs.patch maker, 2010-11-20 23:04
issue10453_quiet.patch maker, 2010-11-21 07:50
test_compileall_windows.patch skrah, 2010-11-22 14:57
compileall_multidir_test.diff r.david.murray, 2010-12-06 19:21
compileall_multidir.diff r.david.murray, 2010-12-06 19:45
compileall_cli_revisited.patch r.david.murray, 2010-12-13 14:34
Messages (40)
msg121467 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2010-11-22 02:43
test_quiet hopefully fixed in r86668.
msg122085 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2010-11-26 00:45
Revision 86758, thanks.
msg123492 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * (Python triager) 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) * (Python committer) 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) * (Python committer) Date: 2010-12-06 19:15
I'm working on it.
msg123497 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2010-12-14 22:19
Works under Windows 7.
msg123984 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-14 22:33
committed in r87248.
msg123986 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-12-14 22:38
Thanks for going through.  Nice patch!
History
Date User Action Args
2010-12-14 22:42:42r.david.murraylinkissue10707 superseder
2010-12-14 22:38:04eric.araujosetresolution: accepted -> fixed
messages: + msg123986
2010-12-14 22:33:56r.david.murraysetstatus: open -> closed
resolution: accepted
messages: + msg123984

stage: test needed -> resolved
2010-12-14 22:19:34pitrousetnosy: + pitrou
messages: + msg123982
2010-12-13 14:38:42donmezsetmessages: + msg123877
2010-12-13 14:34:25r.david.murraysetfiles: + compileall_cli_revisited.patch
2010-12-13 14:34:00r.david.murraysetfiles: - compileall_cli_revisited.patch
2010-12-13 14:32:22r.david.murraysetnosy: + donmez
2010-12-13 14:31:51r.david.murraylinkissue10691 superseder
2010-12-13 14:29:37r.david.murraysetfiles: + compileall_cli_revisited.patch
2010-12-13 14:29:05r.david.murraysetfiles: - compileall_cli_revisited.patch
2010-12-13 14:28:59r.david.murraysetfiles: - compileall_cli_revisited.patch
2010-12-13 14:28:20r.david.murraysetfiles: + compileall_cli_revisited.patch

messages: + msg123875
2010-12-09 18:45:24r.david.murraysetfiles: + compileall_cli_revisited.patch

messages: + msg123694
2010-12-08 18:42:25eric.araujosetmessages: + msg123630
2010-12-08 17:39:32r.david.murraysetassignee: eric.araujo -> r.david.murray
messages: + msg123621
2010-12-06 19:45:57r.david.murraysetfiles: + compileall_multidir.diff

messages: + msg123500
2010-12-06 19:21:08r.david.murraysetfiles: + compileall_multidir_test.diff

messages: + msg123498
2010-12-06 19:19:28eric.araujosetmessages: + msg123497
2010-12-06 19:15:46r.david.murraysetmessages: + msg123496
2010-12-06 19:13:17eric.araujosetmessages: + msg123495
2010-12-06 19:03:10Arfreversetnosy: + Arfrever
messages: + msg123492
2010-12-03 06:47:15eric.araujosetpriority: normal -> high
stage: needs patch -> test needed
2010-11-26 00:45:09eric.araujosetmessages: + msg122422
2010-11-25 23:03:49makersetmessages: + msg122417
2010-11-25 22:31:04skrahsetmessages: + msg122414
2010-11-25 19:25:55eric.araujosetmessages: + msg122397
2010-11-22 14:57:15skrahsetfiles: + test_compileall_windows.patch
nosy: + skrah
messages: + msg122132

2010-11-22 14:47:33eric.araujolinkissue10505 superseder
2010-11-22 03:43:58eric.araujosetmessages: + msg122085
2010-11-22 02:43:46eric.araujosetmessages: + msg122073
2010-11-21 23:23:12eric.araujosetmessages: + msg122017
2010-11-21 07:50:40makersetfiles: + issue10453_quiet.patch

messages: + msg121880
2010-11-21 03:59:26eric.araujosetmessages: + msg121859
2010-11-21 03:56:45eric.araujosetstatus: closed -> open
messages: + msg121858

assignee: r.david.murray -> eric.araujo
resolution: accepted -> (no value)
stage: resolved -> needs patch
2010-11-20 23:04:14makersetfiles: + issue10453_noargs.patch

messages: + msg121809
2010-11-20 22:58:08eric.araujosetmessages: + msg121808
2010-11-20 21:22:18r.david.murraysetstatus: open -> closed
resolution: accepted
messages: + msg121782

stage: commit review -> resolved
2010-11-20 21:04:35r.david.murraysetassignee: eric.araujo -> r.david.murray
stage: needs patch -> commit review
2010-11-20 20:58:05makersetmessages: + msg121774
2010-11-20 20:57:11makersetmessages: + msg121772
2010-11-20 20:42:04brunogolasetnosy: + brunogola
messages: + msg121763
2010-11-20 20:27:40makersetfiles: + issue10453_final.patch
2010-11-20 19:41:34makersetfiles: + issue10453_tests.patch
2010-11-20 19:41:21makersetfiles: + issue10453.patch

messages: + msg121740
2010-11-20 19:40:47makersetfiles: - issue10453.patch
2010-11-20 17:26:35r.david.murraysetnosy: + r.david.murray
messages: + msg121689
2010-11-20 17:11:41Kotansetmessages: + msg121686
2010-11-20 17:06:03makersetmessages: + msg121684
2010-11-20 17:02:37Kotansetfiles: + test_compileall.patch
nosy: + Kotan
messages: + msg121683

2010-11-20 16:55:49makersetfiles: + issue10453.patch
2010-11-20 16:55:33makersetfiles: - issue10453.patch
2010-11-20 16:43:06makersetfiles: + issue10453.patch

messages: + msg121682
2010-11-20 16:41:53makersetfiles: - issue10453.patch
2010-11-20 16:23:55makersetfiles: + issue10453.patch

nosy: + ezio.melotti, maker
messages: + msg121673

keywords: + patch
2010-11-18 16:12:28eric.araujosetmessages: + msg121470
2010-11-18 16:07:05eric.araujocreate