gh-92869: ctypes: Add c_time_t #92870

thp · 2022-05-17T06:25:12Z

This implements a simple way of using ctypes.c_time_t for 32-bit and 64-bit time_t sizes. See #92869 for a discussion on why this is useful and considerations for different platforms. This doesn't take into account platforms where time_t is defined as double or anything else that's not a signed integer.

cpython-cla-bot · 2022-05-17T06:25:14Z

All commit authors signed the Contributor License Agreement.

thp · 2022-06-10T07:57:18Z

Bump after 24 days of no activity. Any takers for reviewing this PR?

m-tmatma · 2022-06-10T08:18:11Z

@thp
It seems azure pipeline test was failed.
You should re-trigger it.

m-tmatma · 2022-06-10T08:23:56Z

Lib/ctypes/__init__.py

+    c_time_t = c_int64
+elif SIZEOF_TIME_T == 4:
+    c_time_t = c_int32
+


@thp
Is it ok not to define else case?

If time_t is not 32-bit or 64-bit, ctypes.c_time_t will not be defined. I haven't seen a system where it's not 32-bit or 64-bit, but if we encounter such a system, the right type mapping can be added then.

@thp
I think you should add to raise an exception on else case to make sure.
It may not happen, but it is a good practice to consider such exceptional case.

PR updated, now raises an exception.

It seems I had approved this PR yesterday before the PR was updated,
But it was a mistake. I made it on a smart phone.

It seem I can't dismiss the approval for this repository.

thp · 2022-06-10T13:00:57Z

@thp
It seems azure pipeline test was failed.
You should re-trigger it.

Is there a way to manually re-trigger it? I don't seem to have permissions. I can of course amend the commits and force-push to indirectly re-trigger a build?

arhadthedev · 2022-06-10T13:18:29Z

Is there a way to manually re-trigger it

You can close and reopen the PR.

m-tmatma · 2022-06-10T13:26:25Z

@thp

Is there a way to manually re-trigger it? I don't seem to have permissions. I can of course amend the commits and force-push to indirectly re-trigger a build?

There is another way to re-trigger.

You can comment /AzurePipelines run or /azp run to a PR.
https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/github?view=azure-devops&tabs=yaml#comment-triggers

AA-Turner

Seems reasonable, please add tests though.

A

Doc/library/ctypes.rst

Lib/ctypes/__init__.py

Misc/NEWS.d/next/Library/2022-05-17-06-27-39.gh-issue-92869.t8oBkw.rst

AA-Turner · 2022-06-11T10:17:25Z

Pinging listed names from the experts index for review.

A

…oBkw.rst Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

thp · 2022-06-11T18:46:52Z

Lib/ctypes/test/test_sizes.py

@@ -28,6 +28,9 @@ def test_size_t(self):
    def test_ssize_t(self):
        self.assertEqual(sizeof(c_void_p), sizeof(c_ssize_t))

+    def test_time_t(self):
+        self.assertIn(sizeof(c_time_t), (4, 8))


Simple test case added.

As the reason for c_time_t is mostly to get the size of the object out of it, test_sizes.py is probably the best place to test for this. This test is mostly about checking for the existence of the field, and that it's 4 or 8. If/when platforms are found that have a differently-sized c_time_t, this test can be updated.

In the future, if we find a platform where c_time_t is of a different type (different size or even non-integer type), some other tests could be added.

Why don't you compare sizeof(c_time_t) with SIZEOF_TIME_T?

It seems like you should be able to use an independent version from _testcapi.

A

thp · 2022-06-11T19:40:31Z

Found that the ctypes documentation's "Calling functions" documentation does this (without setting restype):

>>> print(libc.time(None))  
1150640792

This uses the default return type of int, which is wrong on systems that use 64-bit time_t (and just a happy coincidence on systems that use 32-bit time_t and happen to have 32-bit int):

>>> from ctypes import *
>>> libc = CDLL("libc.so.6")
>>> libc.time
<_FuncPtr object at 0x...>
>>> libc.time.restype
<class 'ctypes.c_int'>

