Skip to content

gh-102141: replace use of getpid on Windows with GetCurrentProcessId #102142

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 11 commits into from
Feb 24, 2023

Conversation

maxbachmann
Copy link
Contributor

@maxbachmann maxbachmann commented Feb 22, 2023

@arhadthedev arhadthedev added OS-windows extension-modules C modules in the Modules dir labels Feb 22, 2023
@eryksun
Copy link
Contributor

eryksun commented Feb 22, 2023

My opinion may be in the minority here, but I'd prefer to not define HAVE_GETPID in "PC/pyconfig.h" if we're not actually using getpid().

cpython/PC/pyconfig.h

Lines 509 to 510 in 7c106a4

/* Define if you have getpid. */
#define HAVE_GETPID

I know other cases have defined a POSIX system function as being available on Windows that isn't actually available (e.g. HAVE_GETPPID and getppid()), or never gets used or shouldn't be used. That seems like a quick and dirty approach that may eventually cause a regression. I'd prefer to explicitly check for Windows in the cases where we use the Windows API directly or have our own implementation with a different name. For example, #if defined(HAVE_GETPID) || defined(MS_WINDOWS).

@maxbachmann
Copy link
Contributor Author

One difference with e.g. HAVE_GETPPID is that it is at least a hack that is local to the posixmodule.

@maxbachmann maxbachmann requested a review from a team as a code owner February 22, 2023 17:08
@ghost
Copy link

ghost commented Feb 22, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@zooba
Copy link
Member

zooba commented Feb 22, 2023

My opinion may be in the minority here, but I'd prefer to not define HAVE_GETPID in "PC/pyconfig.h" if we're not actually using getpid().

We can both be in the minority then. I'm sure I've done it in the past, but have definitely come around to it not being a great idea (I'm still tempted to split the Windows definitions into ntmodule.c, which would actually make things neater).

@rhettinger rhettinger removed their request for review February 23, 2023 15:45
@zooba
Copy link
Member

zooba commented Feb 24, 2023

Great, thanks for being responsive! And thanks for the patch

@zooba zooba merged commit 1fa3890 into python:main Feb 24, 2023
@bedevere-bot
Copy link

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

Hi! The buildbot ARM64 macOS 3.x has failed when building commit 1fa3890.

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/725/builds/3804) and take a look at the build logs.
  4. Check if the failure is related to this commit (1fa3890) 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/725/builds/3804

Failed tests:

  • test_asyncio

Failed subtests:

  • test_wait_for_race_condition - test.test_asyncio.test_waitfor.AsyncioWaitForTest.test_wait_for_race_condition

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

== Tests result: FAILURE then FAILURE ==

405 tests OK.

10 slowest tests:

  • test_signal: 4 min 21 sec
  • test_concurrent_futures: 2 min 39 sec
  • test_multiprocessing_spawn: 2 min 4 sec
  • test_ssl: 1 min 45 sec
  • test_multiprocessing_forkserver: 1 min 36 sec
  • test_asyncio: 1 min 28 sec
  • test_logging: 1 min 10 sec
  • test_math: 1 min 9 sec
  • test_capi: 46.9 sec
  • test_io: 38.7 sec

1 test failed:
test_asyncio

25 tests skipped:
test_check_c_globals test_devpoll test_epoll test_gdb test_idle
test_ioctl test_launcher test_msilib test_multiprocessing_fork
test_ossaudiodev test_peg_generator test_perf_profiler test_spwd
test_startfile test_tcl test_tix test_tkinter test_ttk
test_ttk_textonly test_turtle test_winconsoleio test_winreg
test_winsound test_wmi test_zipfile64

1 re-run test:
test_asyncio

Total duration: 9 min

Click to see traceback logs
Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncio/tasks.py", line 478, in wait_for
    return await fut
           ^^^^^^^^^
asyncio.exceptions.CancelledError


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/unittest/async_case.py", line 90, in _callTestMethod
    if self._callMaybeAsync(method) is not None:
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/unittest/async_case.py", line 112, in _callMaybeAsync
    return self._asyncioRunner.run(
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncio/base_events.py", line 664, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_asyncio/test_waitfor.py", line 163, in test_wait_for_race_condition
    res = await task
          ^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncio/tasks.py", line 477, in wait_for
    async with timeouts.timeout(timeout):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncio/timeouts.py", line 98, in __aexit__
    raise TimeoutError
TimeoutError

JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this pull request Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir OS-windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants