-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Preserve mtime of files installed from wheels #13681
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
base: main
Are you sure you want to change the base?
Conversation
| mtime = datetime(*zipinfo.date_time).timestamp() | ||
| os.utime(self.dest_path, (mtime, mtime)) |
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
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.
|
Hmm, looks like 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. |
|
Oh, nevermind, my mistake is that
Will have another look at this tomorrow. |
|
This is now ready for review. There was no discussion of |
|
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. |
|
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). |
This changeset modifies the
mtimeof files extracted from a.whlto match the stored timestamp.Closes #13207.