Derby #1: Convert 137 sites to Argument Clinic in Modules/posixmodule.c

[larryhastings]

BPO	20170
Nosy	@loewis, @larryhastings, @zware, @serhiy-storchaka
Files	larry.clinicize.posixmodule.1.diff larry.clinicize.posixmodule.2.diff larry.clinicize.posixmodule.3.diff larry.clinicize.posixmodule.4.diff: attempt Update Python Software Foundation Copyright Year. #4 larry.clinicize.posixmodule.5.diff larry.clinicize.posixmodule.6.diff larry.clinicize.posixmodule.7.diff

^{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/larryhastings'
closed_at = <Date 2014-08-05.04:08:51.455>
created_at = <Date 2014-01-07.23:43:05.768>
labels = ['extension-modules', 'easy', 'type-feature']
title = 'Derby #1: Convert 137 sites to Argument Clinic in Modules/posixmodule.c'
updated_at = <Date 2014-08-05.04:08:51.454>
user = 'https://github.com/larryhastings'

bugs.python.org fields:

activity = <Date 2014-08-05.04:08:51.454>
actor = 'larry'
assignee = 'larry'
closed = True
closed_date = <Date 2014-08-05.04:08:51.455>
closer = 'larry'
components = ['Extension Modules']
creation = <Date 2014-01-07.23:43:05.768>
creator = 'larry'
dependencies = []
files = ['33789', '33797', '33798', '36136', '36164', '36172', '36240']
hgrepos = []
issue_num = 20170
keywords = ['patch', 'easy']
message_count = 22.0
messages = ['207615', '209620', '209644', '209646', '209648', '209659', '209662', '209702', '224117', '224158', '224172', '224183', '224308', '224320', '224370', '224428', '224510', '224669', '224716', '224744', '224799', '224800']
nosy_count = 5.0
nosy_names = ['loewis', 'larry', 'python-dev', 'zach.ware', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue20170'
versions = ['Python 3.5']

[larryhastings]

This issue is part of the Great Argument Clinic Conversion Derby,
where we're trying to convert as much of Python 3.4 to use
Argument Clinic as we can before Release Candidate 1 on January 19.

This issue asks you to change the following bundle of files:
Modules/posixmodule.c: 137 sites

Talk to me (larry) if you only want to attack part of a bundle.

For instructions on how to convert a function to work with Argument
Clinic, read the "howto":
http://docs.python.org/dev/howto/clinic.html

[larryhastings]

Submitting this just so I beat the deadline. I'm *about* half done, but I'm still working on it, so I'm just going to keep going--should only be another couple of hours.

(If somebody else pulls this stunt, I guess I'll accept their final patch too.)

[larryhastings]

Here's a complete patch, converts everything that I think should be converted for 3.4. With this patch applied, all unit tests pass on my 64-bit Linux box. I plan to also run tests with the buildbots before checking it in.

The patch... well, it's 14,000 lines. 409k bytes. Do we have any takers?

[larryhastings]

By the way, my plan is to turn on the file preset just before checkin. The patch is *much* easier to read without turning that on first; with the file preset, now you have to keep two windows in sync to compare calls to PyArg_*().

[larryhastings]

Sorry for the fresh update, but here's revision 3. Only changes:

• Gave os.access a -> bool return converter.

• Fixed up a lot of whitespace. Now, major "things" are separated by
  two empty lines, but removed whitespace between

    #ifdef HAVE_SOMETHING

and

/*[clinic input]
os.something

(I used to randomly have blank lines there. No more!)

[larryhastings]

Actually, forget about the file output preset. It wouldn't work for posixmodule. > 80% of the entry points are #ifdef conditional on platform functionality. Which means the Clinic generated stuff needs to be #ifdef too.

It wouldn't be that hard to add the ability to #ifdef your generated code... but then what? Should it generate an #endif too, right before the end of the block?

If it closed the #ifdef, then it'd look dumb:

    /*[clinic input]*
    ifdef HAVE_WHATNOT
    os.whatnot
    [clinic start generated code]*/
    #ifdef HAVE_WHATNOT
    ...
    static PyObject *
    os_whatnot(PyModuleType *)
    #endif /* HAVE_WHATNOT */
    /*[clinic end generated code: output=... ]*/
    #ifdef HAVE_WHATNOT
    {
        ...
    }
    #endif /* HAVE_WHATNOT */

If it didn't close the #ifdef, well, that looks dumb too:

    /*[clinic input]*
    ifdef HAVE_WHATNOT
    os.whatnot
    [clinic start generated code]*/
    #ifdef HAVE_WHATNOT
    ...
    static PyObject *
    os_whatnot(PyModuleType *)
    /*[clinic end generated code: output=... ]*/
    {
        ...
    }
    #endif /* HAVE_WHATNOT */

though maybe that's less dumb.

[serhiy-storchaka]

The curses module also has many conditionally implemented functions.

I think Argument Clinic can detect preprocessor instructions (#if/#ifdef/#ifndef/#else/#endif) and generate needed #if/#endif in generated file. This would be more robust than explicitly repeat condition in clinic declaration, because external conditions can be changed.

[larryhastings]

That's a really good idea! I'm still thinking about how I'd do it, but I think I'm gonna give it a try.

[loewis]

This patch doesn't apply anymore (to c55300337932); please update it.

[larryhastings]

Here's an updated patch. I cleaned it up a little. I think it's about ready to go in.

Zachary, iirc you're a Windows guy and have helped with ensuring patches apply cleanly to Windows in the past. Can you give this a try on Windows?

[zware]

MSVC is not happy, here's some build output:

"P:\ath\to\cpython\PCbuild\pcbuild.sln" (Build target) (1) ->
"P:\ath\to\cpython\PCbuild\python.vcxproj" (default target) (2) ->
"P:\ath\to\cpython\PCbuild\pythoncore.vcxproj" (default target) (3) ->
(ClCompile target) ->
..\Modules\posixmodule.c(2886): warning C4047: 'initializing' : 'int' differs in levels of indirection from 'void *' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(2896): warning C4047: 'return' : 'int' differs in levels of indirection from 'void *' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(2900): warning C4047: 'return' : 'int' differs in levels of indirection from 'void *' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(4865): warning C4047: '=' : 'int' differs in levels of indirection from 'Py_UNICODE *' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(4866): warning C4047: '==' : 'int' differs in levels of indirection from 'void *' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(4877): warning C4047: 'function' : 'LPCWSTR' differs in levels of indirection from 'int' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(4877): warning C4024: 'CreateFileW' : different types for formal and actual parameter 1 [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(5368): warning C4047: 'function' : 'path_t *' differs in levels of indirection from 'path_t **' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(5368): warning C4024: 'path_error2' : different types for formal and actual parameter 1 [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(5368): warning C4047: 'function' : 'path_t *' differs in levels of indirection from 'path_t **' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(5368): warning C4024: 'path_error2' : different types for formal and actual parameter 2 [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(7010): warning C4031: second formal parameter list longer than the first list [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(9894): warning C4013: 'waitpid' undefined; assuming extern returning int [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(10428): warning C4047: 'function' : 'path_t *' differs in levels of indirection from 'path_t **' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(10428): warning C4024: 'path_error2' : different types for formal and actual parameter 1 [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(10428): warning C4047: 'function' : 'path_t *' differs in levels of indirection from 'path_t **' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(10428): warning C4024: 'path_error2' : different types for formal and actual parameter 2 [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(16433): warning C4047: 'return' : 'int' differs in levels of indirection from 'void *' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]

"P:\ath\to\cpython\PCbuild\pcbuild.sln" (Build target) (1) ->
"P:\ath\to\cpython\PCbuild\python.vcxproj" (default target) (2) ->
"P:\ath\to\cpython\PCbuild\pythoncore.vcxproj" (default target) (3) ->
(ClCompile target) ->
..\Modules\posixmodule.c(3324): error C2231: '.wide' : left operand points to 'struct', use '->' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(4863): error C2082: redefinition of formal parameter 'path' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(4865): error C2065: 'path_wchar' : undeclared identifier [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(4866): error C2065: 'path_wchar' : undeclared identifier [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(4877): error C2065: 'path_wchar' : undeclared identifier [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(7004): error C2370: 'os_spawnv__doc__' : redefinition; different storage class [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(7014): error C2084: function 'PyObject *os_spawnv(PyModuleDef *,PyObject *)' already has a body [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(7037): error C2084: function 'PyObject *os_spawnv_impl(PyModuleDef *,int,PyObject *,PyObject *,PyObject *)' already has a body [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(9019): error C2082: redefinition of formal parameter 'pid' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(10936): error C2231: '.wide' : left operand points to 'struct', use '->' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(13997): error C2085: 'win32__getdiskusage' : not informal parameter list [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(13997): error C2143: syntax error : missing ';' before '{' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(16483): error C2085: 'posix_set_handle_inheritable' : not in formal parameter list [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(16483): error C2143: syntax error : missing ';' before '{' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17051): error C2065: 'OS_SPAWNVE_METHODDEF' : undeclared identifier [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17051): error C2099: initializer is not a constant [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17051): error C2059: syntax error : '{' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17051): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17053): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17055): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17056): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17060): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17078): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17084): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17085): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17086): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17087): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17088): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17089): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17091): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17092): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17095): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17102): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17103): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17104): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17115): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17117): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17119): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17136): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17138): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17139): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17141): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17142): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17143): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17145): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17157): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17159): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17160): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17161): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17162): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17163): error C2059: syntax error : ',' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17165): error C2059: syntax error : '}' [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\posixmodule.c(17175): fatal error C1903: unable to recover from previous error(s); stopping compilation [P:\ath\to\cpython\PCbuild\pythoncore.vcxproj]

18 Warning(s)
53 Error(s)

[larryhastings]

thanks! I'm flying from London to Brisbane (via Singapore), gonna take about a day. Now I have something to do on the flight ;-)

(that and nullable ints)

[larryhastings]

Here's a fresh diff. I did some cleanup this time (Clinic now generates the #ifndef versions of the METHODDEF structures) and I believe solved everything MSVC complains about.

Zachary, can you try this one?

[zware]

Close, but no cigar :). Posted Rietveld comments to address the last two compile issues (one of which also appears to be a major bug, but only a warning at compile time).

