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 warning for packages without lower bound with --resolution=lowest #2797

Closed
zanieb opened this issue Apr 3, 2024 · 9 comments · Fixed by #5953
Closed

Add warning for packages without lower bound with --resolution=lowest #2797

zanieb opened this issue Apr 3, 2024 · 9 comments · Fixed by #5953
Labels
error messages Messaging when something goes wrong help wanted Contribution especially encouraged

Comments

@zanieb
Copy link
Member

zanieb commented Apr 3, 2024

per #1718 (comment) we should throw a warning in this case since without a lower bound we will take the oldest version of a package which is actually quite unlikely to be compatible.

@zanieb zanieb added help wanted Contribution especially encouraged error messages Messaging when something goes wrong labels Apr 3, 2024
@ottaviohartman
Copy link

ottaviohartman commented Apr 23, 2024

@zanieb I'd like to try working on this! It's basically my first time in this codebase, so if there's any top-level guidance you can give that would be amazing.

@zanieb
Copy link
Member Author

zanieb commented Apr 23, 2024

Hi! You've picked kind of a hard one for a first task, I'm not sure how to do this off the top of my head.

My first thought is to go to the Resolver in crates/uv-resolver/src/resolver/mod.rs and do something like

        if matches!(
            self.selector.resolution_strategy(),
            ResolutionStrategy::Lowest
        ) {
            for requirement in self.requirements {
                if Some(VersionOrUrl::Version(version)) = requirement.version_or_url {
                    if <the version specifier has no lower bound> {
                        warn_user_once!(...)
                    }
                }
            }
        }

There are a few open questions:

  • Is this the right place to do this? Should we just write a utility and call it from each CLI command implementation?
  • How do we check if there is no lower bound on a version specifier?
  • This doesn't account for self.constraints etc. but needs to, as the bound could be introduced there

Let me know if you want more than that I can look a bit further.

@zanieb
Copy link
Member Author

zanieb commented Apr 23, 2024

This also only seems sufficient for LowestDirect — since these are the direct dependencies. For Lowest, I'm not sure when we will know that there is no lower bound present. cc @charliermarsh ?

@ottaviohartman
Copy link

ottaviohartman commented Apr 25, 2024

Thanks for the guidance @zanieb ! I really appreciate it.

Is this the right place to do this? Should we just write a utility and call it from each CLI command implementation?

I've been playing around to see how feasible this is. The option you suggest of adding it to resolver/mod.rs is pretty straightforward. Moving into a utility function is a little more clean in the sense that it doesn't touch the inner machinery of resolver, however it will be much more complicated since it needs to constrain requirements (again).

How do we check if there is no lower bound on a version specifier?

In my testing, this seems to work

if *dep_version == (Range::full()) {

This doesn't account for self.constraints etc. but needs to, as the bound could be introduced there

One option is to add the warning or utility function here and in similar transitive locations. Would this work?

I'll upload a draft PR to show what I mean.

@ottaviohartman
Copy link

Here's a draft PR with debug logs where the warning might go: https://github.com/ottaviohartman/uv/pull/2/files

@ottaviohartman
Copy link

Opened #4006 , let me know if this is the right track. If not I don't mind closing and handing this task to someone more knowledgeable!

@zanieb
Copy link
Member Author

zanieb commented Jul 18, 2024

I think this is closed in #5142 now

@ottaviohartman
Copy link

Sorry for being AWOL on this!

@zanieb
Copy link
Member Author

zanieb commented Jul 18, 2024

No problem, it happens to all of us :)

konstin added a commit that referenced this issue Aug 9, 2024
Warn when there are missing bounds on transitive dependencies with `--resolution lowest`.

Implemented as a lazy resolution graph check. Dev deps are odd because they are missing the edge from the root that extras have, but this is more complex because we can put dev dep information in a `Requirement` so i special cased them here.

Closes #2797
Should help with #1718
konstin added a commit that referenced this issue Aug 9, 2024
Warn when there are missing bounds on transitive dependencies with `--resolution lowest`.

Implemented as a lazy resolution graph check. Dev deps are odd because they are missing the edge from the root that extras have, but this is more complex because we can put dev dep information in a `Requirement` so i special cased them here.

Closes #2797
Should help with #1718
konstin added a commit that referenced this issue Aug 9, 2024
Warn when there are missing bounds on transitive dependencies with
`--resolution lowest`.

Implemented as a lazy resolution graph check. Dev deps are odd because
they are missing the edge from the root that extras have (they are
currently orphans in the resolution graph), but this is more complex to
solve properly because we can put dev dep information in a `Requirement`
so i special cased them here.

Closes #2797
Should help with #1718

---------

Co-authored-by: Ibraheem Ahmed <ibraheem@ibraheem.ca>
zanieb pushed a commit that referenced this issue Aug 9, 2024
Warn when there are missing bounds on transitive dependencies with
`--resolution lowest`.

Implemented as a lazy resolution graph check. Dev deps are odd because
they are missing the edge from the root that extras have (they are
currently orphans in the resolution graph), but this is more complex to
solve properly because we can put dev dep information in a `Requirement`
so i special cased them here.

Closes #2797
Should help with #1718

---------

Co-authored-by: Ibraheem Ahmed <ibraheem@ibraheem.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error messages Messaging when something goes wrong help wanted Contribution especially encouraged
Projects
None yet
2 participants