`pyodide skeleton pypi --update` (or `--update-patched`) shouldn’t need to install xbuildenv if it fails

[agriyakhetarpal]

Description

I'm not sure if this is expected behaviour or not, but the task of updating a package version using these commands downloads and sets up an xbuildenv before trying to update. This is not caught in a Docker container or in the Pyodide repository folder, because the xbuildenv exists there anyway, and I understand that there is currently no use for this command outside of those locations. Thus, while it's probably not very relevant – this could be a potential improvement towards demarcating CLI commands into use cases that are distinct from each other.

However, it could prove useful once we complete the unvendoring of the package recipes into a separate pyodide-recipes organisation/repository (see pyodide/pyodide#4918 and pyodide/pyodide#4987).

Expected behaviour

Since the xbuildenv is not required to update packages, we could refactor our code to not install it at that moment.

[hoodmane]

Yeah that would be good. Ideally someone should go through the code and make sure we only install the xbuildenv when actually needed. It should only be needed to build one or more extension modules.

[agriyakhetarpal]

I looked into this in a bit deeper manner, and it turns out I was slightly incorrect. The command installs the xbuildenv only if it isn't able to update a package.

For example, if one runs

❯ pyodide skeleton pypi --recipe-dir pyodide_build/tests/_test_recipes --update numpy

in the root directory, this works without having to install the xbuildenv:

numpy is out of date: 1.22.3 <= 2.1.1
pyodide_build/tests/_test_recipes/numpy/meta.yaml 11ms
Updated numpy from 1.22.3 to 2.1.1.

However, if the recipe file is not found, or pointed to, or any other potential cause of failure:

❯ pyodide skeleton pypi --update numpy

then we get

Starting new HTTPS connection (1): raw.githubusercontent.com:443
https://raw.githubusercontent.com:443 "GET /pyodide/pyodide/main/pyodide-cross-build-environments.json HTTP/11" 200 917
Downloading Pyodide cross-build environment from https://github.com/pyodide/pyodide/releases/download/0.27.0a2/xbuildenv-0.27.0a2.tar.bz2                         
Installing Pyodide cross-build environment                                                                                                                        
Using Pyodide cross-build environment version: 0.27.0a2                                                                                                           
/Users/agriyakhetarpal/Desktop/agriya-pyodide-build/packages/numpy/meta.yaml does not exist  

So, this seems like it is a case of an unhandled if-else clause or a missing exception somewhere, which prompts this to happen after the command fails. We should fail early at that moment in time.

[agriyakhetarpal]

This is how it happens:

Here, in_xbuildenv() returns True:

https://github.com/pyodide/pyodide-build/blob/21f91b55ae0442b580727c2a038df312bb0b85f2/pyodide_build/cli/skeleton.py#L58-L63

https://github.com/pyodide/pyodide-build/blob/21f91b55ae0442b580727c2a038df312bb0b85f2/pyodide_build/build_env.py#L112-L114

This is because we set the PYODIDE_ROOT environment variable to the current working (root) directory (see the last line here, it might require scrolling):

https://github.com/pyodide/pyodide-build/blob/21f91b55ae0442b580727c2a038df312bb0b85f2/pyodide_build/build_env.py#L42-L61

This means that the _init_xbuild_env method is called, assuming that the Pyodide root does not have the xbuildenv, and therefore, it gets downloaded.

This structure is present for all the right reasons: the [tool._pyodide] table is expected to be just in the pyproject.toml file for the Pyodide project, and not for any other pyproject.toml files that get found recursively. I think the best way to handle this is to try updating the packages earlier than this check, and not refactor too much at the risk of breaking something.

[agriyakhetarpal]

If there's a better way, happy to try that, too!

[agriyakhetarpal]

This is turning out to be trickier than I expected. I'm confused:

def in_xbuildenv() -> bool:
    pyodide_root = get_pyodide_root()
    return pyodide_root.name == "pyodide-root"

So, in_xbuildenv calls get_pyodide_root internally, which is defined as

@functools.cache
def get_pyodide_root() -> Path:
    init_environment()
    return Path(os.environ["PYODIDE_ROOT"])

So, wouldn't in_xbuildenv() always return True, because init_environment() always creates an xbuildenv?

[ryanking13]

Thanks for the report! Would looking for the pyodide root by calling search_pyodide_root first work?

 if build_env.in_xbuildenv(): 
     root = cwd 
 else: 
     root = build_env.search_pyodide_root(cwd) or cwd 

to

root = build_env.search_pyodide_root(cwd)
if not root:
  try_our_best_to_find_pyodide_root()

[agriyakhetarpal]

I think it could work 🤔 Though, we'll need to define try_our_best_to_find_pyodide_root() fully – what would that be? I guess that we can look for [tool._pyodide] in the pyproject.toml file (like we do right now), then look for packages/, and then look at what PYODIDE_ROOT points to if it is set.

A solution could be to remove the environment initialisation entirely, because it is unlikely that a user will run this command outside the Pyodide tree. Even when we unvendor the recipes from the Pyodide repository, we would most likely be setting up the xbuildenv manually in a separate GHA step (and to a specific/dev version), right?

[ryanking13]

Though, we'll need to define try_our_best_to_find_pyodide_root()

Well, I thought about it, and

root = build_env.search_pyodide_root(cwd)
if not root:
  root = cwd

would be fine enough.

This change might make things a little annoying in an in-tree environment (since we'll have to run the command always from the root directory), but it is not a big deal.

[agriyakhetarpal]

Yes, that's what I thought, too. I opened a branch a few days ago at https://github.com/agriyakhetarpal/pyodide-build/tree/fix/no-xbuildenv-when-updating, but it's good to get your confirmation. I'll rebase and add this change.