-
Notifications
You must be signed in to change notification settings - Fork 22
Add initial support for uv #132
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
Conversation
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.
Thanks, @ryanking13! The failing test on macOS looks like an easy fix. Also, marking #58 as related (could you please check if it fully resolves the issue?)
| uv = [ | ||
| "build[uv]~=1.2.0", | ||
| ] |
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.
Side note; I remember that pypa/build 1.2.0 broke things for us last year. Would it be worth it for us to vendor pypa/build, so that pypabuild.py can use it instead?
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.
Side note; I remember that pypa/build 1.2.0 broke things for us last year.
I don't remember this. Could you please elaborate? Note that build~=1.2.0 is also set in the dependencies.
Would it be worth it for us to vendor pypa/build, so that pypabuild.py can use it instead?
No, I don't think vendoring is a good idea for pypa/build case. If we want to pin into a specific version, I would choose to pin the version in pyproject.toml instead.
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.
Ah, sounds good to me, thanks! It was this issue: numpy/numpy#26164
| run_prefix = ( | ||
| [uv_helper.find_uv_bin(), "run"] if uv_helper.should_use_uv() else [] | ||
| ) |
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 should be careful about this, based on https://github.com/pyodide/pyodide-build/pull/132/files#r1996940827.
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.
Hmm, I see. Probably, we should entirely switch from subprocess call to multiprocessing to avoid this kind of problem.
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
for more information, see https://pre-commit.ci
|
Okay, for now I will go ahead and merge. Let's keep discuss how we can improve uv support. |
Adds initial support for uv: make
uv run pyodide ...work. Resolve #58