cpython
cpython copied to clipboard
python
gh-99138: Isolate _zoneinfo
@pganssle I can break this up in several PRs; let me know what works best for you.
:robot: New build scheduled with the buildbot fleet by @erlend-aasland for commit 573aec1ce40b4c0281c8a2e46c1a09022f7769d8 :robot:
If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
You've beat me to it :) Thanks!
Sorry, I was not aware that you were working on a PR; I should've asked first.
:robot: New build scheduled with the buildbot fleet by @erlend-aasland for commit 6e69e939d14123fab642bbff8bf31bdeda13d4b7 :robot:
If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
@pganssle: are you up for reviewing this? Would you want me to split it up in more manageable pieces?
@kumaraditya303: If I split this up in manageable chunks, would you be up for reviewing it? (Or you can just review it as it is; clinic changes aside, it is not too big of a change.)
:robot: New build scheduled with the buildbot fleet by @erlend-aasland for commit 23cbc43f372de581782d0476409708484132f59c :robot:
If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.
:robot: New build scheduled with the buildbot fleet by @erlend-aasland for commit ae5c5498d4fa4a337b30e78a6f5238535cfff54b :robot:
If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.
If I split this up in manageable chunks, would you be up for reviewing it? (Or you can just review it as it is; clinic changes aside, it is not too big of a change.)
Will review this in one piece though there are other things in queue so will have to wait.
I wonder if I should make a competing PR, where I store the module state as part of the type structs. It should make for less churn.
@kumaraditya303, are you fine with this PR as it stands? If so, let's land it.
Thanks for the review, Kumar. I'll wait until Monday with landing this, to give @pganssle a chance to chime in.
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
Hopefully the tests I put in place for the cache stuff are robust 😅 That kind of thing is hard to test well, and this is the kind of change that really needs it 😛
Yeah, I know, changes like this need really good test suites! I'll take a look at the coverage before and after the change, just to see that we've at least got all the branches covered.
Thanks for the review 🙂
I don't think it needs to block this particular PR, but is the subinterpreter work advanced enough that we can add an actual test that the module isolation worked yet?
I'm guessing the answer is no because we need to import datetime to get the timedeltas, and datetime is not isolated?
I don't think it needs to block this particular PR, but is the subinterpreter work advanced enough that we can add an actual test that the module isolation worked yet?
Subinterpreter testing is not optimal yet. There's some machinery in test_embed / Programs/_testembed.c. See also #98627. Thanks for bringing that up.
I'm guessing the answer is no because we need to import
datetimeto get the timedeltas, anddatetimeis not isolated?
Correct, _datetime is not isolated yet; I've got a fairly up to date proof-of-concept patch for it, though.