uv icon indicating copy to clipboard operation
uv copied to clipboard

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

Open zanieb opened this issue 1 year ago • 5 comments

per https://github.com/astral-sh/uv/issues/1718#issuecomment-1954420636 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 avatar Apr 03 '24 04:04 zanieb

@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.

ottaviohartman avatar Apr 23 '24 22:04 ottaviohartman

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 avatar Apr 23 '24 23:04 zanieb

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 ?

zanieb avatar Apr 23 '24 23:04 zanieb

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 avatar Apr 25 '24 01:04 ottaviohartman

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

ottaviohartman avatar Apr 25 '24 01:04 ottaviohartman

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!

ottaviohartman avatar Jun 04 '24 01:06 ottaviohartman

I think this is closed in https://github.com/astral-sh/uv/pull/5142 now

zanieb avatar Jul 18 '24 14:07 zanieb

Sorry for being AWOL on this!

ottaviohartman avatar Jul 18 '24 14:07 ottaviohartman

No problem, it happens to all of us :)

zanieb avatar Jul 18 '24 14:07 zanieb