-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Make uv pip compile attempt to download a specified --python-version if it can.
#17249
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
base: main
Are you sure you want to change the base?
Conversation
| Ok(Err(error)) => return Err(error.into()), | ||
| Err(error) => return Err(error.into()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a question here regarding if we should instead just report these errors and continue instead of propagating them upstream, as this is kind of a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it seems reasonable to fail on error as long as we don't fail on things like... offline mode being enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this specific context, downloads_enabled is passed the combined management preference, download preference, and offline state. So this block won't get ran in the offline case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the concern is like... if you wifi is off?
| // If both approaches fail, and a specific patch version was requested try | ||
| // again allowing a different patch version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use the same strategy when downloading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems technically correct, though I can't say it's definitely worth it without seeing the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would just be a matter of shoving https://github.com/astral-sh/uv/pull/17249/changes/BASE..37d8cb6fd77ec752c265060db26b92c7584fa78c#diff-d02a01db770da7d68941d9e67624483d066f95310e5c1835dc0edc23f9af6e5cL1469 in a function and then calling it twice...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why github let me make a link and then produced whatever broken thing that was...
I mean the code above in the if downloads_enabled block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's only the error handling that might be annoying? I think you should try it.
Summary
I believe this mostly addresses #16709. Now specifying a simple version with
--pythonor specifying a version using--python-versionwill result in the specified version getting downloaded with a fallback to the previous behaviour if the download fails for some transient reason or if downloads are disabled.The behaviour of how
--pythongets treated as--python-versionif a "simple version" is specified is kept. This means that--python 3.7turns into a soft requirement. This seems at odds with how other similar parts of UV work, but there seem to be quite a few tests which test for this specific behaviour and I think this is best saved for a separate issue.Test Plan
I added a test case which would previously fall back to the default interpreter and warn about it.