Updated the documentation accordingly with setting libc.time.restype to the newly-added c_time_t, even though this introduces the concept of restype and argtypes quite early, but it's probably for the best, as not doing it can result in wrong return types as seen here. Alternatively, another libc function could be used for the "Calling functions" section, and libc.time used as an example for the "Return types" section.

The libc.time.restype == c_int error will be noticeable from 2038 onwards due to 32-bit overflow, at least on systems that have 64-bit time_t and 32-bit int (all modern systems?).

AA-Turner · 2022-06-11T22:48:00Z

Alternatively, another libc function could be used for the "Calling functions" section, and libc.time used as an example for the "Return types" section.

I think this is a better approach. (Thanks for noticing!)

A

bedevere-bot added the awaiting review label May 17, 2022

thp force-pushed the ctypes_c_time_t branch from 97557a5 to 7b9e176 Compare May 17, 2022

ctypes: Add c_time_t (Fixes python#92869)

14034b5

thp force-pushed the ctypes_c_time_t branch 2 times, most recently from 9d077ff to f8a8dd3 Compare May 17, 2022

📜🤖 Added by blurb_it.

984c75f

thp force-pushed the ctypes_c_time_t branch from f8a8dd3 to 984c75f Compare May 17, 2022

AlexWaygood added the expert-ctypes label May 17, 2022

thp mentioned this pull request May 20, 2022

iPod support restored using ctypes and libgpod gpodder/gpodder#1289

Merged

m-tmatma approved these changes Jun 10, 2022

View changes

bedevere-bot added awaiting core review and removed awaiting review labels Jun 10, 2022

thp closed this Jun 10, 2022

thp reopened this Jun 10, 2022

Raise an exception if sizeof(time_t) is not 4 or 8

8a3597f

AA-Turner reviewed Jun 11, 2022

View changes

Doc/library/ctypes.rst Outdated Show resolved Hide resolved

Lib/ctypes/__init__.py Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Library/2022-05-17-06-27-39.gh-issue-92869.t8oBkw.rst Outdated Show resolved Hide resolved

AA-Turner requested review from abalkin, amauryfa and meadori Jun 11, 2022

AA-Turner added the type-feature label Jun 11, 2022

thp and others added 3 commits Jun 11, 2022

Update Misc/NEWS.d/next/Library/2022-05-17-06-27-39.gh-issue-92869.t8…

2297500

…oBkw.rst Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

Update Lib/ctypes/__init__.py

25dada1

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

Update Doc/library/ctypes.rst

3b529d1

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

ctypes: Add simple c_time_t size testcase

7f2bfef

thp reviewed Jun 11, 2022

View changes

Fix ctypes documentation with C time() function return type

f47dccb

Fix typo

0468d0a

gh-92869: ctypes: Add c_time_t #92870

gh-92869: ctypes: Add c_time_t #92870

thp commented May 17, 2022

cpython-cla-bot bot commented May 17, 2022 •

edited

thp commented Jun 10, 2022

m-tmatma commented Jun 10, 2022 •

edited

m-tmatma Jun 10, 2022

thp Jun 10, 2022

m-tmatma Jun 10, 2022

thp Jun 11, 2022

m-tmatma Jun 11, 2022

thp commented Jun 10, 2022

arhadthedev commented Jun 10, 2022

m-tmatma commented Jun 10, 2022

AA-Turner left a comment

AA-Turner commented Jun 11, 2022

thp Jun 11, 2022

m-tmatma Jun 11, 2022

AA-Turner Jun 11, 2022

thp commented Jun 11, 2022

AA-Turner commented Jun 11, 2022

gh-92869: ctypes: Add c_time_t #92870

Are you sure you want to change the base?

gh-92869: ctypes: Add c_time_t #92870

Conversation

thp commented May 17, 2022

cpython-cla-bot bot commented May 17, 2022 • edited

thp commented Jun 10, 2022

m-tmatma commented Jun 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thp commented Jun 10, 2022

arhadthedev commented Jun 10, 2022

m-tmatma commented Jun 10, 2022

AA-Turner left a comment

AA-Turner commented Jun 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thp commented Jun 11, 2022

AA-Turner commented Jun 11, 2022

cpython-cla-bot bot commented May 17, 2022 •

edited

m-tmatma commented Jun 10, 2022 •

edited