Skip to content

gh-81925: Implement the native thread ids for KFreeBSD #111761

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 3 commits into from
Nov 9, 2023

Conversation

sthibaul
Copy link
Contributor

@sthibaul sthibaul commented Nov 5, 2023

@pitrou pitrou requested a review from gpshead November 9, 2023 12:12
@sthibaul sthibaul force-pushed the native_id branch 2 times, most recently from 39c4998 to cea0d45 Compare November 9, 2023 14:13
@sthibaul sthibaul changed the title gh-81925: Implement the native thread ids for the Hurd and KFreeBSD gh-81925: Implement the native thread ids for KFreeBSD Nov 9, 2023
@pitrou
Copy link
Member

pitrou commented Nov 9, 2023

Thanks for the update. Can you also update the docs here and here? The updated markup could be something like:

   .. availability:: Windows, FreeBSD, Linux, macOS, OpenBSD, NetBSD, AIX, DragonFlyBSD, GNU/kFreeBSD.

   .. versionadded:: 3.8

   .. versionchanged:: 3.13
      Added support for GNU/kFreeBSD.

@sthibaul
Copy link
Contributor Author

sthibaul commented Nov 9, 2023

Thanks for the update. Can you also update the docs here and here? The updated markup could be something like:

Done so!

@@ -127,8 +127,8 @@ class Availability(SphinxDirective):
# known platform, libc, and threading implementations
known_platforms = frozenset({
"AIX", "Android", "BSD", "DragonFlyBSD", "Emscripten", "FreeBSD",
"Linux", "NetBSD", "OpenBSD", "POSIX", "Solaris", "Unix", "VxWorks",
"WASI", "Windows", "macOS",
"GNU/kFreeBSD", "Linux", "NetBSD", "OpenBSD", "POSIX", "Solaris",
Copy link
Member

@pitrou pitrou Nov 9, 2023

Choose a reason for hiding this comment

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

@encukou Does this seem ok to you?

@pitrou pitrou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 9, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pitrou for commit b74f76e 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 9, 2023
@encukou
Copy link
Member

encukou commented Nov 9, 2023

The code change looks OK to me, but I would not change the docs -- the mentions suggest that we'll keep GNU/kFreeBSD support working, which we currently can't promise as we (AFAIK) don't have a stable buildbot for it.

@encukou
Copy link
Member

encukou commented Nov 9, 2023

(Sorry, @sthibaul, for maintainers disagreeing with each other with you caught in the cross-fire. Don't change stuff until we agree what to do.)

@pitrou
Copy link
Member

pitrou commented Nov 9, 2023

The code change looks OK to me, but I would not change the docs -- the mentions suggest that we'll keep GNU/kFreeBSD support working, which we currently can't promise as we (AFAIK) don't have a stable buildbot for it.

We're not promising anything even on other platforms, AFAICT. The tests for native_id / get_native_id don't check for specific platforms, so the feature could quietly be disabled on supposedly-supported platforms.

@pitrou
Copy link
Member

pitrou commented Nov 9, 2023

The buildbot failures are unrelated, so I'll merge.

@pitrou pitrou merged commit 0802fd6 into python:main Nov 9, 2023
@malemburg
Copy link
Member

Just saw this on the checkins ML and curious: why are we adding support for an officially terminated variant of Debian ?

@pitrou
Copy link
Member

pitrou commented Nov 10, 2023

Oh, I was not aware of that. Given that support here requires minimal code and doesn't affect the rest of the (non-kFreeBSD-specific) code, I thought the PR was fine.

@sthibaul Is kFreeBSD actually unmaintained, or is it just maintained outside of Debian?

Also cc @gpshead for opinions.

@sthibaul
Copy link
Contributor Author

GNU/kFreeBSD is not only about Debian. Python already has various places where the GNU/kFreeBSD __FreeBSD_kernel__ macro appears, so this is not a new thing for Python, just a missing bit here.
The support of GNU/kFreeBSD was indeed recently dropped in the Debian distribution, I haven't checked if it exists in other distributions, Arch for instance, probably. It doesn't hurt much to keep the macros in place (I have to say that python is very portable with very few such #ifdefs)

@malemburg
Copy link
Member

So this is more about supporting FreeBSD, than specifically GNU/kFreeBSD ? That would make sense.

@sthibaul
Copy link
Contributor Author

Yes, it's about supporting the FreeBSD kernel without the usual BSD userland. It's not actually specific to being GNU/kFreeBSD, it could be whatever/kFreeBSD

@gpshead
Copy link
Member

gpshead commented Nov 10, 2023

The key part of this PR is just adding || defined(__FreeBSD_kernel__) to the C preprocessor checks. That's fine even if nobody can build or maintain anything on such an esoteric system.

I think the docs mentioning the never going to be used by any non-hobbyist GNU/kFreeBSD is the wrong terminology: just leave that as a more generic "FreeBSD kernel" statement (not even expecting anyone to understand the kFreeBSD term). If I were authoring this PR I'd have just updated "FreeBSD" in the existing list to "FreeBSD kernel" in the docs as that itself implies actual FreeBSD as far as most any reader is concerned.

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@hugovk
Copy link
Member

hugovk commented Mar 15, 2024

Triage: I've closed the issue for this PR. Please re-open or create a new issue if needed.

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants