-
Notifications
You must be signed in to change notification settings - Fork 122
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
env: add support for uv #751
Conversation
779e7bc
to
77bf09d
Compare
It looks like |
Yeah astral-sh/uv#2096, there's no official support for PyPy today, and what works, it works by chance. |
@layday have you considered exposing an API for pluggable build env provisioners? |
Can you go into more detail? |
I mean, it'd be nice if third parties could stick their bits of build env provisioning logic through some standardized interface. This could be useful for various downstreams and probably other tools that act as frontends. |
19.0 incorrectly interprets Python version constraints resulting in venv being used in pref to virtualenv when an outer pip is too old.
So, this adds an I've modified the venv selection order slightly so that The Finally, I've added a |
|
src/build/env.py
Outdated
# find it under ``sys.prefix``, essentially potentially rearranging | ||
# the user's $PATH. We'll only look for uv under the prefix of | ||
# the running interpreter for unactivated venvs then defer to $PATH. | ||
uv_bin = shutil.which('uv', path=sysconfig.get_path('scripts')) |
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.
uv comes with a find_uv_bin method we should use that.
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.
The comment explains why it's not being used.
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.
Guess I agree here with uv's choice, I find it more expected, and disagree with the comment. And even if should uv's strategy be an actual problem, I'd like them to fix it, and not us work around it.
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.
How is it expected that a uv you've installed in a venv might load a uv from an outer env?
$ python -m venv --without-pip foo
$ VIRTUAL_ENV=foo uv pip install uv
Resolved 1 package in 3ms
Installed 1 package in 5ms
+ uv==0.1.16
$ foo/bin/python -c 'import uv; print(uv.find_uv_bin())'
/Users/dimitris/code/python-packaging/build/foo/bin/uv
$ rm foo/bin/uv
$ foo/bin/python -c 'import uv; print(uv.find_uv_bin())'
/Users/dimitris/.local/bin/uv
If uv should function as a Python package, it has no business looking outside its prefix.
I'd personally prefer to take a standards-based approach than rely on uv
's custom script finder. Is this not the correct way to locate a script under the running interpreter's prefix?
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.
Open an issue in UV and we should fix it there then.
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.
Changed to only use uv.find_uv_bin
.
uv_bin = shutil.which('uv', path=sysconfig.get_path('scripts')) | ||
|
||
if not uv_bin: | ||
uv_bin = shutil.which('uv') |
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 strongly disagree with such fallback. Either pick uv from sys.prefix or fail. Using a stray executable we pick up from PATH could use surprises for users...
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've generally been respecting the design of uv, which is to not install itself in environments, but to be used globally (via https://astral.sh/uv/install.sh, cargo install, homebrew, pipx, pacman, etc) and only target environments. If a user does install uv in build's environment, that should take preference, but I think we should try to use an outer/system uv
otherwise. I don't think someone passing --installer=uv
would be unhappy if a uv
executable on their path is used. (same with cmake
, gcc
, ninja
, patchelf
, etc., lots of executables get used from the path)
But willing to go with this if you really prefer.
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.
Then remove the UV extra because user should install it separately? Having an extra but then taking a global is unexpected to me.
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.
A user can install it, say if the system uv
is older than they like, or if they just use pipx run build[uv]
regardless of if the system has uv or not. But, if they are on a system that does not support binary wheels, like BSDs, they might prefer or even need to install it globally and not use the extra.
(This is very common for binary deps with optional PyPI packages like CMake and ninja)
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.
We are not in the business of installing cmake, why is UV any different (other than it happens to be available on pypi, is the same a random binary).
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 I can accept falling back to the operating system version, but if we do that in my opinion we must print out of warning onto the standard output or error to notify the user that this is happening.
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.
Something like this would be perfect, I think:
* Creating venv isolated environment...
* Using uv 0.1.16 from /usr/local/bin/uv to install packages...
* Installing packages in isolated environment... (hatch-fancy-pypi-readme, hatch-vcs, hatchling)
(Maybe it could include "found on PATH" or something too?) Whereas if it was from the local environment, it could be simplified:
* Creating venv isolated environment...
* Using uv 0.1.16 to install packages...
* Installing packages in isolated environment... (hatch-fancy-pypi-readme, hatch-vcs, hatchling)
Would that work?
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.
Perfect.
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.
Reminder that we have a verbosity flag now that will print out the command invoked including the full uv executable path. Printing out the version for a uv executable from $PATH would require an additional subprocess call and parsing the output of -V
which is a flimsy affair. This same consideration also applies to an outer pip, although the surprise factor is slightly diminished since we’re only looking in the running interpreter’s Python path.
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.
@@ -21,6 +20,11 @@ | |||
from ._util import check_dependency | |||
|
|||
|
|||
Installer = typing.Literal['pip', 'uv'] |
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.
Do we wanna call this uv-pip
instead of simply uv
?
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 the --installer
implies uv pip. And if they added a “uv install”, we could transparently start using that instead.
I would’ve preferred to squash both this and the other PR you merged personally. There’s a fair few fix commits and reverts and counter-reverts with attribution ultimately being lost in git blame. I’ll open another PR to add a change log entry. |
No description provided.