-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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-118201: Simplify conv_confname #126089
Conversation
!buildbot android |
!buildbot ios |
Well I'm not sure why it ran the buildbots on all platforms when I only asked for two, but I've checked the failures, and all of them are already present on the main branch. |
@sobolevn: You've done some work on the |
Nope, sorry, I am not faimiliar with that code :( |
I'm not sure if any active developer is familiar with this code, but I hope the change should be fairly self-explanatory. @JelleZijlstra: are you able to take a look, or can you recommend anyone else? |
I can take a look but I'm also not familiar so I'll need a bit of time to familiarize myself with it. Have you tried looking at the |
It looks like this code has remained almost unchanged since it was written in 1999 by @freddrake. |
Wow, that seems like a good reason to be extra careful with any changes. |
!buildbot android |
!buildbot android|ios |
@mhsmith thanks, this looks good! I assume the buildbot failure are the result of the unrelated GC change that came in the other day? |
Yes, and it looks like that's been fixed on the main branch now, so I'll try another run. Let's not backport this PR to 3.13, as it's a relatively disruptive change, and a backport isn't really necessary:
|
!buildbot android|ios |
OK, looks like this is ready to merge. |
On Android and iOS we've been seeing intermittent buildbot failures where
os.confstr
,os.pathconf
oros.sysconf
raise a ValueError. This is not a failure in the underlying system call, but a failure to look up a mapping from strings to integers before making the system call.I haven't been able to explain this (see #118201 for details), but I have found a way to simplify the relevant code, which will hopefully fix the issue, or at least give us some more information about it.
All three functions depend on
conv_confname
, which currently maps from strings to integers using a binary search in a C array. This PR changes it to useos.{confstr,pathconf,sysconf}_names
, which are dicts containing the same information.I also took the opportunity to improve the test coverage of this area.
os.sysconf
on iOS #118453