Remove uses of utcnow in non-vendored code
#12006
Conversation
Reading through the code:
So as I understand it the version of Pip does not affect the filename. And be aware, it could be quite possible that someone could downgrade their version of Pip and it would read from the same file. I think if the format is changed in the JSON then some kind of magic number should be used in or to generate the filename so that two different formats aren't read or written to the same file. |
This cleans up some of the datetime handling in the self check. Note that this changes the format of the state file, since the datetime now uses ``.isoformat()`` instead of ``.strftime``. Reading an outdated state file will still work on Python 3.11+, but not on earlier versions.
| last_check = datetime.datetime.strptime(self._state["last_check"], _DATE_FMT) | ||
| seconds_since_last_check = (current_time - last_check).total_seconds() | ||
| if seconds_since_last_check > seven_days_in_seconds: | ||
| last_check = datetime.datetime.fromisoformat(self._state["last_check"]) |
There was a problem hiding this comment.
Does fromisoformat support a trailing "Z" meaning UTC in Python versions back to 3.7? Because we need to be able to read older versions of the state file. I know recent Pythons support the Z, but I don't have easy access to 3.7 to test that version.
(Should we include a test that uses the old format, to confirm we don't break it? I'm ambivalent, because on the one hand, the test will only be relevant for a limited time, but on the other hand, it would check this compatibility point).
There was a problem hiding this comment.
No. All versions of Python with a .fromisoformat will parse any valid output of .isoformat, but I think 3.11 is when we added the ability to parse arbitrary ISO formats. As I explicitly mentioned in the commit message and in the description of the PR, my assumption was that no one would care about breaking old state files, since they are basically ephemeral anyway. From my testing, the result seemed to be that you get a warning about failing to read the state file.
Should be easy enough to adapt with a fallback in the event of a failure to parse the date, or by having pip explicitly check the version in the old state file and either determine the parsing algorithm based on that or just invalidate all state files from other versions of pip (or outside some range or something).
I don't think there's anything that can be done about the situation where someone generates a state file with the latest version of pip, then downgrades to an earlier version. Might be good to put an upper limit cap in place now just so that's not a concern in the future if you want to make other backwards-incompatible changes to the state files.
Also, this change to the format is not really that important, it's just a slightly cleaner way to do things IMO. We could also make the utcnow-related changes without any of the changes to the state file.
There was a problem hiding this comment.
Hmm, sorry - I was reading the code wrongly. This is just "do we need to refresh the state because it's over 7 days old?" So we can simply return None (saying that we need to refresh the state) if we hit a parse error. So it's not as big a problem as I was implying. I do think that trapping parse errors is worthwhile, now we're not controlling the serialisation format explicitly.
|
I think this PR is ready to be merged? |
This cleans up some of the datetime handling in the self check and in one test.
Note that this changes the format of the state file, since the datetime now uses
.isoformat()instead of.strftime. Reading an outdated state file will still work on Python 3.11+, but not on earlier versions.I assumed that
pipdoesn't try to persist state files across versions, but if it does it shouldn't be too difficult to adapt this in some way.