Make install_xbuildenv work even if user doesn't source the venv

[hoodmane]

This was written assuming that the user sourced the virtual environment explicitly, but it fails if they do .venv/bin/pyodide venv .venv-pyodide. There's probably some other places with similar problems.

@henryiii does this look reasonable to you?

[hoodmane]

Probably the added functions should be in build_env.py instead of bash_runner.py...

[henryiii]

The standard way to detect a venv is to check for pyvenv.cfg at the base of the environment. If it exists, it's a venv (and the contents tell you various details about the venv). Is that helpful here?

[hoodmane]

The standard way to detect a venv is to check for pyvenv.cfg at the base of the environment.

Well the situation is that I want someone to be able to say .venv/bin/some-pyodide-cmd and have the implementation of some-pyodide-cmd invoke .venv/bin/pip. If the user ran source .venv/bin/activate then that placed .venv/bin/ at the beginning of their path and so pip will resolve to .venv/bin/pip. Otherwise we need to add it.

I thought sys.prefix != sys.base_prefix was a good enough way to test for a virtual environment. But you're saying I should do something more like (Path(sys.prefix) / "pyvenv.cfg").exists()?

[ryanking13]

Generally looks good to me.

[ryanking13]

It would be nice to have some tests for this.

[henryiii]

Here's a recent example: https://github.com/astral-sh/uv/pull/1504/files. (Edit, that looks problematic, I like the way you did it better)

[henryiii]

Do you need to compute bin/pip? Couldn't you use {sys.executable} -m pip?

[hoodmane]

I guess we could also do

{sys.executable} -m pyodide_cli ...

instead of pyodide ... and get rid of this environment calculation entirely.

[hoodmane]

@henryiii I think I found the most to-the-point version of this that I can come up with:

def _calculate_venv_path() -> str:
    """Make sure that the current executable is the first one on the path."""
    path = os.environ["PATH"]
    if shutil.which(Path(sys.executable).name) == sys.executable:
        return path
    # We aren't the first entry on the path. Add current executable directory to
    # the front of path to correct this.
    bin_dir = str(Path(sys.executable).parent)
    return f"{bin_dir}:{path}"

Though perhaps an even simpler option would be to unconditionally prepend bin_dir to the path, since it's not like doubling it up harms anyone much.

[hoodmane]

Okay, just using sys.executable -m some_module everywhere is definitely the simplest thing we can do so I'll stick with that. This was surprisingly confusing...

[hoodmane]

Hmm well the last iteration seems particularly broken...