Don't set PYODIDE_ROOT inside xbuildenv installation function (pyodid…

…e#3922) This is some cleanup after pyodide#3883. - Make the `PYODIDE_ROOT` be modified only inside the `init_environment` function. - Removes `__LOADED_PYODIDE_ENV`. I think it is redundant as we use `PYODIDE_ROOT` for a similar purpose.
rsohlot · Jun 15, 2023 · 1691d34 · 1691d34
1 parent 5454036
commit 1691d34
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 49 deletions.
diff --git a/pyodide-build/pyodide_build/common.py b/pyodide-build/pyodide_build/common.py
@@ -256,12 +256,18 @@ def get_build_environment_vars() -> dict[str, str]:
     return env
 
 
-def _get_make_environment_vars() -> dict[str, str]:
+def _get_make_environment_vars(*, pyodide_root: Path | None = None) -> dict[str, str]:
     """Load environment variables from Makefile.envs
 
-    This allows us to set all build vars in one place"""
+    This allows us to set all build vars in one place
 
-    PYODIDE_ROOT = get_pyodide_root()
+    Parameters
+    ----------
+    pyodide_root
+        The root directory of the Pyodide repository. If None, this will be inferred.
+    """
+
+    PYODIDE_ROOT = get_pyodide_root() if pyodide_root is None else pyodide_root
     environment = {}
     result = subprocess.run(
         ["make", "-f", str(PYODIDE_ROOT / "Makefile.envs"), ".output_vars"],
@@ -374,63 +380,47 @@ def init_environment(*, quiet: bool = False) -> None:
     """
     Initialize Pyodide build environment.
     This function needs to be called before any other Pyodide build functions.
+
+    Parameters
+    ----------
+    quiet
+        If True, do not print any messages
     """
-    if os.environ.get("__LOADED_PYODIDE_ENV"):
+
+    # Already initialized
+    if "PYODIDE_ROOT" in os.environ:
         return
 
-    os.environ["__LOADED_PYODIDE_ENV"] = "1"
+    try:
+        root = search_pyodide_root(Path.cwd())
+    except FileNotFoundError:  # Not in Pyodide tree
+        root = _init_xbuild_env(quiet=quiet)
 
-    _set_pyodide_root(quiet=quiet)
+    os.environ["PYODIDE_ROOT"] = str(root)
 
 
-def _set_pyodide_root(*, quiet: bool = False) -> None:
+def _init_xbuild_env(*, quiet: bool = False) -> Path:
     """
-    Set PYODIDE_ROOT environment variable.
-
-    This function works both in-tree and out-of-tree builds:
-    - In-tree builds: Searches for the root of the Pyodide repository in parent directories
-    - Out-of-tree builds: Downloads and installs the Pyodide build environment into the current directory
-
-    Note: this function is supposed to be called only in init_environment(), and should not be called directly.
+    Initialize the build environment for out-of-tree builds.
 
     Parameters
     ----------
     quiet
         If True, do not print any messages
-    """
 
+    Returns
+    -------
+        The path to the Pyodide root directory inside the xbuild environment
+    """
     from . import install_xbuildenv  # avoid circular import
 
-    # If we are building docs, we don't need to know the PYODIDE_ROOT
-    if "sphinx" in sys.modules:
-        os.environ["PYODIDE_ROOT"] = ""
-        return
-
-    # 1) If PYODIDE_ROOT is already set, do nothing
-    if "PYODIDE_ROOT" in os.environ:
-        return
-
-    # 2) If we are doing an in-tree build,
-    #    set PYODIDE_ROOT to the root of the Pyodide repository
-    try:
-        os.environ["PYODIDE_ROOT"] = str(search_pyodide_root(Path.cwd()))
-        return
-    except FileNotFoundError:
-        pass
-
-    # 3) If we are doing an out-of-tree build,
-    #    download and install the Pyodide build environment
+    # TODO: Do not hardcode the path
+    # TODO: Add version numbers to the path
     xbuildenv_path = Path(".pyodide-xbuildenv").resolve()
 