Also, Victor has added os.get_blocking() and os.set_blocking(), which prevent your patch from applying cleanly (I tested against the parent of Victor's changeset).

After fixing the two issues I pointed out on Rietveld, I still get major failure on test:

======================================================================
ERROR: test_1565150 (main.StatAttributeTests)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "P:\ath\to\cpython\lib\test\test_os.py", line 214, in tearDown
    os.rmdir(support.TESTFN)
OSError: [WinError 145] The directory is not empty: '@test_13492_tmp'

======================================================================
ERROR: test_1686475, test_file_attributes, test_large_time, test_stat_attributes, test_stat_attributes_bytes, test_stat_result_pickle, test_utime, test_utime_dir, test_utime_invalid_arguments, test_utime_ns, test_utime_subsecond, test_exist_ok_existing_directory, test_exist_ok_existing_regular_file, test_exist_ok_s_isgid_directory, test_makedir (All the same error)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "P:\ath\to\cpython\lib\test\test_os.py", line ###, in setUp
    os.mkdir(support.TESTFN)
FileExistsError: [WinError 183] Cannot create a file when that file already exists: '@test_13492_tmp'

======================================================================
ERROR: test_urandom_fd_reopened (main.URandomTests)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "P:\ath\to\cpython\lib\test\test_os.py", line 1138, in test_urandom_fd_reopened
    with open(support.TESTFN, 'wb') as f:
PermissionError: [Errno 13] Permission denied: '@test_13492_tmp'

======================================================================
FAIL: test_chdir (main.Win32ErrorTests)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "P:\ath\to\cpython\lib\test\test_os.py", line 1274, in test_chdir
    self.assertRaises(OSError, os.chdir, support.TESTFN)
AssertionError: OSError not raised by chdir

---

Ran 164 tests in 5.122s

The problem appears to be in unlink or rmdir, but I can't see anything amiss in either one. I'll keep looking, but you may have a better idea what's going wrong.

[larryhastings]

Two small fixes from Zach (thanks again Zach!) and I updated against current trunk so it should apply cleanly. How's it look now?

[zware]

The patch applies and compiles cleanly, and I finally tracked down what was causing the errors I reported yesterday: os_utime_impl was changed to use Py_RETURN_NONE instead of just setting return_value = Py_None, so Windows skipped the exit routine and left an open handle on any call to os.utime. Commented on the bad line on Rietveld.

[zware]

Another nit to pick: long lines in docstrings. There are several lines about 75-78 characters long in several different docstrings, which look absolutely terrible when you try "import os;help(os)" on an 80-character-wide terminal due to an 8 character indent. Blame can be spread pretty far and wide on this, but I wonder if Clinic should enforce a 72 character limit on docstring lines to try to mitigate this?

Other than that (and the fix to utime mentioned earlier), I don't see anything obviously wrong with the patch, though I admit to not having read through the whole thing (it's huge!).

[larryhastings]

Diff tweaked to undo the ill-concieved Py_RETURN_NONE change. Thanks, Zachary! Does it now compile and pass tests on Windows?

[zware]

Yep, Windows is happy with the latest patch.

Since this is such an enormous patch, I'm assuming it's only going into 3.5 and have changed the version field accordingly.

[larryhastings]

Yeah, I've been meaning to mark all the Derby patches as 3.5. We're not adding new Clinic conversions to 3.4.

[python-dev]

New changeset 0c57aba6b1a3 by Larry Hastings in branch 'default':
Issue bpo-20170: Convert posixmodule to use Argument Clinic.
http://hg.python.org/cpython/rev/0c57aba6b1a3

[larryhastings]

Gonna keep an eye on the buildbots and make sure I haven't caused any new breakage. Otherwise... fingers crossed, I think it's done! Thanks for the help everybody (particularly Zach!).