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

Add user warning for packages without lower bound with --resolution=lowest or lowest-direct #4006

Conversation

ottaviohartman
Copy link

Summary

Fixes #2797.

Warns the user if a direct or transitive dependency is unpinned when using --resolution=lowest or lowest-direct.

TODO

  • Write unit tests

Test Plan

  • Awaiting confirmation that this implementation looks relatively OK before writing unit tests.

@@ -11,6 +11,7 @@ extend-ignore-re = [
"FrIeNdLy-\\._\\.-bArD",
"I borken you cache",
"eb1ba5f5",
"github_pat_.*"
Copy link
Author

Choose a reason for hiding this comment

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

Had to add this otherwise the pre-commit hook would think Nd should be And in pip_install.rs

@konstin
Copy link
Member

konstin commented Jun 5, 2024

Thanks for creating this PR!

There's two parts to having warnings here: Warnings for direct dependencies, those that the user controls, and warnings for indirect dependencies, those that the user doesn't control.

We can warn for direct dependencies, the user can add dependency constraints (as implemented).

For indirect dependencies, the user can't add constraints on that other package. Whenever we encounter an unconstrained dependency in pubgrub, we should debug! it with the name and version package that contained the dependency (instead of warn_user_once!). This helps with debugging without warning about something that users can't change themselves.

In the resolver, we may encounter unconstrained dependencies that are constrained elsewhere, either by another dependency or by the user passing a constraint files. After the resolver is done, we should note unconstrained dependencies in two case: There was a build failure of a dependency lacking a lower bound. In this case, we should add a note about the missing lower bound to the context of the build error. The other case is that after we're done, we have picked the lowest version of a dependency without any constraint (that is most likely to fail when running the code), we should warn about that and ask the user to add a constraint.

For the tests, we should follow our own best practices and add lower bounds for the lowest(-direct) tests except for one or two tests that check for warnings.

I would suggest splitting this into two PRs: One that only adds the warnings for the lowest-direct mode and debug logging for missing constraint on indirect dependencies, and another one that adds the context on failed builds and warns after the resolution finished.

@ottaviohartman
Copy link
Author

Thanks for the guidance @konstin , it's very helpful.

I'll convert this PR into what you recommend as the first PR:

only adds the warnings for the lowest-direct mode and debug logging for missing constraint on indirect dependencies

What's the best place in the code to put the warning? The resolver, as implemented, misses constraints (I think?) but more importantly doesn't correctly notice that the following is pinned:

anyio @ https://files.pythonhosted.org/packages/2d/b8/7333d87d5f03247215d86a86362fd3e324111788c6cdd8d2e6196a6ba833/anyio-4.2.0.tar.gz

I'll be looking around the code in the meantime to find an appropriate spot to check if a version is unpinned.

@konstin
Copy link
Member

konstin commented Jun 7, 2024

You can check PubGrubPackage for PubGrubPackageInner -> Package -> url: None

@zanieb
Copy link
Member

zanieb commented Jul 18, 2024

Taken up in #5142 now

@zanieb zanieb closed this Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add warning for packages without lower bound with --resolution=lowest
3 participants