-    if xbuildenv_path.exists():
-        os.environ["PYODIDE_ROOT"] = str(xbuildenv_path / "xbuildenv" / "pyodide-root")
-        return
-
     context = redirect_stdout(StringIO()) if quiet else nullcontext()
     with context:
-        # install_xbuildenv will set PYODIDE_ROOT env variable, so we don't need to do it here
-        # TODO: return the path to the xbuildenv instead of setting the env variable inside install_xbuildenv
-        install_xbuildenv.install(xbuildenv_path, download=True)
+        return install_xbuildenv.install(xbuildenv_path, download=True)
 
 
 @functools.cache

diff --git a/pyodide-build/pyodide_build/install_xbuildenv.py b/pyodide-build/pyodide_build/install_xbuildenv.py
@@ -1,11 +1,10 @@
 import json
-import os
 import shutil
 import subprocess
 from pathlib import Path
 from urllib.request import urlopen, urlretrieve
 
-from .common import exit_with_stdio, get_build_flag
+from .common import _get_make_environment_vars, exit_with_stdio
 from .create_pypa_index import create_pypa_index
 from .logger import logger
 
@@ -31,15 +30,19 @@ def _download_xbuildenv(
         unpack_archive(f.name, xbuildenv_path)
 
 
-def install_xbuildenv(version: str, xbuildenv_path: Path) -> None:
+def install_xbuildenv(version: str, xbuildenv_path: Path) -> Path:
     logger.info("Installing xbuild environment")
 
     xbuildenv_path = xbuildenv_path / "xbuildenv"
     xbuildenv_root = xbuildenv_path / "pyodide-root"
 
-    os.environ["PYODIDE_ROOT"] = str(xbuildenv_root)
+    if (xbuildenv_path / ".installed").exists():
+        return xbuildenv_root
 
-    host_site_packages = Path(get_build_flag("HOSTSITEPACKAGES"))
+    # TODO: use a separate configuration file for variables that are used only in package building
+    host_site_packages = Path(
+        _get_make_environment_vars(pyodide_root=xbuildenv_root)["HOSTSITEPACKAGES"]
+    )
     host_site_packages.mkdir(exist_ok=True, parents=True)
     result = subprocess.run(
         [
@@ -72,8 +75,12 @@ def install_xbuildenv(version: str, xbuildenv_path: Path) -> None:
     version = repodata["info"]["version"]
     create_pypa_index(repodata["packages"], xbuildenv_root, cdn_base)
 
+    (xbuildenv_path / ".installed").touch()
+
+    return xbuildenv_root
 
-def install(path: Path, *, download: bool = True, url: str | None = None) -> None:
+
+def install(path: Path, *, download: bool = True, url: str | None = None) -> Path:
     """
     Install cross-build environment.
 
@@ -92,6 +99,10 @@ def install(path: Path, *, download: bool = True, url: str | None = None) -> Non
         Warning: if you are downloading from a version that is not the same
         as the current version of pyodide-build, make sure that the cross-build
         environment is compatible with the current version of Pyodide.
+
+    Returns
+    -------
+    Path to the Pyodide root directory for the cross-build environment.
     """
     from . import __version__
 
@@ -107,4 +118,4 @@ def install(path: Path, *, download: bool = True, url: str | None = None) -> Non
     elif download:
         _download_xbuildenv(version, path, url=url)
 
-    install_xbuildenv(version, path)
+    return install_xbuildenv(version, path)
diff --git a/pyodide-build/pyodide_build/tests/test_build_env.py b/pyodide-build/pyodide_build/tests/test_build_env.py
@@ -17,7 +17,6 @@ def reset_env_vars():
     # Will reset the environment variables to their original values after each test.
 
     os.environ.pop("PYODIDE_ROOT", None)
-    os.environ.pop("__LOADED_PYODIDE_ENV", None)
     old_environ = dict(os.environ)
 
     try: