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-126789: fix some sysconfig data on late site initializations #126812

Merged
merged 7 commits into from
Nov 17, 2024

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Nov 13, 2024

Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00
Copy link
Member Author

FFY00 commented Nov 13, 2024

@hugovk could you maybe have a look at this PR when you have time, as I am currently the only active maintainer?

Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00
Copy link
Member Author

FFY00 commented Nov 14, 2024

There's an address sanitizer failure, but seems unrelated. Might be worth investigating still.

https://github.com/python/cpython/actions/runs/11827914917/job/32957010967?pr=126812

@FFY00 FFY00 merged commit acbd5c9 into python:main Nov 17, 2024
36 checks passed
@miss-islington-app
Copy link

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

@freakboy3742
Copy link
Contributor

freakboy3742 commented Nov 18, 2024

@FFY00 This PR (and the 3.13 backport) appears to have broken the iOS and Android buildbots. venv tests are also impractical on those platforms due to the lack of subprocesses. I'm working on a fix now.

Update: #126941 is the PR; it also needs a backport to 3.13.

@FFY00
Copy link
Member Author

FFY00 commented Nov 18, 2024

@freakboy3742 sorry about that! I probably should have triggered the buildbot tests before merging.

I hadn't run into issues caused by the unavailability of subprocesses, so I forgot about that. This was an oversight on my part 🫤

@FFY00
Copy link
Member Author

FFY00 commented Nov 18, 2024

We could write the tests to not use subprocesses, but they depend on global state, which makes the test implementation tricky, so I just opted for using virtual environments, which unfortunately required the use of subprocesses. I don't think there is quite enough platform dependence in the tested code to warrant rewriting to be compatible with platforms that don't support subprocesses, but I am happy to hear other opinions on this.

@freakboy3742
Copy link
Contributor

@FFY00 I'm not sure it's worth a major rewrite. Virtual environments aren't really useful in the iOS/Android/Emscripten context, because those platforms are already sandboxed and isolated by the app/webpage bundle that they're delivered in. Its similar to the question of whether you need venv inside a Docker container - except that in this case, there's no ability to call subprocess, which makes using a venv near impractical, rather than just an arguable design choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants