From 4a7cc0c20e4ce2746c74640e242fd1d9dbda3b7c Mon Sep 17 00:00:00 2001 From: Gyeongjae Choi Date: Mon, 5 Jun 2023 17:34:07 +0900 Subject: [PATCH] Merge initialization functions for in-tree and out-of-tree builds (#3883) --- Makefile.envs | 2 +- pyodide-build/pyodide_build/cli/build.py | 12 +- .../pyodide_build/cli/build_recipes.py | 5 +- pyodide-build/pyodide_build/cli/config.py | 5 +- pyodide-build/pyodide_build/cli/venv.py | 4 +- pyodide-build/pyodide_build/common.py | 94 ++++++---- .../pyodide_build/install_xbuildenv.py | 17 +- .../pyodide_build/out_of_tree/utils.py | 39 ----- .../pyodide_build/tests/test_build_env.py | 165 ++++++++++++++++++ .../pyodide_build/tests/test_common.py | 45 +++-- src/tests/test_cmdline_runner.py | 4 +- 11 files changed, 273 insertions(+), 119 deletions(-) delete mode 100644 pyodide-build/pyodide_build/out_of_tree/utils.py create mode 100644 pyodide-build/pyodide_build/tests/test_build_env.py diff --git a/Makefile.envs b/Makefile.envs index 3a7f8f562..e88e512a3 100644 --- a/Makefile.envs +++ b/Makefile.envs @@ -28,7 +28,7 @@ export TARGETINSTALLDIR=$(PYODIDE_ROOT)/cpython/installs/python-$(PYVERSION) export HOSTINSTALLDIR=$(PYODIDE_ROOT)/packages/.artifacts export HOSTSITEPACKAGES=$(PYODIDE_ROOT)/packages/.artifacts/lib/python$(PYMAJOR).$(PYMINOR)/site-packages export WASM_LIBRARY_DIR=$(PYODIDE_ROOT)/packages/.libs -export WASM_PKG_CONFIG_PATH=$(PYODIDE_ROOT)/packages/.libs/lib/pkgconfig +export PKG_CONFIG_PATH=$(PYODIDE_ROOT)/packages/.libs/lib/pkgconfig export PYTHONINCLUDE=$(PYODIDE_ROOT)/cpython/installs/python-$(PYVERSION)/include/python$(PYMAJOR).$(PYMINOR) diff --git a/pyodide-build/pyodide_build/cli/build.py b/pyodide-build/pyodide_build/cli/build.py index 998930d95..80c7bcf6e 100644 --- a/pyodide-build/pyodide_build/cli/build.py +++ b/pyodide-build/pyodide_build/cli/build.py @@ -10,13 +10,13 @@ import requests import typer from .. import common +from ..common import init_environment from ..out_of_tree import build from ..out_of_tree.pypi import ( build_dependencies_for_wheel, build_wheels_from_pypi_requirements, fetch_pypi_package, ) -from ..out_of_tree.utils import initialize_pyodide_root def pypi( @@ -29,8 +29,6 @@ def pypi( ctx: typer.Context = typer.Context, ) -> Path: """Fetch a wheel from pypi, or build from source if none available.""" - initialize_pyodide_root() - common.check_emscripten_version() backend_flags = ctx.args with tempfile.TemporaryDirectory() as tmpdir: srcdir = Path(tmpdir) @@ -70,8 +68,6 @@ def url( ctx: typer.Context = typer.Context, ) -> Path: """Fetch a wheel or build sdist from url.""" - initialize_pyodide_root() - common.check_emscripten_version() backend_flags = ctx.args with tempfile.TemporaryDirectory() as tmpdir: tmppath = Path(tmpdir) @@ -100,8 +96,6 @@ def source( ctx: typer.Context = typer.Context, ) -> Path: """Use pypa/build to build a Python package from source""" - initialize_pyodide_root() - common.check_emscripten_version() backend_flags = ctx.args built_wheel = build.run(source_location, output_directory, exports, backend_flags) return built_wheel @@ -147,6 +141,10 @@ def main( ctx: typer.Context = typer.Context, ) -> None: """Use pypa/build to build a Python package from source, pypi or url.""" + + init_environment() + common.check_emscripten_version() + if output_directory_compat: print( "--output-directory is deprecated, use --outdir or -o instead", diff --git a/pyodide-build/pyodide_build/cli/build_recipes.py b/pyodide-build/pyodide_build/cli/build_recipes.py index 875c7d04a..9102cc5d7 100644 --- a/pyodide-build/pyodide_build/cli/build_recipes.py +++ b/pyodide-build/pyodide_build/cli/build_recipes.py @@ -5,7 +5,6 @@ import typer from .. import buildall, buildpkg, common, pywasmcross from ..common import get_num_cores, init_environment from ..logger import logger -from ..out_of_tree.utils import initialize_pyodide_root def recipe( @@ -67,7 +66,7 @@ def recipe( ), ) -> None: """Build packages using yaml recipes and create repodata.json""" - initialize_pyodide_root() + init_environment() if common.in_xbuildenv(): common.check_emscripten_version() @@ -81,8 +80,6 @@ def recipe( if not recipe_dir_.is_dir(): raise FileNotFoundError(f"Recipe directory {recipe_dir_} not found") - init_environment() - build_args = pywasmcross.BuildArgs( cflags=cflags, cxxflags=cxxflags, diff --git a/pyodide-build/pyodide_build/cli/config.py b/pyodide-build/pyodide_build/cli/config.py index 35ed8c03f..27353392c 100644 --- a/pyodide-build/pyodide_build/cli/config.py +++ b/pyodide-build/pyodide_build/cli/config.py @@ -1,7 +1,6 @@ import typer -from ..common import get_build_environment_vars -from ..out_of_tree.utils import initialize_pyodide_root +from ..common import get_build_environment_vars, init_environment app = typer.Typer(help="Manage config variables used in pyodide") @@ -19,7 +18,7 @@ def callback() -> None: def _get_configs() -> dict[str, str]: - initialize_pyodide_root(quiet=True) + init_environment(quiet=True) configs: dict[str, str] = get_build_environment_vars() diff --git a/pyodide-build/pyodide_build/cli/venv.py b/pyodide-build/pyodide_build/cli/venv.py index d7656f52f..08b1c695c 100644 --- a/pyodide-build/pyodide_build/cli/venv.py +++ b/pyodide-build/pyodide_build/cli/venv.py @@ -2,8 +2,8 @@ from pathlib import Path import typer +from ..common import init_environment from ..out_of_tree import venv -from ..out_of_tree.utils import initialize_pyodide_root def main( @@ -13,5 +13,5 @@ def main( ), ) -> None: """Create a Pyodide virtual environment""" - initialize_pyodide_root() + init_environment() venv.create_pyodide_venv(dest) diff --git a/pyodide-build/pyodide_build/common.py b/pyodide-build/pyodide_build/common.py index 4e273f9dc..bcd21e192 100644 --- a/pyodide-build/pyodide_build/common.py +++ b/pyodide-build/pyodide_build/common.py @@ -10,7 +10,8 @@ import textwrap import zipfile from collections import deque from collections.abc import Generator, Iterable, Iterator, Mapping -from contextlib import contextmanager +from contextlib import contextmanager, nullcontext, redirect_stdout +from io import StringIO from pathlib import Path from tempfile import TemporaryDirectory from typing import Any, NoReturn @@ -49,9 +50,8 @@ BUILD_VARS: set[str] = { "SIDE_MODULE_CXXFLAGS", "SIDE_MODULE_LDFLAGS", "STDLIB_MODULE_CFLAGS", - "UNISOLATED_PACKAGES", "WASM_LIBRARY_DIR", - "WASM_PKG_CONFIG_PATH", + "PKG_CONFIG_PATH", "CARGO_BUILD_TARGET", "CARGO_TARGET_WASM32_UNKNOWN_EMSCRIPTEN_LINKER", "RUSTFLAGS", @@ -232,16 +232,8 @@ def get_build_environment_vars() -> dict[str, str]: # TODO: Add modifiable configuration file instead. # (https://github.com/pyodide/pyodide/pull/3737/files#r1161247201) env.update({key: os.environ[key] for key in BUILD_VARS if key in os.environ}) - env["PYODIDE"] = "1" - if "PYODIDE_JOBS" in os.environ: - env["PYODIDE_JOBS"] = os.environ["PYODIDE_JOBS"] - - env["PKG_CONFIG_PATH"] = env["WASM_PKG_CONFIG_PATH"] - if "PKG_CONFIG_PATH" in os.environ: - env["PKG_CONFIG_PATH"] += f":{os.environ['PKG_CONFIG_PATH']}" - tools_dir = Path(__file__).parent / "tools" env["CMAKE_TOOLCHAIN_FILE"] = str( @@ -348,18 +340,67 @@ def search_pyodide_root(curdir: str | Path, *, max_depth: int = 5) -> Path: ) -def init_environment() -> None: +def init_environment(*, quiet: bool = False) -> None: + """ + Initialize Pyodide build environment. + This function needs to be called before any other Pyodide build functions. + """ if os.environ.get("__LOADED_PYODIDE_ENV"): return + os.environ["__LOADED_PYODIDE_ENV"] = "1" + + _set_pyodide_root(quiet=quiet) + + +def _set_pyodide_root(*, quiet: bool = False) -> None: + """ + 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. + + Parameters + ---------- + quiet + If True, do not print any messages + """ + + 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: - os.environ["PYODIDE_ROOT"] = str(Path(os.environ["PYODIDE_ROOT"]).resolve()) - else: - os.environ["PYODIDE_ROOT"] = str(search_pyodide_root(os.getcwd())) + 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 + 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) @functools.cache @@ -428,29 +469,6 @@ def chdir(new_dir: Path) -> Generator[None, None, None]: os.chdir(orig_dir) -def set_build_environment(env: dict[str, str]) -> None: - """Assign build environment variables to env. - - Sets common environment between in tree and out of tree package builds. - """ - env.update({key: os.environ[key] for key in BUILD_VARS if key in os.environ}) - env["PYODIDE"] = "1" - - pkg_config_parts = [] - if "WASM_PKG_CONFIG_PATH" in os.environ: - pkg_config_parts.append(env["WASM_PKG_CONFIG_PATH"]) - if "PKG_CONFIG_PATH" in os.environ: - pkg_config_parts.append(os.environ["PKG_CONFIG_PATH"]) - env["PKG_CONFIG_PATH"] = ":".join(pkg_config_parts) - - tools_dir = Path(__file__).parent / "tools" - - env["CMAKE_TOOLCHAIN_FILE"] = str( - tools_dir / "cmake/Modules/Platform/Emscripten.cmake" - ) - env["PYO3_CONFIG_FILE"] = str(tools_dir / "pyo3_config.ini") - - def get_num_cores() -> int: """ Return the number of CPUs the current process can use. diff --git a/pyodide-build/pyodide_build/install_xbuildenv.py b/pyodide-build/pyodide_build/install_xbuildenv.py index 2c39850f0..987afd92e 100644 --- a/pyodide-build/pyodide_build/install_xbuildenv.py +++ b/pyodide-build/pyodide_build/install_xbuildenv.py @@ -10,7 +10,7 @@ from .create_pypa_index import create_pypa_index from .logger import logger -def download_xbuildenv( +def _download_xbuildenv( version: str, xbuildenv_path: Path, *, url: str | None = None ) -> None: from shutil import rmtree, unpack_archive @@ -73,7 +73,7 @@ def install_xbuildenv(version: str, xbuildenv_path: Path) -> None: create_pypa_index(repodata["packages"], xbuildenv_root, cdn_base) -def install(path: Path, *, download: bool = False, url: str | None = None) -> None: +def install(path: Path, *, download: bool = True, url: str | None = None) -> None: """ Install cross-build environment. @@ -96,6 +96,15 @@ def install(path: Path, *, download: bool = False, url: str | None = None) -> No from . import __version__ version = __version__ - if download: - download_xbuildenv(version, path, url=url) + + if not download and not path.exists(): + logger.error("xbuild environment not exists") + raise FileNotFoundError(path) + + if download and path.exists(): + logger.warning("xbuild environment already exists, skipping download") + + elif download: + _download_xbuildenv(version, path, url=url) + install_xbuildenv(version, path) diff --git a/pyodide-build/pyodide_build/out_of_tree/utils.py b/pyodide-build/pyodide_build/out_of_tree/utils.py deleted file mode 100644 index caa563dc5..000000000 --- a/pyodide-build/pyodide_build/out_of_tree/utils.py +++ /dev/null @@ -1,39 +0,0 @@ -import os -from contextlib import ExitStack, redirect_stdout -from io import StringIO -from pathlib import Path - -from ..common import search_pyodide_root - - -def ensure_env_installed(env: Path, *, quiet: bool = False) -> None: - if env.exists(): - return - from .. import __version__ - from ..install_xbuildenv import download_xbuildenv, install_xbuildenv - - if "dev" in __version__: - raise RuntimeError( - "To use out of tree builds with development Pyodide, you must explicitly set PYODIDE_ROOT" - ) - - with ExitStack() as stack: - if quiet: - # Prevent writes to stdout - stack.enter_context(redirect_stdout(StringIO())) - - download_xbuildenv(__version__, env) - install_xbuildenv(__version__, env) - - -def initialize_pyodide_root(*, quiet: bool = False) -> None: - if "PYODIDE_ROOT" in os.environ: - return - try: - os.environ["PYODIDE_ROOT"] = str(search_pyodide_root(Path.cwd())) - return - except FileNotFoundError: - pass - env = Path(".pyodide-xbuildenv").resolve() - os.environ["PYODIDE_ROOT"] = str(env / "xbuildenv/pyodide-root") - ensure_env_installed(env, quiet=quiet) diff --git a/pyodide-build/pyodide_build/tests/test_build_env.py b/pyodide-build/pyodide_build/tests/test_build_env.py new file mode 100644 index 000000000..65c485c0d --- /dev/null +++ b/pyodide-build/pyodide_build/tests/test_build_env.py @@ -0,0 +1,165 @@ +# This file contains tests that ensure build environment is properly initialized in +# both in-tree and out-of-tree builds. + +# TODO: move functions that are tested here to a separate module + +import os +from pathlib import Path + +import pytest + +from conftest import ROOT_PATH +from pyodide_build import common + + +@pytest.fixture(scope="function") +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: + yield + finally: + os.environ.clear() + os.environ.update(old_environ) + + +@pytest.fixture(scope="function") +def reset_cache(): + # Will remove all caches before each test. + + common.get_pyodide_root.cache_clear() + common.get_build_environment_vars.cache_clear() + common.get_unisolated_packages.cache_clear() + + yield + + +class TestInTree: + def test_init_environment(self, reset_env_vars, reset_cache): + assert "PYODIDE_ROOT" not in os.environ + + common.init_environment() + + assert "PYODIDE_ROOT" in os.environ + assert os.environ["PYODIDE_ROOT"] == str(ROOT_PATH) + + def test_init_environment_pyodide_root_already_set( + self, reset_env_vars, reset_cache + ): + assert "PYODIDE_ROOT" not in os.environ + os.environ["PYODIDE_ROOT"] = "/set_by_user" + + common.init_environment() + + assert os.environ["PYODIDE_ROOT"] == "/set_by_user" + + def test_get_pyodide_root(self, reset_env_vars, reset_cache): + assert "PYODIDE_ROOT" not in os.environ + + assert common.get_pyodide_root() == ROOT_PATH + + def test_get_pyodide_root_pyodide_root_already_set( + self, reset_env_vars, reset_cache + ): + assert "PYODIDE_ROOT" not in os.environ + os.environ["PYODIDE_ROOT"] = "/set_by_user" + + assert str(common.get_pyodide_root()) == "/set_by_user" + + def test_search_pyodide_root(self, tmp_path, reset_env_vars, reset_cache): + pyproject_file = tmp_path / "pyproject.toml" + pyproject_file.write_text("[tool.pyodide]") + assert common.search_pyodide_root(tmp_path) == tmp_path + assert common.search_pyodide_root(tmp_path / "subdir") == tmp_path + assert common.search_pyodide_root(tmp_path / "subdir" / "subdir") == tmp_path + + pyproject_file.unlink() + with pytest.raises(FileNotFoundError): + common.search_pyodide_root(tmp_path) + + def test_in_xbuildenv(self, reset_env_vars, reset_cache): + assert not common.in_xbuildenv() + + def test_get_build_environment_vars(self, reset_env_vars, reset_cache): + build_vars = common.get_build_environment_vars() + extra_vars = set( + ["PYODIDE", "PKG_CONFIG_PATH", "CMAKE_TOOLCHAIN_FILE", "PYO3_CONFIG_FILE"] + ) + + for var in build_vars: + assert var in common.BUILD_VARS | extra_vars, f"Unknown {var}" + + # Additionally we set these variables + for var in extra_vars: + assert var in build_vars, f"Missing {var}" + + def test_get_build_flag(self, reset_env_vars, reset_cache): + for key, val in common.get_build_environment_vars().items(): + assert common.get_build_flag(key) == val + + with pytest.raises(ValueError): + common.get_build_flag("UNKNOWN_VAR") + + +class TestOutOfTree(TestInTree): + # TODO: selenium fixture is a hack to make these tests run only after building Pyodide. + @pytest.fixture(scope="function", autouse=True) + def xbuildenv(self, selenium, tmp_path, reset_env_vars, reset_cache): + import subprocess as sp + + assert "PYODIDE_ROOT" not in os.environ + + envpath = Path(tmp_path) / ".pyodide-xbuildenv" + result = sp.run( + [ + "pyodide", + "xbuildenv", + "create", + str(envpath), + "--root", + ROOT_PATH, + "--skip-missing-files", + ] + ) + + assert result.returncode == 0 + + yield tmp_path + + @pytest.fixture(scope="function", autouse=True) + def chdir_xbuildenv(self, xbuildenv): + cur_dir = os.getcwd() + + os.chdir(xbuildenv) + + try: + yield + finally: + os.chdir(cur_dir) + + # Note: other tests are inherited from TestInTree + + def test_init_environment(self, xbuildenv, reset_env_vars, reset_cache): + assert "PYODIDE_ROOT" not in os.environ + + common.init_environment() + + assert "PYODIDE_ROOT" in os.environ + assert os.environ["PYODIDE_ROOT"] == str( + xbuildenv / ".pyodide-xbuildenv/xbuildenv/pyodide-root" + ) + + def test_get_pyodide_root(self, xbuildenv, reset_env_vars, reset_cache): + assert "PYODIDE_ROOT" not in os.environ + + assert ( + common.get_pyodide_root() + == xbuildenv / ".pyodide-xbuildenv/xbuildenv/pyodide-root" + ) + + def test_in_xbuildenv(self, reset_env_vars, reset_cache): + assert common.in_xbuildenv() diff --git a/pyodide-build/pyodide_build/tests/test_common.py b/pyodide-build/pyodide_build/tests/test_common.py index e41a77779..7741f542f 100644 --- a/pyodide-build/pyodide_build/tests/test_common.py +++ b/pyodide-build/pyodide_build/tests/test_common.py @@ -14,7 +14,6 @@ from pyodide_build.common import ( platform, repack_zip_archive, search_pyodide_root, - set_build_environment, ) @@ -249,29 +248,37 @@ def test_repack_zip_archive( assert input_path.stat().st_size == expected_size -def test_set_build_environment(monkeypatch): +def test_get_build_environment_vars_host_env(monkeypatch): + # host environment variables should have precedence over + # variables defined in Makefile.envs + import os - monkeypatch.delenv("PKG_CONFIG_PATH", raising=False) - monkeypatch.delenv("WASM_PKG_CONFIG_PATH", raising=False) - monkeypatch.setenv("RANDOM_ENV", 1234) - e: dict[str, str] = {} - set_build_environment(e) - assert e.get("HOME") == os.environ.get("HOME") - assert e.get("PATH") == os.environ.get("PATH") + get_build_environment_vars.cache_clear() + e = get_build_environment_vars() assert e["PYODIDE"] == "1" - assert "RANDOM_ENV" not in e - assert e["PKG_CONFIG_PATH"] == "" - e = {} + monkeypatch.setenv("HOME", "/home/user") + monkeypatch.setenv("PATH", "/usr/bin:/bin") monkeypatch.setenv("PKG_CONFIG_PATH", "/x/y/z:/c/d/e") - set_build_environment(e) - assert e["PKG_CONFIG_PATH"] == "/x/y/z:/c/d/e" + + get_build_environment_vars.cache_clear() + + e_host = get_build_environment_vars() + assert e_host.get("HOME") == os.environ.get("HOME") + assert e_host.get("PATH") == os.environ.get("PATH") + assert e_host["PKG_CONFIG_PATH"].endswith("/x/y/z:/c/d/e") + + assert e_host.get("HOME") != e.get("HOME") + assert e_host.get("PATH") != e.get("PATH") + assert e_host.get("PKG_CONFIG_PATH") != e.get("PKG_CONFIG_PATH") + + get_build_environment_vars.cache_clear() monkeypatch.delenv("HOME") - monkeypatch.setenv("WASM_PKG_CONFIG_PATH", "/a/b/c") - monkeypatch.setenv("PKG_CONFIG_PATH", "/x/y/z:/c/d/e") - e = {} - set_build_environment(e) + monkeypatch.setenv("RANDOM_ENV", "1234") + + get_build_environment_vars.cache_clear() + e = get_build_environment_vars() assert "HOME" not in e - assert e["PKG_CONFIG_PATH"] == "/a/b/c:/x/y/z:/c/d/e" + assert "RANDOM_ENV" not in e diff --git a/src/tests/test_cmdline_runner.py b/src/tests/test_cmdline_runner.py index 1d091b77a..bf745f563 100644 --- a/src/tests/test_cmdline_runner.py +++ b/src/tests/test_cmdline_runner.py @@ -11,7 +11,7 @@ import pytest import pyodide from pyodide_build.common import emscripten_version, get_pyodide_root -from pyodide_build.install_xbuildenv import download_xbuildenv, install_xbuildenv +from pyodide_build.install_xbuildenv import _download_xbuildenv, install_xbuildenv only_node = pytest.mark.xfail_browsers( chrome="node only", firefox="node only", safari="node only" @@ -441,7 +441,7 @@ def test_pypa_index(tmp_path): expected.""" path = Path(tmp_path) version = "0.21.0" # just need some version that already exists - download_xbuildenv(version, path) + _download_xbuildenv(version, path) # We don't need host dependencies for this test so zero them out (path / "xbuildenv/requirements.txt").write_text("")