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.

Sign up for GitHub

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

Jump to bottom

Remove uses of utcnow in non-vendored code #12006

Merged
merged 4 commits into from
Aug 31, 2023
Merged

Remove uses of utcnow in non-vendored code #12006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f68a41
Remove reference to utcnow
pganssle Apr 30, 2023
339b770
Use aware datetime object in x509.CertificateBuilder
pganssle Apr 30, 2023
e181c4b
Add news entry
pganssle Apr 30, 2023
c14099f
fixup! Remove reference to utcnow
pganssle May 1, 2023
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
1 change: 1 addition & 0 deletions news/12005.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Removed uses of ``datetime.datetime.utcnow`` from non-vendored code.
15 changes: 6 additions & 9 deletions src/pip/_internal/self_outdated_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@
from pip._internal.utils.filesystem import adjacent_tmp_file, check_path_owner, replace
from pip._internal.utils.misc import ensure_dir

_DATE_FMT = "%Y-%m-%dT%H:%M:%SZ"

_WEEK = datetime.timedelta(days=7)

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -73,12 +72,10 @@ def get(self, current_time: datetime.datetime) -> Optional[str]:
if "pypi_version" not in self._state:
return None

seven_days_in_seconds = 7 * 24 * 60 * 60

# Determine if we need to refresh the state
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 •
edited
Loading

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 •
edited
Loading

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.

time_since_last_check = current_time - last_check
if time_since_last_check > _WEEK:
return None

return self._state["pypi_version"]
Expand All @@ -100,7 +97,7 @@ def set(self, pypi_version: str, current_time: datetime.datetime) -> None:
# Include the key so it's easy to tell which pip wrote the
# file.
"key": self.key,
"last_check": current_time.strftime(_DATE_FMT),
"last_check": current_time.isoformat(),
"pypi_version": pypi_version,
}

Expand Down Expand Up @@ -229,7 +226,7 @@ def pip_self_version_check(session: PipSession, options: optparse.Values) -> Non
try:
upgrade_prompt = _self_version_check_logic(
state=SelfCheckState(cache_dir=options.cache_dir),
current_time=datetime.datetime.utcnow(),
current_time=datetime.datetime.now(datetime.timezone.utc),
local_version=installed_dist.version,
get_remote_version=functools.partial(
_get_current_remote_pip_version, session, options
Expand Down
6 changes: 3 additions & 3 deletions tests/lib/certs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from datetime import datetime, timedelta
from datetime import datetime, timedelta, timezone
from typing import Tuple

from cryptography import x509
Expand All @@ -23,8 +23,8 @@ def make_tls_cert(hostname: str) -> Tuple[x509.Certificate, rsa.RSAPrivateKey]:
.issuer_name(issuer)
.public_key(key.public_key())
.serial_number(x509.random_serial_number())
.not_valid_before(datetime.utcnow())
.not_valid_after(datetime.utcnow() + timedelta(days=10))
.not_valid_before(datetime.now(timezone.utc))
.not_valid_after(datetime.now(timezone.utc) + timedelta(days=10))
.add_extension(
x509.SubjectAlternativeName([x509.DNSName(hostname)]),
critical=False,
Expand Down
11 changes: 8 additions & 3 deletions tests/unit/test_self_check_outdated.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ def test_pip_self_version_check_calls_underlying_implementation(
mocked_state.assert_called_once_with(cache_dir=str(tmpdir))
mocked_function.assert_called_once_with(
state=mocked_state(cache_dir=str(tmpdir)),
current_time=datetime.datetime(1970, 1, 2, 11, 0, 0),
current_time=datetime.datetime(
1970, 1, 2, 11, 0, 0, tzinfo=datetime.timezone.utc
),
local_version=ANY,
get_remote_version=ANY,
)
Expand Down Expand Up @@ -167,14 +169,17 @@ def test_writes_expected_statefile(self, tmpdir: Path) -> None:

# WHEN
state = self_outdated_check.SelfCheckState(cache_dir=str(cache_dir))
state.set("1.0.0", datetime.datetime(2000, 1, 1, 0, 0, 0))
state.set(
"1.0.0",
datetime.datetime(2000, 1, 1, 0, 0, 0, tzinfo=datetime.timezone.utc),
)

# THEN
assert state._statefile_path == os.fspath(expected_path)

contents = expected_path.read_text()
assert json.loads(contents) == {
"key": sys.prefix,
"last_check": "2000-01-01T00:00:00Z",
"last_check": "2000-01-01T00:00:00+00:00",
"pypi_version": "1.0.0",
}