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.

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

gh-112026: Add again <unistd.h> include in Python.h #112046

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 13, 2023

Add again <ctype.h> and <unistd.h> includes in Python.h, but don't include them in the limited C API version 3.13 and newer.


📚 Documentation preview 📚: https://cpython-previews--112046.org.readthedocs.build/

Add again <ctype.h> and <unistd.h> includes in Python.h, but don't
include them in the limited C API version 3.13 and newer.
@vstinner
Copy link
Member Author

The removal of <ctype.h> and <unistd.h> header files impacted many projects: https://discuss.python.org/t/ongoing-packages-rebuild-with-python-3-13-in-fedora/38134

  • 17 fail with: missing <unistd.h> declaration: onboard, dbus-python, opae, borgbackup, python-schedutils, mercurial, python-maxminddb, rdiff-backup, python-zstandard, python-pywayland, python-kerberos, pyicu, python-apt, py-spidev, python-netifaces, i2c-tools, python-yara

  • 8 fail with: missing <ctype.h> declaration: pyodbc, python-cx-oracle, proxmark3, python-gammu, python-igraph, sudo, python-cffi, python-bitstruct

@vstinner
Copy link
Member Author

@vstinner
Copy link
Member Author

The commit 03c4080 also removed a workaround on macOS (_PY_PORT_CTYPE_UTF8_ISSUE). This change does not restore it.

@pitrou
Copy link
Member

pitrou commented Nov 14, 2023

-1 on this. It is not Python's job to include system headers that are not used by Python's own header files. Let third-party authors fix their own code.

@vstinner
Copy link
Member Author

@gpshead wrote:

I'd much prefer 'revert' for any API anyone is found using in 3.13.

We need to treat 3.13 as a more special than usual release and aim to minimize compatibility headaches for existing project code. That way more things that build and run on 3.12 build can run on 3.13 as is or with minimal work.

This will enable ecosystem code owners to focus on the bigger picture task of enabling existing code to be built and tested on an experimental pep703 free-threading build rather than having a pile of unrelated cleanup trivia blocking that.

@gvanrossum wrote:

(Restoring removed APIs at users’ requests is fine.)

@zooba wrote:

I’d rather not make the Python headers be responsible for including other system headers, and there isn’t really any way to warn people that they’re relying on it. Eventually they’ll be removed in another alpha 1 to provide the maximum time to catch up.

In issue gh-108765, I modified Python.h to removed multiple "standard" header files from Python.h:

  • <ctype.h>
  • <ieeefp.h>
  • <stddef.h> (only on Windows)
  • <sys/time.h>
  • <unistd.h>

These changes are documented in What's New in Python 3.13 and I know that multiple projects have already been updated to include these header explicitly.

My colleague Karolina didn't ask for revert, she only listed projects impacted by Python 3.13 incompatible C API changes.

Since I did these changes, obviously, I'm supportive of these changes, but I'm questioning if Python 3.13 is the right time to make these changes. We can make them again in Python 3.14. Also, I'm thinking about a new way to introduce incompatible changes which are not "mandatory" to have a smoother migration plan than "affect everybody at once".

@alex
Copy link
Member

alex commented Nov 14, 2023

Perhaps there's a middle ground:

  • File bugs with the known impacted projects, letting them know it will break in the future
  • Revert the change for now
  • Re-land for 3.14

This means we don't have to maintain this forever, but also lets projects fix themselves on a longer timeline, and avoids potentially disrupting all the other large work in 3.13.

@vstinner
Copy link
Member Author

Revert the change for now. Re-land for 3.14

We did that multiple times in the past and it went well:

  • open() "U" flag
  • collections ABC aliases
  • remove asyncore, asynchat, stmpd

For these examples, the change was done again in the following Python version

@zooba
Copy link
Member

zooba commented Nov 14, 2023

Yeah, I'm okay with using alpha releases as "scream tests" (as we call them at work) - make the change, find out who screams, revert the change and let those people know how long they've got until it becomes permanent.

Add a comment saying that these should be removed in 3.14.0a1 and I'm fine.

@thesamesam
Copy link
Contributor

thesamesam commented Nov 15, 2023

I can't speak on the #112046 (comment) bit because that's a CPython decision, but I will say from the distribution perspective, we're very used to having various transitive include issues with new compiler versions, new libc versions, and new library versions. This wouldn't be that different for us and is fine.

Given this was introduced pretty early in the cycle, I'm a little surprised at it bothering anybody.

@eli-schwartz
Copy link
Contributor

Seconded. The reason why downstream projects might feel a compelling desire to have CPython APIs reverted is because use of private APIs is standardly done for reasons of "I feel I have no choice" and people are correct for using private APIs if they ended up using them.

Sure, use of private APIs means that you're accepting the responsibility of depending on something that isn't guaranteed to exist in the future, and that might mean a hard time if CPython decides to remove them for whatever reason, but at the time you started using that private API, it wasn't a bug that you decided to use the private API.

Failing to include a required header because your other includes transitively included that header, is a bug. You wrote incorrect code, it was always wrong, you just didn't happen to notice. Every dependency you possess might be hiding transitive includes. Even the compiler regularly documents such issues in "porting to XXX". The solution is adding the header that you always needed. There are tools to automate this, such as include-what-you-use. Either way, adding that header is semantically correct for all old versions of python as well.

Fixing use of private API may often mean ifdef'ing code for different versions of the CPython API, or dropping support for some versions of CPython, or not having any good solution at all and becoming much slower, and that's the main reason people might want a grace period for private API.

I'd be interested to hear if there's any project with missing unistd.h includes that, faced with a bug report on python 3.13, doesn't just... add the include in a heartbeat. Unless the project is entirely dead upstream, of course. :)

@encukou
Copy link
Member

encukou commented Nov 15, 2023

It is not Python's job to include system headers that are not used by Python's own header files. Let third-party authors fix their own code.

I agree with that in general.
However, as @gpshead said: for Python 3.13 specifically, let's keep trivial things like this as backwards-compatible as possible, so that users can test as early and easily as possible, and focus on dealing with the real issues.

This is a trivial issue, it affects surprisingly many projects -> let's revert and then discuss.

@vstinner
Copy link
Member Author

However, as @gpshead said: for Python 3.13 specifically, let's keep trivial things like this as backwards-compatible as possible, so that users can test as early and easily as possible, and focus on dealing with the real issues.

I think that there are two valid use cases:

  • Fix <Python.h>: avoid including standard/system header files such as <ctype.h>
  • Smooth Python upgrade: decide when to handle "tidy" changes such as removed <ctype.h>

Currently, we only have a single big hammer button: change the main branch and affect everybody right now.

IMO we need a different approach to satisfy both groups: let developers eager to make their code "future proof" and fix "tidy" issues right now, and give time to other busy maintainers who are already dealing with the long list of other Python 3.13 incompatible changes.

Such solution is being discussed there:

This is a trivial issue, it affects surprisingly many projects -> let's revert and then discuss

The Phase 2 of my "Clarify private vs public functions in Python 3.13" plan required to make "most popular projects like Cython, numpy and pip" compatible in Python 3.13 alpha 1 release. Well, this target release/date was missed, but we can now target the next alpha2 scheduled next Tuesday.

I'm actively working on fixing numpy since it blocks my collleagues who are upgrading Python from 3.12 to 3.13 in Fedora 40: https://discuss.python.org/t/ongoing-packages-rebuild-with-python-3-13-in-fedora/38134

Until numpy is fixed, they are hundreds of Python packages which cannot be tested, and so we may miss many bugs. I would prefer to get bug reports as soon as possible to have more time to fix them.

Previously, it was discussed to introduce such "tidy" changes in alpha1 version. Once affected projects are aware, revert the change to give more time to maintainers to update their code. Well, I think that it is exactly what was done in Python 3.13 alpha1.

I was asked by my colleague Miro to batch all incompatible changes in alpha1 but then don't make further changes before Python 3.14. I did exactly that :-) That's why I removed all deprecated functions scheduled for removal in Python 3.13 at once. So now we can consider to restore them if needed.

@vstinner vstinner merged commit b338ffa into python:main Nov 15, 2023
30 checks passed
@vstinner vstinner deleted the restore_include_unistd branch November 15, 2023 15:59
@vstinner
Copy link
Member Author

If people are interested to "tidy" Python C API, I suggest discussing solutions which would have a smoother migration path in capi-workgroup/api-evolution#24

Thanks for the interesting discussion and feedback.

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
)

Add again <ctype.h> and <unistd.h> includes in Python.h, but don't
include them in the limited C API version 3.13 and newer.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
)

Add again <ctype.h> and <unistd.h> includes in Python.h, but don't
include them in the limited C API version 3.13 and newer.
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