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

✨ exhaustive dependencies_in_sync #51

Merged
merged 3 commits into from
Jan 2, 2024
Merged

Conversation

juftin
Copy link
Owner

@juftin juftin commented Dec 21, 2023

resolves #50

Changes

Instead of only checking packages specified on the pyproject.toml when running self.dependencies_in_sync() this PR uses all packages from the lockfile instead.

This is done by reading an exhaustive list of dependencies from the lockfile and passing them to hatchling.dep.core.dependencies_in_sync.

This PR uses piptools._compat.pip_compat to interface with the lockfile.

@juftin
Copy link
Owner Author

juftin commented Dec 21, 2023

I'm super happy with this one. It goes along perfectly with #48. I tested by changing a minor version of a package on the lockfile and it picked it up automatically and installed the new version.

pyproject.toml Outdated
@@ -20,7 +20,8 @@ classifiers = [
"Programming Language :: Python :: Implementation :: PyPy"
]
dependencies = [
"hatch>=1.7.0"
"hatch>=1.7.0,<2",
"pip-tools>=6,<8"
Copy link
Owner Author

@juftin juftin Dec 21, 2023

Choose a reason for hiding this comment

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

I found our usage of pip-tools API to be compatible with 6.x and 7.x. 8.x isn't released so I pinned it on >=6,<8. This doesn't affect which version of pip-tools gets installed into the virtualenvs and invoked via the API though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't add upper version constraints.
https://iscinumpy.dev/post/bound-version-constraints/

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, that was my initial inclination as well - thanks for the gut check. I'll remove the upper bound and keep an eye out for any breaking API changes

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that a private import is used.. that's definitely more scary then 😕
Is there any way to instead copy their code or something

Copy link
Owner Author

@juftin juftin Dec 21, 2023

Choose a reason for hiding this comment

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

The private import is the reason I was so strict on the version pinning. Unfortunately it's private imports all the way down as pip-tools uses a bunch of private methods from pip https://github.com/jazzband/pip-tools/blob/main/piptools/_compat/pip_compat.py

Ultimately it's pip._internal.req.parse_requirements that this PR is leveraging.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I could trade pip-tools private imports for pip private imports. Not sure if this makes anything better though.

from packaging.requirements import Requirement
from pip._internal.network.session import PipSession
from pip._internal.req import parse_requirements
from pip._internal.req.constructors import install_req_from_parsed_requirement


def lockfile_to_requirements(lockfile: str) -> list[Requirement]:
    requirements = []
    session = PipSession()
    for parsed_req in parse_requirements(
        lockfile, session, finder=None, options=None, constraint=False
    ):
        install_req = install_req_from_parsed_requirement(parsed_req, isolated=False)
        requirements.append(install_req.req)
    return requirements

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah nothing to do about imports then.

I was just checking that pip-tools doesn't pin pip's version. But of course that makes sense, pinning it could cause unimaginable issues

Copy link
Owner Author

Choose a reason for hiding this comment

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

They enforce >= 22.2 https://github.com/jazzband/pip-tools/blob/e02d186efc74bd2509b4a0f038b0184bdc106e42/pyproject.toml#L41

So just to confirm here, you're recommending to go forward with the private piptools import and to remove the upper bound dependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so..

@juftin juftin requested a review from oprypin December 22, 2023 04:34
@juftin juftin merged commit 99767d9 into main Jan 2, 2024
9 checks passed
@juftin juftin deleted the feat/exhaustive_dependency_check branch January 2, 2024 19:43
github-actions bot added a commit that referenced this pull request Jan 2, 2024
# [v1.9.0](v1.8.3...v1.9.0) (2024-01-02)

## ✨ New Features
- [`99767d9`](99767d9)  exhaustive dependencies in sync (Issues: [`#51`](#51))

[skip ci]
@juftin
Copy link
Owner Author

juftin commented Jan 2, 2024

🎉 This PR is included in version 1.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@juftin juftin added the released label Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exhaustively check dependencies_in_sync
2 participants