Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
de24338
Update __init__.py
Shrey-N Mar 3, 2026
ec2958b
Implement test for script installation mtime
Shrey-N Mar 3, 2026
1428e75
Relint test_venv.py and fix trailing whitespace
Shrey-N Mar 3, 2026
8778209
Refine venv test to use Activate.ps1 and check mode (gh-145417)
Shrey-N Mar 3, 2026
52ee5ef
Add news entry
Shrey-N Mar 3, 2026
eec4e46
Rename News
Shrey-N Mar 3, 2026
c4d32d5
Delete Misc/NEWS.d/next/Library/gh-issue-145417.shrey.rst
Shrey-N Mar 3, 2026
88fae45
Apply maintainer's suggestion for docstring clarity
Shrey-N Mar 3, 2026
7dc69f0
Move template protection check before content assertion
Shrey-N Mar 3, 2026
878d3b8
Merge branch 'python:main' into main
Shrey-N Mar 3, 2026
75bd937
Clean up blank lines in test_venv.py
Shrey-N Mar 3, 2026
a76da3e
Enhance test for Activate.ps1 file integrity
Shrey-N Mar 3, 2026
89c0c92
📜🤖 Added by blurb_it.
blurb-it[bot] Mar 3, 2026
431c4ac
Applying maintainer's suggestion for news wording
Shrey-N Mar 3, 2026
9bcf6db
Change Location of time module
Shrey-N Mar 3, 2026
ead93d7
Merge branch 'main' into main
Shrey-N Mar 3, 2026
97652be
Reorder imports alphabetically
Shrey-N Mar 3, 2026
4e127d8
Refactor test for SELinux acc to suggestion
Shrey-N Mar 5, 2026
0e506f1
Fix SELinux context test and update patch usage
Shrey-N Mar 5, 2026
ace163b
Fix mock path for listxattr in SELinux test
Shrey-N Mar 5, 2026
d15ad82
Update test_venv.py
Shrey-N Mar 5, 2026
7abaefc
Fix indentation for test_install_scripts_selinux
Shrey-N Mar 5, 2026
2c23790
Update Misc/NEWS.d/next/Library/2026-03-03-11-49-44.gh-issue-145417.m…
Shrey-N Mar 5, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions Lib/test/test_venv.py
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,47 @@ def create_contents(self, paths, filename):
with open(fn, 'wb') as f:
f.write(b'Still here?')

def test_install_scripts_mtime(self):
"""
Test that install_scripts does not preserve mtime when copying scripts.
Using mtime serves as a proxy to verify that shutil.copy2 (and thus
SELinux bin_t contexts) is not being used during script installation.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's useful to link the issue here so people reading this know why it matters.

See gh-145417.
"""
import time
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps move this to the toplevel imports. All the other unconditional imports are there, at the beginning of the file.


venv_dir = os.path.dirname(venv.__file__)
src_path = os.path.join(venv_dir, 'scripts', 'common', 'Activate.ps1')
src_mtime = os.path.getmtime(src_path)
if abs(time.time() - src_mtime) < 1.0:
time.sleep(1.1)
Copy link
Member

Choose a reason for hiding this comment

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

IMO this test is overengineered and can be simplified. I would prefer a simple test like:

    def test_install_scripts_selinux(self):
        """
        gh-145417: Test that install_scripts does not copy SELinux context when
        copying scripts.
        """
        with patch('os.listxattr') as listxattr_mock:
            venv.create(self.env_dir)
        listxattr_mock.assert_not_called()

Checking mtime is a low-level implementation detail. For example, the FAT file system only has a resolution of 2 seconds and so your test would fail if run on FAT. Other filesystems have a mtime resolution in nanoseconds making the sleep(1.1) inefficient.

I don't think that we should check every aspect of shutil.copy() here (mtime, st_mode, file content, etc.). Your test checks many file properties, except of SELinux context. I would prefer to do the opposite, just check the SELinux context by checking if extended attributes are copied.


rmtree(self.env_dir)
venv.create(self.env_dir)

dst_path = os.path.join(self.env_dir, self.bindir, 'Activate.ps1')
self.assertTrue(os.path.exists(dst_path), "Activate.ps1 not found in venv")
dst_mtime = os.path.getmtime(dst_path)

# shutil.copy should update mtime, whereas shutil.copy2 would preserve it
self.assertNotEqual(src_mtime, dst_mtime,
"mtime was preserved, meaning shutil.copy2 was used")

# Permissions and content should still match
src_stat = os.stat(src_path)
dst_stat = os.stat(dst_path)
self.assertEqual(src_stat.st_mode, dst_stat.st_mode, "File modes do not match")

with open(src_path, 'rb') as f:
src_data = f.read()
with open(dst_path, 'rb') as f:
dst_data = f.read()
self.assertEqual(src_data, dst_data, "File contents do not match")

self.assertNotIn(b'__VENV_PYTHON__', src_data,
"Test assumes Activate.ps1 is a static file, not a template")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Move this before the equality check. It's unlikely the files will be identical when this happens, and this assertion is more meaningful than the previous one.



def test_overwrite_existing(self):
"""
Test creating environment in an existing directory.
Expand Down
2 changes: 1 addition & 1 deletion Lib/venv/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ def skip_file(f):
'may be binary: %s', srcfile, e)
continue
if new_data == data:
shutil.copy2(srcfile, dstfile)
shutil.copy(srcfile, dstfile)
else:
with open(dstfile, 'wb') as f:
f.write(new_data)
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/gh-145417.shrey.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
gh-145417: Fixed venv to prevent incorrect preservation of SELinux contexts when copying scripts by using shutil.copy instead of shutil.copy2.
Loading