Add -h/--help option to compileall

[merwok]

BPO	10453
Nosy	@pitrou, @ezio-melotti, @merwok, @bitdancer, @skrah, @mmaker
Files	test_compileall.patch: unit test, to test that -h returns usage and returns with error code 0 issue10453.patch issue10453_tests.patch issue10453_final.patch issue10453_noargs.patch issue10453_quiet.patch test_compileall_windows.patch compileall_multidir_test.diff compileall_multidir.diff compileall_cli_revisited.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/bitdancer'
closed_at = <Date 2010-12-14.22:33:56.625>
created_at = <Date 2010-11-18.16:07:05.010>
labels = ['easy', 'type-feature', 'library']
title = 'Add -h/--help option to compileall'
updated_at = <Date 2010-12-14.22:38:04.325>
user = 'https://github.com/merwok'

bugs.python.org fields:

activity = <Date 2010-12-14.22:38:04.325>
actor = 'eric.araujo'
assignee = 'r.david.murray'
closed = True
closed_date = <Date 2010-12-14.22:33:56.625>
closer = 'r.david.murray'
components = ['Library (Lib)']
creation = <Date 2010-11-18.16:07:05.010>
creator = 'eric.araujo'
dependencies = []
files = ['19685', '19702', '19703', '19708', '19717', '19728', '19772', '19960', '19961', '20031']
hgrepos = []
issue_num = 10453
keywords = ['patch', 'easy']
message_count = 40.0
messages = ['121467', '121470', '121673', '121682', '121683', '121684', '121686', '121689', '121740', '121763', '121772', '121774', '121782', '121808', '121809', '121858', '121859', '121880', '122017', '122073', '122085', '122132', '122397', '122414', '122417', '122422', '123492', '123495', '123496', '123497', '123498', '123500', '123621', '123630', '123694', '123875', '123877', '123982', '123984', '123986']
nosy_count = 10.0
nosy_names = ['pitrou', 'donmez', 'brunogola', 'ezio.melotti', 'eric.araujo', 'Arfrever', 'r.david.murray', 'skrah', 'maker', 'Kotan']
pr_nums = []
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue10453'
versions = ['Python 3.2']

[merwok]

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!

[merwok]

One neat way to solve this is to make compileall use argparse.

[mmaker]

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.

[mmaker]

The new attached patch passes the unittest.

[Kotan]

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...

[mmaker]

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.

[Kotan]

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.

[bitdancer]

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.

[mmaker]

Unittest added; should be enough.

[brunogola]

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

---

[mmaker]

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.

[mmaker]

*discussing that on IRC with R. David Murray

[bitdancer]

Patch committed with minor formatting changes and one fixed test (test_force) in r86611. Thanks, Michele!

[merwok]

Invocation without arguments does not work. I have a fix, I’ll add a test and commit.

[mmaker]

Sorry.

[merwok]

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.

[merwok]

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?

[mmaker]

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.

[merwok]

Patch for test_quiet looks good, thanks.

Do you want to write test_noargs?

[merwok]

test_quiet hopefully fixed in r86668.

[merwok]

Not fixed: http://www.python.org/dev/buildbot/all/builders/AMD64%20Windows%20Server%202008%203.x/builds/54/steps/test/logs/stdio

[skrah]

On Windows, test_compileall fails due to bpo-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.

[merwok]

Thanks for the patch Stefan. I can’t test on Windows now; can you/have you?

[skrah]

Yes, the patch is tested on Windows. Feel free to commit it if you have
a chance.

[mmaker]

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.

[merwok]

Revision 86758, thanks.

[Arfrever]

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

[merwok]

Whoops, a nargs='?' should have been '*'. Who wants to write the test?

[bitdancer]

I'm working on it.

[merwok]

Let me be more helpful, just in case. This is the offending line:
parser.add_argument('compile_dest', metavar='FILE|DIR', nargs='?')

[bitdancer]

Here's the test. The fix isn't as simple as making it nargs='*', though.

[bitdancer]

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.

[bitdancer]

I'm still working on this, making sure the remaining options that aren't currently tested have tests and work.

[merwok]

Thank you for stepping up. I plead guilty too for letting bugs slip. I’ll be here for pre- or post-commit review.

[bitdancer]

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 :)

[bitdancer]

Updating patch because the assertTestRegexMatches name was updated.

[donmez]

Patch works fine on Mac OSX 10.6.5

[pitrou]

Works under Windows 7.

[bitdancer]

committed in r87248.

[merwok]

Thanks for going through. Nice patch!