Skip to content

Make create_pip_script work with uv actually this time #185

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

Merged
merged 6 commits into from
Apr 15, 2025

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Apr 9, 2025

Weird hack to work around:
astral-sh/python-build-standalone#380

If we resolve the symlink all the way, the python-host interpreter works but won't install into our pyodide venv. If we don't resolve the symlink, sys.prefix is calculated incorrectly. To ensure that we get the right sys.prefix, we explicitly set it with the PYTHONHOME environment variable and then call the symlink.

Weird hack to work around:
astral-sh/python-build-standalone#380
If we resolve the symlink all the way, the python-host interpreter works
but won't install into our pyodide venv. If we don't resolve the symlink,
sys.prefix is calculated incorrectly. To ensure that we get the right
sys.prefix, we explicitly set it with the PYTHONHOME environment variable
and then call the symlink.
Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for dealing with these issues.

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Apr 12, 2025

(still needs the integration test, BTW)

Edit: the integration label via #191 will make this a bit nicer.

@agriyakhetarpal agriyakhetarpal added the integration This PR will run the integration tests. This label can be used as a persistent marker to do so. label Apr 12, 2025
@joerick
Copy link

joerick commented Apr 15, 2025

I think the pip shebang issue on macOS can be fixed by executing that with /usr/bin/env i.e. #!/usr/bin/env /path/to/python-host - the reason it doesn't work as-is is that macOS expects script interpreters to be actual executables, not themselves scripts. But the /usr/bin/env program passes that through fine.

However, I don't know how to address the linux problem! My assumption is that pip is succeeding here, but not installing into the right venv?

@hoodmane
Copy link
Member Author

That does look like what happened. But I can try it on local dev and fix it manually. OTOH, I don't have a mac so your advice there is very much appreciated.

@hoodmane
Copy link
Member Author

Okay, I fixed at least a few problems.

@hoodmane
Copy link
Member Author

Okay, it's green. Gonna go ahead and merge this. Thanks again for your help @joerick!

@agriyakhetarpal
Copy link
Member

Thanks a lot, @hoodmane!

@hoodmane
Copy link
Member Author

And via manual testing I can confirm that it now works for me when pyodide-build is installed in a uv-managed venv.

@hoodmane hoodmane merged commit 71a6d09 into pyodide:main Apr 15, 2025
18 checks passed
@hoodmane hoodmane deleted the uv-double-venv branch April 15, 2025 18:27
@hoodmane
Copy link
Member Author

@agriyakhetarpal would you mind making a patch release?

@agriyakhetarpal
Copy link
Member

Sure, @hoodmane. Though, since we didn't include the previously incomplete fix in 0.30.0 (as we reverted it in #181 before releasing), should we create a 0.31 release instead?

@hoodmane
Copy link
Member Author

Not sure what our versioning rule is. But if it's semver-esque, this change itself is not a breaking change. So unless there is another breaking change on the main branch that you're worried about, I'd call it 0.30.1.

@agriyakhetarpal
Copy link
Member

Ah, okay, makes sense to me. Triggering a new release, then!

@agriyakhetarpal
Copy link
Member

Actually, I checked and we do have some new changes merged after 0.30.0 that are strictly features (#186 and #187). So, coupled with this change, it makes for two user-facing features and one bug fix. Given that we don't have branches set up to backport this change—and maybe we don't need them either, unlike how we do for the main Pyodide repo—could we go ahead with 0.31.0, if you're okay with it?

@joerick
Copy link

joerick commented Apr 21, 2025

I've tried this in pypa/cibuildwheel#2002 (installing from main) and it works! The only thing that doesn't quite work yet are package entrypoints like pytest - for those I get the same error. I'm guessing the approach used for pip here will work there too?

hoodmane added a commit to hoodmane/pyodide-build that referenced this pull request May 2, 2025
This fixes a regression introduced in pyodide#185.
hoodmane added a commit to hoodmane/pyodide-build that referenced this pull request May 2, 2025
This fixes a regression introduced in pyodide#185.
hoodmane added a commit that referenced this pull request May 5, 2025
This fixes a regression introduced in #185.
hoodmane added a commit to hoodmane/pyodide-build that referenced this pull request May 23, 2025
This fixes quoting in the pip arguments. Otherwise:
```
.venv-pyodide/bin/pip install 'pkg; sys_platform != "emscripten"'
```
will fail because the quotes are removed and it is interpretered as a
request to install packages named pkg, sys_platform, !=, and emscripten.

This fixes another regression introduced in pyodide#185.
hoodmane added a commit that referenced this pull request May 23, 2025
This fixes quoting in the pip arguments. Otherwise:
```
.venv-pyodide/bin/pip install 'pkg; sys_platform != "emscripten"'
```
will fail because the quotes are removed and it is interpretered as a
request to install packages named pkg, sys_platform, !=, and emscripten.

This fixes another regression introduced in #185.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration This PR will run the integration tests. This label can be used as a persistent marker to do so.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants