Skip to content
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

Remove uses of utcnow in non-vendored code #12006

Merged
merged 4 commits into from
Aug 31, 2023
Merged

Conversation

pganssle
Copy link
Member

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 pip doesn't try to persist state files across versions, but if it does it shouldn't be too difficult to adapt this in some way.

@notatallshaw
Copy link
Member

notatallshaw commented Apr 30, 2023

I assumed that pip doesn't try to persist state files across versions, but if it does it shouldn't be too difficult to adapt this in some way.

Reading through the code:

  1. The name of the file to read is determined by _get_statefile_name
  2. _get_statefile_name returns an encoding of key which comes from self.key
  3. self.key is simply sys.prefix: https://docs.python.org/3/library/sys.html#sys.prefix

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.

src/pip/_internal/self_outdated_check.py Outdated Show resolved Hide resolved
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.
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Everything seems to work now.

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"])
Copy link
Member

@pfmoore pfmoore May 3, 2023

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@pfmoore pfmoore May 4, 2023

Choose a reason for hiding this comment

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

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.

@edgarrmondragon
Copy link
Contributor

I think this PR is ready to be merged?

@pradyunsg pradyunsg merged commit f612503 into pypa:main Aug 31, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants