Skip to content

Conversation

@SnoopJ
Copy link
Contributor

@SnoopJ SnoopJ commented Dec 4, 2025

This changeset modifies the mtime of files extracted from a .whl to match the stored timestamp.

Closes #13207.

Comment on lines 376 to 377
mtime = datetime(*zipinfo.date_time).timestamp()
os.utime(self.dest_path, (mtime, mtime))
Copy link
Contributor Author

@SnoopJ SnoopJ Dec 4, 2025

Choose a reason for hiding this comment

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

Perhaps this belongs in a new helper, I'll leave that to maintainer discretion. I've moved this to a separate helper, but the below comment still applies to the newer code.

Additionally, I can see the rationale behind preserving the existing atime here and modifying only the mtime of the file, but it requires the addition of os.stat() (alas, it does not seem to be possible to pass NULL to the underlying call to utime to express "use current time")

Copy link
Member

Choose a reason for hiding this comment

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

This is standard practice for this kind of mtime manipulation, we're not "accessing" the file, and there's no other information to go off in the zip file, so updating both the atime and the mtime to the provided date time by the zip metadata is fine.

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Dec 4, 2025

Hmm, looks like os.utime() doesn't do quite what I expected across platforms, seems that MacOS and Windows (well, some systems, one of them passed?) have some offset of a round number of hours. Timezone issue maybe?

I don't know how to go about debugging that, this behavior does not occur on my local Windows test system and I don't have a MacOS system. Would appreciate any hints about what's going wrong.

FAILED tests/functional/test_install_wheel.py::test_wheel_install_mtime - AssertionError: mtime does not match expected value
assert 1523959024 == 1523973424
 +  where 1523959024 = int(1523959024.0)
 +    where 1523959024.0 = <FoundFile /private/var/folders/p6/nlmq3k8146990kpkxl73mq340000gn/T/pytest-of-runner/pytest-1/popen-gw1/test_wheel_install_mtime0/workspace:venv/lib/python3.13/site-packages/simplewheel/__init__.py>.mtime

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Dec 4, 2025

Oh, nevermind, my mistake is that ZipInfo.date_time is local time, so some extra gymnastics are required to do the right thing here. The systems where this passes probably just happen to be in the same timezone as me:

The central directory timestamp is interpreted as representing local time, rather than UTC time, to match the behavior of other zip tools.

Will have another look at this tomorrow.

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Dec 5, 2025

This is now ready for review. There was no discussion of atime on the originating issue, so I suspect this is one of the main remaining sticking points, aside from general style (or unless I've missed something).

@notatallshaw notatallshaw self-requested a review December 5, 2025 04:01
@notatallshaw
Copy link
Member

Thanks for PR, I will do a review as soon as I have some spare time (definitely before 26.0 but maybe much earlier).

I'm going to try and quantify the performance impact, at least on my machines, and see if varies much between Windows and Linux. Another thing I'm going to check is the mtime of directories, my experience with this kind of mtime manipulation is they often get accidentally updated because they get set when initially extracted and then updated when child items are extracted to them.

@pfmoore
Copy link
Member

pfmoore commented Dec 6, 2025

We should consider pypa/packaging-problems#118 here.

The current spec says nothing about what installers should do with regard to the timestamps of files extracted from wheels, so it's implementation-defined what tools will do. This PR is therefore spec-compliant, although it's equally true that the current behaviour is spec-compliant 🙂

From the linked discussion, there's clearly some disagreement over what the "correct" behaviour is, so I expect this change could be considered by some as breaking existing behaviour. I don't get the impression that anyone cares enough to push for a standard change to mandate any particular behaviour, so what matters to pip should be what is the right behaviour for the majority of our users.

I don't have a strong opinion on the right thing to do here (I said on #13207 that it seemed like a "reasonable request", but I don't personally care either way, certainly not enough to defend the change against people claiming it broke their workflow).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pip updates mtimes when insatlling wheels

3 participants