Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-123797: Check for runtime availability of ptsname_r on macos #123806

Merged
merged 4 commits into from
Sep 20, 2024
Merged

gh-123797: Check for runtime availability of ptsname_r on macos #123806

merged 4 commits into from
Sep 20, 2024

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Sep 7, 2024 •
edited by bedevere-app bot
Loading

@Eclips4 Eclips4 added the needs backport to 3.13 bugs and security fixes label Sep 7, 2024
vstinner
vstinner reviewed Sep 9, 2024
View reviewed changes
Modules/posixmodule.c Outdated Show resolved Hide resolved
ronaldoussoren
ronaldoussoren requested changes Sep 9, 2024
View reviewed changes
Modules/posixmodule.c Outdated
Comment on lines 8684 to 8687
#else
// unknown error:
// both ptsname_r and ptsname are not available for some reason.
ret = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a "cannot happen" case: ptsname is available on all versions of macOS where configure detects ptsname_r.

I'd either drop drop the guard for the block above or replace this bit by an #error.

(And regardless ret = -1 is wrong, the value should be an errno value such as ENOSYS).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks for the review! 👍

Copy link
Member

Choose a reason for hiding this comment

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

This is a "cannot happen" case: ptsname is available on all versions of macOS where configure detects ptsname_r.

Is there a macOS or iOS platform where ptsname() and ptsname_r() are not available?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a macOS or iOS platform where ptsname() and ptsname_r() are not available?
In short, no.

ptsname_r() is available on macOS 10.13.4 and iOS 11.3 (and later), ptsname() has been available from the start. Or at least, there are no availability annotations in Apple's headers which is a good indication that the API was available from the start.

@bedevere-app
Copy link

bedevere-app bot commented Sep 9, 2024

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

vstinner
vstinner reviewed Sep 10, 2024
View reviewed changes
Modules/posixmodule.c Outdated Show resolved Hide resolved
Modules/posixmodule.c Outdated Show resolved Hide resolved
Modules/posixmodule.c Outdated Show resolved Hide resolved
@sobolevn
Copy link
Member Author

sobolevn commented Sep 10, 2024 •
edited
Loading

Done, thanks a lot, @vstinner, for the helpful suggestions! 👍

vstinner
vstinner approved these changes Sep 10, 2024
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@sobolevn
Copy link
Member Author

I will wait for @ronaldoussoren's feedback for a ~week and then merge 👍

@sobolevn
Copy link
Member Author

One week has passed (why the time is so fast?), so I am merging this.
Thanks to @vstinner and @ronaldoussoren for the reviews!

@sobolevn sobolevn merged commit 3e36e5a into python:main Sep 20, 2024
38 of 39 checks passed
@miss-islington-app
Copy link

Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 20, 2024
@sobolevn @miss-islington
pythongh-123797: Check for runtime availability of ptsname_r on mac…
1bdedd4 
…os (pythonGH-123806)

(cherry picked from commit 3e36e5a)

Co-authored-by: sobolevn <mail@sobolevn.me>
@bedevere-app
Copy link

bedevere-app bot commented Sep 20, 2024

GH-124270 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Sep 20, 2024
@bedevere-app bedevere-app bot mentioned this pull request Sep 20, 2024
Closed
savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this pull request Sep 22, 2024
@sobolevn @savannahostrowski
pythongh-123797: Check for runtime availability of ptsname_r on mac…
c119ced 
…os (python#123806)
savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this pull request Sep 22, 2024
@sobolevn @savannahostrowski
pythongh-123797: Check for runtime availability of ptsname_r on mac…
dbf38b3 
…os (python#123806)
Yhg1s pushed a commit that referenced this pull request Sep 30, 2024
@miss-islington @sobolevn
[3.13] gh-123797: Check for runtime availability of ptsname_r on ma…
dbfc37a 
…cos (GH-123806) (#124270)

gh-123797: Check for runtime availability of `ptsname_r` on macos (GH-123806)
(cherry picked from commit 3e36e5a)

Co-authored-by: sobolevn <mail@sobolevn.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronaldoussoren ronaldoussoren ronaldoussoren requested changes

@vstinner vstinner vstinner approved these changes

@ned-deily ned-deily Awaiting requested review from ned-deily

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sobolevn @vstinner @ronaldoussoren @Eclips4