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

Fix lock updates for locks with sdist bystanders. #2325

Merged
merged 2 commits into from
Jan 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Release Notes

## 2.1.157

This release fixes a bug in `pex3 lock update` for updates that leave
projects unchanged whose primary artifact is an sdist.

* Fix lock updates for locks with sdist bystanders. (#2325)

## 2.1.156

This release optimizes wheel install overhead for warm caches. Notably,
Expand Down
57 changes: 51 additions & 6 deletions pex/resolve/lockfile/updater.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
from pex.util import named_temporary_file

if TYPE_CHECKING:
from typing import Dict, Iterable, Iterator, List, Mapping, Optional, Tuple, Union
from typing import Any, Dict, Iterable, Iterator, List, Mapping, Optional, Tuple, Union

import attr # vendor:skip
else:
Expand Down Expand Up @@ -316,11 +316,56 @@ def update_resolve(
self.update_constraints_by_project_name
and project_name not in self.update_constraints_by_project_name
):
assert updated_pin == original_pin
assert updated_requirement.artifact == locked_requirement.artifact
assert (
updated_requirement.additional_artifacts
== locked_requirement.additional_artifacts
assert updated_pin == original_pin, (
"The locked requirement {original} should have been undisturbed by the lock "
"update, but it changed to {updated}.".format(
original=original_pin.as_requirement(), updated=updated_pin.as_requirement()
)
)

# N.B.: We use a custom key for artifact equality comparison since `Artifact`
# contains a `verified` attribute that can both vary based on Pex's current
# knowledge about the trustworthiness of an artifact hash and is not relevant to
# whether the artifact refers to the same artifact.
def artifact_key(artifact):
# type: (Artifact) -> Any
return artifact.url, artifact.fingerprint

assert artifact_key(updated_requirement.artifact) == artifact_key(
locked_requirement.artifact
), (
"The locked requirement {original} should have been undisturbed by the lock "
"update, but its primary artifact changed from:\n"
"{original_artifact}\n"
"to:\n"
"{updated_artifact}".format(
original=original_pin.as_requirement(),
original_artifact=artifact_key(locked_requirement.artifact),
updated_artifact=artifact_key(updated_requirement.artifact),
)
)
assert set(map(artifact_key, updated_requirement.additional_artifacts)) == set(
map(artifact_key, locked_requirement.additional_artifacts)
), (
"The locked requirement {original} should have been undisturbed by the lock "
"update, but its additional artifact set changed from:\n"
"{original_artifacts}\n"
"to:\n"
"{updated_artifacts}".format(
original=original_pin.as_requirement(),
original_artifacts="\n".join(
map(
lambda a: str(artifact_key(a)),
locked_requirement.additional_artifacts,
)
),
updated_artifacts="\n".join(
map(
lambda a: str(artifact_key(a)),
updated_requirement.additional_artifacts,
)
),
)
)
elif original_pin != updated_pin:
updates[project_name] = VersionUpdate(
Expand Down
2 changes: 1 addition & 1 deletion pex/version.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

__version__ = "2.1.156"
__version__ = "2.1.157"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't you normally bump this (and edit the CHANGES) in a separate release prep PR? Is this a rebasing error, or deliberate?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is deliberate. I paused this time and asked myself if there was a good reason to keep these separate, and in the case of a single change release, I couldn't think of a good enough reason.

80 changes: 80 additions & 0 deletions tests/integration/test_issue_2324.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
# Copyright 2023 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import print_function

import itertools
import os.path

from pex.pep_440 import Version
from pex.pep_503 import ProjectName
from pex.resolve.locked_resolve import FileArtifact
from pex.resolve.lockfile import json_codec
from pex.resolve.resolved_requirement import Pin
from pex.typing import TYPE_CHECKING
from testing.cli import run_pex3

if TYPE_CHECKING:
from typing import Any


def test_update_sdists_not_updated(tmpdir):
# type: (Any) -> None

constraints = os.path.join(str(tmpdir), "constraints.txt")
with open(constraints, "w") as fp:
print("ansicolors<1.1.8", file=fp)
print("cowsay<6", file=fp)

lock = os.path.join(str(tmpdir), "lock.json")

def assert_lock(*pins):
# type: (*Pin) -> None

lockfile = json_codec.load(lock)
assert 1 == len(lockfile.locked_resolves)
locked_resolve = lockfile.locked_resolves[0]
locked_requirements = {
locked_req.pin: tuple(locked_req.iter_artifacts())
for locked_req in locked_resolve.locked_requirements
}
assert set(pins) == set(locked_requirements)
assert all(
isinstance(artifact, FileArtifact) and artifact.is_source
for artifact in itertools.chain.from_iterable(locked_requirements.values())
)

run_pex3(
"lock",
"create",
"--no-wheel",
"--constraints",
constraints,
"ansicolors",
"cowsay",
"--indent",
"2",
"-o",
lock,
).assert_success()
assert_lock(
Pin(ProjectName("ansicolors"), Version("1.1.7")), Pin(ProjectName("cowsay"), Version("5.0"))
)

# N.B.: Pre-fix this test would lead to an artifact comparison assertion for cowsay, which is
# expected to be unmodified by the lock update.
#
# E Traceback (most recent call last):
# E File "/home/jsirois/dev/pantsbuild/jsirois-pex/pex/result.py", line 105, in catch
# E return func(*args, **kwargs)
# E ^^^^^^^^^^^^^^^^^^^^^
# E File "/home/jsirois/dev/pantsbuild/jsirois-pex/pex/resolve/lockfile/updater.py", line 320, in update_resolve
# E assert updated_requirement.artifact == locked_requirement.artifact
# E ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
# E AssertionError
# E Encountered 1 error updating /tmp/pytest-of-jsirois/pytest-8/test_update_sdists_not_updated0/lock.json:
# E 1.) cp311-cp311-manylinux_2_35_x86_64:
run_pex3("lock", "update", "-v", "-p", "ansicolors<1.1.9", lock).assert_success()
assert_lock(
Pin(ProjectName("ansicolors"), Version("1.1.8")), Pin(ProjectName("cowsay"), Version("5.0"))
)