Skip to content

bpo-35134: Add Include/cpython/pytime.h file #23988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 16, 2021
Merged

Conversation

nw0
Copy link
Contributor

@nw0 nw0 commented Dec 29, 2020

Add Include/cpython/pytime.h header file.

Move CPython C API from Include/pytime.h into a new
Include/cpython/pytime.h header file, which is included by
Include/pytime.h.

https://bugs.python.org/issue35134

In the same vein as previous PRs by @vstinner. I've never done this before, so apologies in advance if I've messed anything up.

https://bugs.python.org/issue35134

Add Include/cpython/pytime.h header file.

Move CPython C API from Include/pytime.h into a new
Include/cpython/pytime.h header file, which is included by
Include/pytime.h.
@nw0
Copy link
Contributor Author

nw0 commented Dec 29, 2020

Hmm, I'd like to add the skip-news label like previous PRs for this issue, but can't seem to do this. Help?

@vstinner
Copy link
Member

Hum, since the new Include/pytime.h is basically empty. I suggest to leave the file unchanged and the move it into Include/cpython/.

I don't expect anyone to include it directly, and it's already included by "Python.h". The C API documentation asks to only include "Python.h."

Address review; replaced by Include/cpython/pytime.h
@nw0 nw0 requested a review from pablogsal as a code owner February 13, 2021 08:19
@@ -31,7 +31,7 @@
#include "pycore_pyerrors.h"
#include "pycore_pystate.h" // _PyThreadState_GET()
#include "pydtrace.h"
#include "pytime.h" // _PyTime_GetMonotonicClock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum. Since it's included by Python.h, including it explicitly is not needed, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated these, and moved the pytime.h include further down in Python.h to allow for it. I believe there aren't any other files including pytime.h directly now.

@@ -2,8 +2,7 @@
#ifndef Py_PYTIME_H
#define Py_PYTIME_H

#include "pyconfig.h" /* include for defines */
#include "object.h"
#include "../object.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that people should include pytime.h directly, and so I suggest to simply remove these includes. Let people respect the Python C API documentation which suggests to only include "Python.h".

@vstinner vstinner merged commit 17dbd40 into python:master Feb 16, 2021
@vstinner
Copy link
Member

Thank you, I merged your PR.

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE RHEL8 LTO 3.x has failed when building commit 17dbd40.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/361/builds/1230) and take a look at the build logs.
  4. Check if the failure is related to this commit (17dbd40) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/361/builds/1230

Summary of the results of the build (if available):

== Tests result: ENV CHANGED ==

412 tests OK.

10 slowest tests:

  • test_peg_generator: 4 min 37 sec
  • test_concurrent_futures: 2 min 46 sec
  • test_gdb: 2 min 38 sec
  • test_multiprocessing_spawn: 1 min 36 sec
  • test_tokenize: 1 min 12 sec
  • test_unparse: 1 min 6 sec
  • test_multiprocessing_forkserver: 1 min 4 sec
  • test_asyncio: 1 min 3 sec
  • test_lib2to3: 1 min 3 sec
  • test_multiprocessing_fork: 54.1 sec

1 test altered the execution environment:
test_asyncio

13 tests skipped:
test_devpoll test_ioctl test_kqueue test_msilib test_ossaudiodev
test_startfile test_tix test_tk test_ttk_guionly test_winconsoleio
test_winreg test_winsound test_zipfile64

Total duration: 5 min 42 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/asyncio/sslproto.py", line 321, in __del__
    self.close()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/asyncio/sslproto.py", line 316, in close
    self._ssl_protocol._start_shutdown()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/asyncio/sslproto.py", line 590, in _start_shutdown
    self._abort()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/asyncio/sslproto.py", line 731, in _abort
    self._transport.abort()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/asyncio/selector_events.py", line 680, in abort
    self._force_close(None)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/asyncio/selector_events.py", line 731, in _force_close
    self._loop.call_soon(self._call_connection_lost, exc)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/asyncio/base_events.py", line 745, in call_soon
    self._check_closed()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/asyncio/base_events.py", line 510, in _check_closed
    raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
This change is backward compatible since C extension modules
must not include "pytime.h" directly, but only include "Python.h".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants