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

Resolvelib: process Requires-Python before other dependencies #13160

Closed
wants to merge 3 commits into from
Closed

Resolvelib: process Requires-Python before other dependencies #13160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0538477
Fix silently broken requires-python test
ichard26 Jan 12, 2025
e21141a
Emit Requires-Python dependency first
uranusjr Aug 21, 2022
7b6e3b6
Return dependencies lazily in resolvelib provider
ichard26 Jan 29, 2025
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
3 changes: 3 additions & 0 deletions news/11142.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix a regression that causes dependencies to be checked *before* ``Requires-Python``
project metadata is checked, leading to wasted cycles when the Python version is
unsupported.
2 changes: 1 addition & 1 deletion src/pip/_internal/resolution/resolvelib/candidates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,10 @@ def _prepare(self) -> BaseDistribution:
return dist

def iter_dependencies(self, with_requires: bool) -> Iterable[Optional[Requirement]]:
yield self._factory.make_requires_python_requirement(self.dist.requires_python)
requires = self.dist.iter_dependencies() if with_requires else ()
for r in requires:
yield from self._factory.make_requirements_from_spec(str(r), self._ireq)
yield self._factory.make_requires_python_requirement(self.dist.requires_python)

def get_install_requirement(self) -> Optional[InstallRequirement]:
return self._ireq
Expand Down
4 changes: 2 additions & 2 deletions src/pip/_internal/resolution/resolvelib/provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,9 @@ def _eligible_for_upgrade(identifier: str) -> bool:
def is_satisfied_by(self, requirement: Requirement, candidate: Candidate) -> bool:
return requirement.is_satisfied_by(candidate)

def get_dependencies(self, candidate: Candidate) -> Sequence[Requirement]:
def get_dependencies(self, candidate: Candidate) -> Iterable[Requirement]:
with_requires = not self._ignore_dependencies
return [r for r in candidate.iter_dependencies(with_requires) if r is not None]
return (r for r in candidate.iter_dependencies(with_requires) if r is not None)
Copy link
Member

Choose a reason for hiding this comment

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

Why change to an iterator here? I’m not against doing so, but is it needed for this change? If it is, then a comment explaining why would be important.

OK, I think I see why - if it’s not an iterator, we precompute the dependencies, which defeats the purpose of getting requires-python first. That definitely warrants a comment.


@staticmethod
def is_backtrack_cause(
Expand Down
25 changes: 13 additions & 12 deletions tests/functional/test_new_resolver_errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,32 +101,33 @@ def test_new_resolver_checks_requires_python_before_dependencies(
) -> None:
incompatible_python = f"<{sys.version_info.major}.{sys.version_info.minor}"

pkg_dep = create_basic_wheel_for_package(
pkgdep = create_basic_wheel_for_package(
script,
name="pkg-dep",
name="pkgdep",
version="1",
)
create_basic_wheel_for_package(
script,
name="pkg-root",
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to change the names here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, I just cherry picked this change from your original PR. 0538477 (#13160)

Copy link
Member Author

Choose a reason for hiding this comment

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

From the original PR: f3c6b2a (#11398)

Copy link
Member Author

Choose a reason for hiding this comment

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

@uranusjr if you'd like, I can revert the name changes, although I'd prefer leaving your commit unchanged. Do you remember why you changed the names? I can definitely take a closer look at your commit, but if you can answer the review comments from Paul, I would appreciate that.

Your original PR is more than two years old, though, so don't worry if you have no idea.

name="pkgroot",
version="1",
# Refer the dependency by URL to prioritise it as much as possible,
# to test that Requires-Python is *still* inspected first.
depends=[f"pkg-dep@{pathlib.Path(pkg_dep).as_uri()}"],
depends=[
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the comment here? Is it no longer valid?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe to avoid needing to normalise in the check below? Again, if so that deserves a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add the comment back although I can't attest to whether it's still valid.

Again, if so that deserves a comment.

I don't understand why I should add a comment about the removal of another comment.

Copy link
Member

Choose a reason for hiding this comment

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

I think the second comment was related to something else. I can't tell what from the way Github is showing it (I don't normally put 2 separate comments one after the other like this, so I think something changed here from what I originally wrote.

Sorry I can't recall the original context. Lets just ignore what I said here...

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 worries!

f"pkgdep@{pathlib.Path(pkgdep).as_uri()}",
],
requires_python=incompatible_python,
)

result = script.pip(
r = script.pip(
"install",
"--no-cache-dir",
"--no-index",
"--find-links",
script.scratch_path,
"pkg-root",
"pkgroot",
expect_error=True,
)

# Resolution should fail because of pkg-a's Requires-Python.
# This check should be done before pkg-b, so pkg-b should never be pulled.
assert incompatible_python in result.stderr, str(result)
assert "pkg-b" not in result.stderr, str(result)
Copy link
Member

Choose a reason for hiding this comment

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

Was pkg-b a bug in the previous code here? That’s not the correct name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess so? This isn't my change, I cherry picked this from @uranusjr's PR f3c6b2a (#11398)

# Resolution should fail because of pkgroot's Requires-Python.
# This is done before dependencies so pkgdep should never be pulled.
assert incompatible_python in r.stderr, str(r)
assert "pkgdep" not in r.stderr, str(r)
assert "pkgdep" not in r.stdout, str(r)
Loading