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-118201: Simplify conv_confname #126089

Merged
merged 6 commits into from
Nov 19, 2024

Conversation

mhsmith
Copy link
Member

@mhsmith mhsmith commented Oct 28, 2024

On Android and iOS we've been seeing intermittent buildbot failures where os.confstr, os.pathconf or os.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 onconv_confname, which currently maps from strings to integers using a binary search in a C array. This PR changes it to use os.{confstr,pathconf,sysconf}_names, which are dicts containing the same information.

I also took the opportunity to improve the test coverage of this area.

@mhsmith
Copy link
Member Author

mhsmith commented Oct 28, 2024

!buildbot android

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mhsmith for commit 2a3e50a 🤖

The command will test the builders whose names match following regular expression: android

The builders matched are:

  • aarch64 Android PR
  • AMD64 Android PR

@mhsmith
Copy link
Member Author

mhsmith commented Oct 28, 2024

!buildbot ios

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mhsmith for commit 2a3e50a 🤖

The command will test the builders whose names match following regular expression: ios

The builders matched are:

  • iOS ARM64 Simulator PR

@mhsmith
Copy link
Member Author

mhsmith commented Oct 29, 2024

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.

@mhsmith
Copy link
Member Author

mhsmith commented Oct 29, 2024

@sobolevn: You've done some work on the posix module recently; are you able to review this PR?

@sobolevn
Copy link
Member

Nope, sorry, I am not faimiliar with that code :(

@mhsmith
Copy link
Member Author

mhsmith commented Oct 31, 2024

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?

@JelleZijlstra
Copy link
Member

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 git blame for the affected functions to see if any were written by people who are still active?

@JelleZijlstra JelleZijlstra self-requested a review October 31, 2024 22:19
@mhsmith
Copy link
Member Author

mhsmith commented Oct 31, 2024

It looks like this code has remained almost unchanged since it was written in 1999 by @freddrake.

@JelleZijlstra
Copy link
Member

Wow, that seems like a good reason to be extra careful with any changes.

Modules/posixmodule.c Outdated Show resolved Hide resolved
Modules/posixmodule.c Outdated Show resolved Hide resolved
@mhsmith
Copy link
Member Author

mhsmith commented Nov 10, 2024

!buildbot android

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mhsmith for commit a214081 🤖

The command will test the builders whose names match following regular expression: android

The builders matched are:

  • AMD64 Android PR
  • aarch64 Android PR

@mhsmith
Copy link
Member Author

mhsmith commented Nov 19, 2024

!buildbot android|ios

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mhsmith for commit 8a026f1 🤖

The command will test the builders whose names match following regular expression: android|ios

The builders matched are:

  • iOS ARM64 Simulator PR
  • aarch64 Android PR
  • AMD64 Android PR

@JelleZijlstra
Copy link
Member

@mhsmith thanks, this looks good! I assume the buildbot failure are the result of the unrelated GC change that came in the other day?

@mhsmith mhsmith added needs backport to 3.13 bugs and security fixes and removed needs backport to 3.13 bugs and security fixes labels Nov 19, 2024
@mhsmith
Copy link
Member Author

mhsmith commented Nov 19, 2024

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:

  • On iOS, the relevant tests are already skipped.
  • On Android, the tests have never failed on the 3.13 branch.

@mhsmith
Copy link
Member Author

mhsmith commented Nov 19, 2024

!buildbot android|ios

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mhsmith for commit 8a026f1 🤖

The command will test the builders whose names match following regular expression: android|ios

The builders matched are:

  • aarch64 Android PR
  • iOS ARM64 Simulator PR
  • AMD64 Android PR

@mhsmith
Copy link
Member Author

mhsmith commented Nov 19, 2024

OK, looks like this is ready to merge.

@JelleZijlstra JelleZijlstra merged commit c5c9286 into python:main Nov 19, 2024
50 checks passed
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
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.

Intermittent "unrecognized configuration name" failure on iOS and Android
4 participants