Skip to content

gh-75723: Idempotent .pth execution in site.addsitedir - #147951

Closed
asottile wants to merge 7 commits into
python:mainfrom
asottile:asottile-gh-75723
Closed

gh-75723: Idempotent .pth execution in site.addsitedir#147951
asottile wants to merge 7 commits into
python:mainfrom
asottile:asottile-gh-75723

Conversation

@asottile

@asottileasottile commented Mar 31, 2026

Copy link
Copy Markdown
Contributor

@tcely

tcely commented Apr 1, 2026

Copy link
Copy Markdown

@asottile

Do you have any ideas how to explain the test failure?

classDateTimeTestCase(unittest.TestCase):
deftest_default(self):
withmock.patch('time.localtime') aslocaltime_mock:
time_struct=time.struct_time(
[2013, 7, 15, 0, 24, 49, 0, 196, 0])
localtime_mock.return_value=time_struct
localtime=time.localtime()
t=xmlrpclib.DateTime()
self.assertEqual(str(t),
time.strftime("%Y%m%dT%H:%M:%S", localtime))


 FAIL: test_default (test.test_xmlrpc.DateTimeTestCase.test_default)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/runner/Library/Developer/CoreSimulator/Devices/CED4C8DA-56B5-4FBF-A288-C5C2C197B3AD/data/Containers/Bundle/Application/4B219D99-CC21-4E94-B496-5F175FEC5822/iOSTestbed.app/python/lib/python3.15/test/test_xmlrpc.py", line 487, in test_default
self.assertEqual(str(t),
~~~~~~~~~~~~~~~~^^^^^^^^
time.strftime("%Y%m%dT%H:%M:%S", localtime))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: '20260331T20:39:38' != '20130715T00:24:49'
- 20260331T20:39:38
+ 20130715T00:24:49
----------------------------------------------------------------------
Ran 93 tests in 27.345s
FAILED (failures=1)
test test_xmlrpc failed

@FFY00FFY00 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 2, 2026
@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @FFY00 for commit 6c342df 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F147951%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 2, 2026
@FFY00

FFY00 commented Apr 2, 2026

Copy link
Copy Markdown
Member

The test_xmlrpc failure on iOS only seems weird, I triggered a re-run.

@FFY00

FFY00 commented Apr 2, 2026

Copy link
Copy Markdown
Member

Well, the test is still failing. It seems like the time.localtime mock is not being installed in xmlrpc.client, or is losing the side effect after one call, weird.

I am gonna sync with main, just to be sure this is not a known issue.

@FFY00FFY00 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks okay, overall. There is just the test_xmlrpc failure we need to fix, and the news text that should be updated.

@bedevere-app

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@tcely

tcely commented Apr 2, 2026

Copy link
Copy Markdown

@FFY00 After looking into the code for a bit, this is the best test refinement I have come up with:

deftest_default(self):
# Patch BOTH time and localtime inside the client modulewithmock.patch('xmlrpc.client.time.time') astime_mock, \
mock.patch('xmlrpc.client.time.localtime') aslocaltime_mock:
# 1. Force the 'current' time to be a specific floattime_mock.return_value=1373847889.0# 2. Force the conversion to be your specific structtime_struct=time.struct_time([2013, 7, 15, 0, 24, 49, 0, 196, 0])
localtime_mock.return_value=time_structt=xmlrpclib.DateTime()
self.assertEqual(str(t), "20130715T00:24:49")

@picnixz

picnixz commented Apr 2, 2026

Copy link
Copy Markdown
Member

I think this can be simplified as follows:

@mock.patch('xmlrpc.client.time.time', return_value=1373847889.0)@mock.patch('xmlrpc.client.time.localtime',return_value=time.struct_time([2013, 7, 15, 0, 24, 49, 0, 196, 0]),)deftest_default(self, *_):
t=xmlrpclib.DateTime()
self.assertEqual(str(t), "20130715T00:24:49")

@asottile

Copy link
Copy Markdown
ContributorAuthor

I didn't expect the Spanish Inquisition

@bedevere-app

Copy link
Copy Markdown

Nobody expects the Spanish Inquisition!

@FFY00: please review the changes made to this pull request.

@bedevere-app
bedevere-appBot requested a review from FFY00April 5, 2026 18:43

@johnslavikjohnslavik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<insert a cat picture>

Small nits

Comment threadLib/test/test_site.py
Comment on lines +449 to +453
PROG = f'''\
if {self.imported!r} in sys.modules:
open({self.idempotent_fail_path!r}, 'a+').close()
'''
print(f"import sys; exec({PROG!r})", file=FILE)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider this (zero effort because dedent is already imported)

Suggested change
PROG= f'''\
if {self.imported!r} insys.modules:
open({self.idempotent_fail_path!r}, 'a+').close()
'''
print(f"import sys; exec({PROG!r})", file=FILE)
PROG=dedent(f'''\
if {self.imported!r} insys.modules:
open({self.idempotent_fail_path!r}, 'a+').close()
''')
print(f"import sys; exec({PROG!r})", file=FILE)

Comment threadLib/test/test_site.py
@johnslavik

johnslavik commented Apr 5, 2026

Copy link
Copy Markdown
Member

I'm adding backport labels, but take them off if you see any room for surprise in the implications of this change and we want to sell this as a feature

@johnslavikjohnslavik added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Apr 5, 2026
@emmatyping

Copy link
Copy Markdown
Member

It looks like the macOS failure is an instance of #149156

@asottile

Copy link
Copy Markdown
ContributorAuthor

@warsaw@brettcannon this also fixes the same double call bug with the new pep 829 implementation

@zsol-openai

This comment was marked as resolved.

@asottile

This comment was marked as resolved.

@zsol-openai

This comment was marked as resolved.

@warsawwarsaw self-assigned this May 8, 2026
@warsaw

Copy link
Copy Markdown
Member

I want to fix and test this differently in Python 3.15, but this looks pretty good for 3.14 and 3.13. Once I land that in main (and not backport), I'll get this in shape for those older branches.

Thanks for both finding and fixing this bug!

warsaw added a commit that referenced this pull request May 11, 2026
…9583)
* Idempotent `.pth` execution in `site.addsitedir`
* potentially fix monkeypatch leak?
* fix blind copy paste of recommendation
* Update 2026-03-31-16-15-15.gh-issue-75723.BZ4Rsn.rst
* fix implicit merge conflict with 24c4aec
* Add failing tests for gh-75723
Based on @asottle branch !asottle-gh-75723 but refactored for `main`.
This will need a different backport.
* Repair gh-75723
The original fix is here: #147951
but I'm refactoring a bit for `main`.
* Refactor _make_mod() so we can use it to create package modules too
* Add myself to CODEOWNERS for the site module
---------
Co-authored-by: anthony sottile <asottile@umich.edu>
Co-authored-by: Filipe Laíns <lains@riseup.net>
@tcely

Copy link
Copy Markdown

@warsaw@asottile

Anything left for this? Perhaps a mention of which pull request was actually merged with the similar changes. There has been a lot to keep track of since this was opened.

@asottile

Copy link
Copy Markdown
ContributorAuthor

yeah this can be closed out -- superseded by #149799

@asottile
asottile deleted the asottile-gh-75723 branch May 27, 2026 12:53
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting change reviewneeds backport to 3.13bugs and security fixesneeds backport to 3.14bugs and security fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@asottile@tcely@bedevere-bot@FFY00@picnixz@johnslavik@emmatyping@zsol-openai@warsaw