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

bpo-44220: Export PyStructSequence_UnnamedField in C API #26331

Merged
merged 7 commits into from
Oct 21, 2021
Merged

bpo-44220: Export PyStructSequence_UnnamedField in C API #26331

merged 7 commits into from
Oct 21, 2021

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented May 24, 2021 •
edited by bedevere-bot
Loading

The field is already present in the documentation so I think it's part of the C API:
https://docs.python.org/3/c-api/tuple.html#c.PyStructSequence_UnnamedField

https://bugs.python.org/issue44220

@Fidget-Spinner Fidget-Spinner requested a review from a team as a code owner May 24, 2021 14:16
@Fidget-Spinner Fidget-Spinner requested a review from encukou May 24, 2021 14:16
@Fidget-Spinner Fidget-Spinner added the needs backport to 3.10 only security fixes label May 24, 2021
@shreyanavigyan
Copy link
Contributor

I don't know but shouldn't it be "added 3.10" instead of "added 3.11 # and also 3.10"?

@Fidget-Spinner
Copy link
Member Author

I don't know but shouldn't it be "added 3.10" instead of "added 3.11 # and also 3.10"?

I'm not sure. Currently it's getting merged into the 3.11/main branch then backported to 3.10. So I thought 3.11 would be appropiate.

@shreyanavigyan
Copy link
Contributor

shreyanavigyan commented May 25, 2021 •
edited
Loading

I'm not sure. Currently it's getting merged into the 3.11/main branch then backported to 3.10. So I thought 3.11 would be appropiate.

But when it's merged, it would seem the opposite. Backporting is for our convenience. But others will see it as 3.10 -> 3.11 not 3.11 -> 3.10.

@Fidget-Spinner
Copy link
Member Author

I'm not sure. Currently it's getting merged into the 3.11/main branch then backported to 3.10. So I thought 3.11 would be appropiate.

But when it's merged, it would seem the opposite. Backporting is for our convenience. But others will see it as 3.10 -> 3.11 not 3.11 -> 3.10.

I'm no expert on the C API, but isn't this file an internal thing not exposed to users? I thought it's just used for tracking changes and validating exported symbols for ourselves?

There are other examples in this file of adding something in a point release and not calling it "added since that". Eg. some things were added in 3.7 and 3.6.1, but 3.7 is still used instead of saying 3.6.1 onwards (which is what is normally said in user facing whatsnew/news entry).

I don't know enough to comment further. So I'll see what Petr/another core dev says and follow that. Thanks for the concern!

@encukou
Copy link
Member

encukou commented May 25, 2021

PyStructSequence_UnnamedField is already part of the C API. This commit is adding it to the limited API, which is a subset of the full C API.

If the PR is backported, the docs should say it was added in 3.10.

Adding something to the Stable ABI in a point release, like 3.6.1, doesn't make much sense, and it should not happen from now on. The Stable ABI doesn't really have point releases.
For the historical cases, something added in Python 3.6.1 is documented as added in Stable ABI 3.7.

@Fidget-Spinner
Copy link
Member Author

PyStructSequence_UnnamedField is already part of the C API. This commit is adding it to the limited API, which is a subset of the full C API.

If the PR is backported, the docs should say it was added in 3.10.

Adding something to the Stable ABI in a point release, like 3.6.1, doesn't make much sense, and it should not happen from now on. The Stable ABI doesn't really have point releases.
For the historical cases, something added in Python 3.6.1 is documented as added in Stable ABI 3.7.

Thanks for the clarification Petr! I have a few questions:

  1. Should this variable be in the limited API or public C API? I'm having second thoughts about this now.
  2. Does this mean we can't change the value of the constant once its added to the limited API (or just the type)? The devguide isn't too clear about this.

Thanks in advance.

@encukou
Copy link
Member

encukou commented May 25, 2021

  1. I'd say Limited API. It's not possible to mark unnamed fields using the existing Limited API, right?
  2. It's possible to change the value.
    If it was a #define, it wouldn't be possible to change it, since the value would be compiled into people's extensions.
    Since it's a PyAPI_DATA, extensions will retrieve the value by name.
    It won't be possible to change the type.

@Fidget-Spinner
Copy link
Member Author

  1. I'd say Limited API. It's not possible to mark unnamed fields using the existing Limited API, right?
  2. It's possible to change the value.
    If it was a #define, it wouldn't be possible to change it, since the value would be compiled into people's extensions.
    Since it's a PyAPI_DATA, extensions will retrieve the value by name.
    It won't be possible to change the type.
  1. Yes.
  2. Thanks so much for the clarifications. This cleared up a lot of my confusion.

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jun 25, 2021
@Fidget-Spinner Fidget-Spinner removed stale Stale PR or inactive for long period of time. needs backport to 3.10 only security fixes labels Oct 20, 2021
@vstinner
Copy link
Member

IMO it's now too late to backport it to 3.10 which is now released. I suggest to only change the limited C API in Python 3.11.

vstinner
vstinner approved these changes Oct 20, 2021
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.

@Fidget-Spinner
Copy link
Member Author

IMO it's now too late to backport it to 3.10 which is now released. I suggest to only change the limited C API in Python 3.11.

Agreed. I removed the needs backport to 3.10 label for that reason, we've missed that train.

@encukou encukou merged commit 2cbf50e into python:main Oct 21, 2021
@encukou
Copy link
Member

encukou commented Oct 21, 2021

Thank you!
And thank you for the reminder yesterday – this issue slipped off my TODO list since May :(

shihai1991 added a commit to shihai1991/cpython that referenced this pull request Oct 21, 2021
@shihai1991
Merge branch 'main' into bpo_45113_3
9d162ef 
* main: (263 commits)
  bpo-45521: Fix a bug in the obmalloc radix tree code. (pythonGH-29051)
  bpo-45522: Allow to disable freelists on build time (pythonGH-29056)
  bpo-34451: Document prompt and output toggle feature in html tutorial (pythonGH-27105)
  bpo-44019: Add operator.call() to __all__ for the operator module (pythonGH-29110)
  bpo-45315: PyType_FromSpec: Copy spec->name and have the type own the memory for its name (pythonGH-29103)
  bpo-44220: Export PyStructSequence_UnnamedField in the limited API (pythonGH-26331)
  bpo-44174: [Enum] add reference to name mangling (pythonGH-29116)
  bpo-45548: add some missing entries to `Modules/Setup` (pythonGH-29115)
  bpo-35673: Add a public alias for namespace package __loader__ attribute (python#29049)
  bpo-45192: Fix a bug that infers the type of an os.PathLike[bytes] object as str (pythonGH-28323)
  bpo-45527: Don't count cache hits, just misses. (pythonGH-29092)
  bpo-45320: Remove long-deprecated inspect methods (pythonGH-28618)
  bpo-41374: Remove obsolete exclusion of netinet/tcp.h on Cygwin (pythonGH-21649)
  bpo-45532: Replace 'default' with 'main' as default in sys.version (pythonGH-29100)
  bpo-45464: [doc] Explain that subclassing multiple exceptions is fragile (pythonGH-29094)
  Cleanup a couple of comments left on PR 28775 post-merge. (pythonGH-29079)
  bpo-45536: Check OpenSSL APIs in configure (pythonGH-29088)
  Add PEPs 593 & 647 to list of PEPs at top of typing docs (pythonGH-29097)
  Add a comment about how to fix bogus test_host_resolution_bad_address failures (python#29085)
  bpo-44525: Specialize simple Python calls. (pythonGH-29033)
  ...
@moreati moreati mentioned this pull request Aug 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@encukou encukou encukou left review comments

@vstinner vstinner vstinner approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@Fidget-Spinner @shreyanavigyan @encukou @vstinner @the-knights-who-say-ni @bedevere-bot