From 9109741e847d8b28edd32925ffacd4a6a666809e Mon Sep 17 00:00:00 2001 From: Gyeongjae Choi Date: Thu, 28 Nov 2024 16:12:54 +0900 Subject: [PATCH 1/2] Add config variable for setting index url for build requirements (#64) This adds a new configuration variable, `build_dependency_index_url,` which will be used (TODO) to install build requirements when building packages. I'm splitting PRs to make the changes smaller. related: #43 --- pyodide_build/config.py | 3 +++ pyodide_build/tests/test_config.py | 2 ++ 2 files changed, 5 insertions(+) diff --git a/pyodide_build/config.py b/pyodide_build/config.py index d61b002f..2490ea76 100644 --- a/pyodide_build/config.py +++ b/pyodide_build/config.py @@ -175,6 +175,7 @@ def to_env(self) -> dict[str, str]: "path": "PATH", "zip_compression_level": "PYODIDE_ZIP_COMPRESSION_LEVEL", "skip_emscripten_version_check": "SKIP_EMSCRIPTEN_VERSION_CHECK", + "build_dependency_index_url": "BUILD_DEPENDENCY_INDEX_URL", # maintainer only "_f2c_fixes_wrapper": "_F2C_FIXES_WRAPPER", } @@ -190,6 +191,7 @@ def to_env(self) -> dict[str, str]: "rust_toolchain", "meson_cross_file", "skip_emscripten_version_check", + "build_dependency_index_url", # maintainer only "_f2c_fixes_wrapper", } @@ -208,6 +210,7 @@ def to_env(self) -> dict[str, str]: # Other configuration "pyodide_jobs": "1", "skip_emscripten_version_check": "0", + "build_dependency_index_url": "https://pypi.anaconda.org/pyodide/simple", # maintainer only "_f2c_fixes_wrapper": "", } diff --git a/pyodide_build/tests/test_config.py b/pyodide_build/tests/test_config.py index 88bedb80..69b4911e 100644 --- a/pyodide_build/tests/test_config.py +++ b/pyodide_build/tests/test_config.py @@ -89,6 +89,7 @@ def test_load_config_from_file( ldflags = "-L/path/to/lib" rust_toolchain = "nightly" meson_cross_file = "$(MESON_CROSS_FILE)" + build_dependency_index_url = "https://example.com/simple" """) xbuildenv_manager = CrossBuildEnvManager( @@ -103,6 +104,7 @@ def test_load_config_from_file( assert config["ldflags"] == "-L/path/to/lib" assert config["rust_toolchain"] == "nightly" assert config["meson_cross_file"] == "/path/to/crossfile" + assert config["build_dependency_index_url"] == "https://example.com/simple" def test_config_all(self, dummy_xbuildenv, reset_env_vars, reset_cache): xbuildenv_manager = CrossBuildEnvManager( From 6364ccec5973e21feb7353ff95becbda2d928864 Mon Sep 17 00:00:00 2001 From: Christian Clauss Date: Thu, 28 Nov 2024 15:01:56 +0100 Subject: [PATCH 2/2] Ruff comprehensions and performance (#66) Upgrade [`ruff`](https://docs.astral.sh/ruff) to the current version. Complete the compliance with Pylint rules. Add various list, dict, set comprehension, and performance improvements. * https://docs.astral.sh/ruff/rules/#flake8-comprehensions-c4 * https://docs.astral.sh/ruff/rules/#perflint-perf --- .pre-commit-config.yaml | 2 +- pyodide_build/build_env.py | 4 +++- pyodide_build/common.py | 2 +- pyodide_build/config.py | 1 + pyodide_build/mkpkg.py | 2 +- pyodide_build/out_of_tree/venv.py | 2 ++ pyodide_build/pypabuild.py | 14 +++++++------- pyodide_build/pywasmcross.py | 22 +++++++++++++++------- pyodide_build/tests/test_build_env.py | 2 +- pyodide_build/tests/test_buildpkg.py | 2 +- pyodide_build/tests/test_io.py | 6 +++--- pyodide_build/tests/test_py_compile.py | 6 +++--- pyodide_build/tests/test_pypi.py | 12 +++++++----- pyodide_build/tests/test_pywasmcross.py | 11 ++++++++--- pyodide_build/xbuildenv.py | 1 + pyproject.toml | 25 ++++++++++++++++--------- 16 files changed, 71 insertions(+), 43 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 713bbf64..7f8b9235 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -16,7 +16,7 @@ repos: - id: trailing-whitespace - repo: https://github.com/astral-sh/ruff-pre-commit - rev: "v0.6.9" + rev: "v0.8.0" hooks: - id: ruff args: [--fix] diff --git a/pyodide_build/build_env.py b/pyodide_build/build_env.py index c0d598bb..eeaca3a4 100644 --- a/pyodide_build/build_env.py +++ b/pyodide_build/build_env.py @@ -236,7 +236,9 @@ def emscripten_version() -> str: def get_emscripten_version_info() -> str: """Extracted for testing purposes.""" - return subprocess.run(["emcc", "-v"], capture_output=True, encoding="utf8").stderr + return subprocess.run( + ["emcc", "-v"], capture_output=True, encoding="utf8", check=True + ).stderr def check_emscripten_version() -> None: diff --git a/pyodide_build/common.py b/pyodide_build/common.py index 38dbdbd3..2f930704 100644 --- a/pyodide_build/common.py +++ b/pyodide_build/common.py @@ -73,7 +73,7 @@ def parse_top_level_import_name(whlfile: Path) -> list[str] | None: whlzip = zipfile.Path(whlfile) def _valid_package_name(dirname: str) -> bool: - return all([invalid_chr not in dirname for invalid_chr in ".- "]) + return all(invalid_chr not in dirname for invalid_chr in ".- ") def _has_python_file(subdir: zipfile.Path) -> bool: queue = deque([subdir]) diff --git a/pyodide_build/config.py b/pyodide_build/config.py index 2490ea76..89cdc809 100644 --- a/pyodide_build/config.py +++ b/pyodide_build/config.py @@ -66,6 +66,7 @@ def _get_make_environment_vars(self) -> Mapping[str, str]: capture_output=True, text=True, env={"PYODIDE_ROOT": str(self.pyodide_root)}, + check=False, ) if result.returncode != 0: diff --git a/pyodide_build/mkpkg.py b/pyodide_build/mkpkg.py index 4c45bba7..ca2db67d 100755 --- a/pyodide_build/mkpkg.py +++ b/pyodide_build/mkpkg.py @@ -138,7 +138,7 @@ def _download_wheel(pypi_metadata: URLDict) -> Iterator[Path]: def run_prettier(meta_path: str | Path) -> None: - subprocess.run(["npx", "prettier", "-w", meta_path]) + subprocess.run(["npx", "prettier", "-w", meta_path], check=True) def make_package( diff --git a/pyodide_build/out_of_tree/venv.py b/pyodide_build/out_of_tree/venv.py index f19c3e90..593167ac 100644 --- a/pyodide_build/out_of_tree/venv.py +++ b/pyodide_build/out_of_tree/venv.py @@ -95,6 +95,7 @@ def get_pip_monkeypatch(venv_bin: Path) -> str: ], capture_output=True, encoding="utf8", + check=False, ) check_result(result, "ERROR: failed to invoke Pyodide") platform_data = result.stdout @@ -248,6 +249,7 @@ def install_stdlib(venv_bin: Path) -> None: ], capture_output=True, encoding="utf8", + check=False, ) check_result(result, "ERROR: failed to install unvendored stdlib modules") diff --git a/pyodide_build/pypabuild.py b/pyodide_build/pypabuild.py index 5f7877bd..c72574ea 100644 --- a/pyodide_build/pypabuild.py +++ b/pyodide_build/pypabuild.py @@ -258,13 +258,13 @@ def get_build_env( a package with pypa/build. """ - kwargs = dict( - pkgname=pkgname, - cflags=cflags, - cxxflags=cxxflags, - ldflags=ldflags, - target_install_dir=target_install_dir, - ) + kwargs = { + "pkgname": pkgname, + "cflags": cflags, + "cxxflags": cxxflags, + "ldflags": ldflags, + "target_install_dir": target_install_dir, + } args = common.environment_substitute_args(kwargs, env) args["builddir"] = str(Path(".").absolute()) diff --git a/pyodide_build/pywasmcross.py b/pyodide_build/pywasmcross.py index 858b7481..76ee51b8 100755 --- a/pyodide_build/pywasmcross.py +++ b/pyodide_build/pywasmcross.py @@ -351,7 +351,11 @@ def calculate_object_exports_readobj(objects: list[str]) -> list[str] | None: "-st", ] + objects completedprocess = subprocess.run( - args, encoding="utf8", capture_output=True, env={"PATH": os.environ["PATH"]} + args, + encoding="utf8", + capture_output=True, + env={"PATH": os.environ["PATH"]}, + check=False, ) if completedprocess.returncode: print(f"Command '{' '.join(args)}' failed. Output to stderr was:") @@ -367,7 +371,11 @@ def calculate_object_exports_readobj(objects: list[str]) -> list[str] | None: def calculate_object_exports_nm(objects: list[str]) -> list[str]: args = ["emnm", "-j", "--export-symbols"] + objects result = subprocess.run( - args, encoding="utf8", capture_output=True, env={"PATH": os.environ["PATH"]} + args, + encoding="utf8", + capture_output=True, + env={"PATH": os.environ["PATH"]}, + check=False, ) if result.returncode: print(f"Command '{' '.join(args)}' failed. Output to stderr was:") @@ -475,9 +483,9 @@ def handle_command_generate_args( # noqa: C901 return ["emcc", "-v"] cmd = line[0] - if cmd == "c++" or cmd == "g++": + if cmd in {"c++", "g++"}: new_args = ["em++"] - elif cmd in ("cc", "gcc", "ld", "lld"): + elif cmd in {"cc", "gcc", "ld", "lld"}: new_args = ["emcc"] # distutils doesn't use the c++ compiler when compiling c++ if any(arg.endswith((".cpp", ".cc")) for arg in line): @@ -514,7 +522,7 @@ def handle_command_generate_args( # noqa: C901 ] return line - elif cmd in ("install_name_tool", "otool"): + elif cmd in {"install_name_tool", "otool"}: # In MacOS, meson tries to run install_name_tool to fix the rpath of the shared library # assuming that it is a ELF file. We need to skip this step. # See: https://github.com/mesonbuild/meson/issues/8027 @@ -597,7 +605,7 @@ def handle_command( if tmp is None: # No source file, it's a query for information about the compiler. Pretend we're # gfortran by letting gfortran handle it - return subprocess.run(line).returncode + return subprocess.run(line, check=False).returncode line = tmp @@ -608,7 +616,7 @@ def handle_command( scipy_fixes(new_args) - result = subprocess.run(new_args) + result = subprocess.run(new_args, check=False) return result.returncode diff --git a/pyodide_build/tests/test_build_env.py b/pyodide_build/tests/test_build_env.py index 47889ab4..dc57839f 100644 --- a/pyodide_build/tests/test_build_env.py +++ b/pyodide_build/tests/test_build_env.py @@ -66,7 +66,7 @@ def test_get_build_environment_vars( build_vars = build_env.get_build_environment_vars(manager.pyodide_root) # extra variables that does not come from config files. - extra_vars = set(["PYODIDE", "PYODIDE_PACKAGE_ABI", "PYTHONPATH"]) + extra_vars = {"PYODIDE", "PYODIDE_PACKAGE_ABI", "PYTHONPATH"} all_keys = set(BUILD_KEY_TO_VAR.values()) | extra_vars for var in build_vars: diff --git a/pyodide_build/tests/test_buildpkg.py b/pyodide_build/tests/test_buildpkg.py index 2eaf3fa2..b86c4a0a 100644 --- a/pyodide_build/tests/test_buildpkg.py +++ b/pyodide_build/tests/test_buildpkg.py @@ -143,7 +143,7 @@ def touch(path: Path) -> None: def rlist(input_dir): """Recursively list files in input_dir""" - paths = list(sorted(input_dir.rglob("*"))) + paths = sorted(input_dir.rglob("*")) res = [] for el in paths: diff --git a/pyodide_build/tests/test_io.py b/pyodide_build/tests/test_io.py index 3f0bc3c9..59fc4c1c 100644 --- a/pyodide_build/tests/test_io.py +++ b/pyodide_build/tests/test_io.py @@ -61,9 +61,9 @@ def test_is_rust_package_1(exe): @pytest.mark.parametrize( "reqs", [ - dict(), - dict(requirements={"host": ["rustc"]}), - dict(requirements={"executable": ["something_else"]}), + {}, + {"requirements": {"host": ["rustc"]}}, + {"requirements": {"executable": ["something_else"]}}, ], ) def test_is_rust_package_2(reqs): diff --git a/pyodide_build/tests/test_py_compile.py b/pyodide_build/tests/test_py_compile.py index 50aa2f04..5fd2c326 100644 --- a/pyodide_build/tests/test_py_compile.py +++ b/pyodide_build/tests/test_py_compile.py @@ -87,7 +87,7 @@ def test_py_compile_zip(tmp_path, keep): else: expected = {"test1.zip"} - assert set(el.name for el in tmp_path.glob("*")) == expected + assert {el.name for el in tmp_path.glob("*")} == expected with zipfile.ZipFile(archive_path) as fh_zip: assert fh_zip.namelist() == ["packageA/c/a.pyc", "packageA/d.c"] @@ -215,7 +215,7 @@ def test_py_compile_archive_dir(tmp_path, with_lockfile): expected_in.add("pyodide-lock.json") expected_out.add("pyodide-lock.json") - assert set(el.name for el in tmp_path.glob("*")) == expected_in + assert {el.name for el in tmp_path.glob("*")} == expected_in mapping = _py_compile_archive_dir(tmp_path, keep=False) @@ -224,7 +224,7 @@ def test_py_compile_archive_dir(tmp_path, with_lockfile): "test1.zip": "test1.zip", } - assert set(el.name for el in tmp_path.glob("*")) == expected_out + assert {el.name for el in tmp_path.glob("*")} == expected_out if not with_lockfile: return diff --git a/pyodide_build/tests/test_pypi.py b/pyodide_build/tests/test_pypi.py index 49c760cf..418f05b7 100644 --- a/pyodide_build/tests/test_pypi.py +++ b/pyodide_build/tests/test_pypi.py @@ -46,9 +46,9 @@ def _make_fake_package( else: requirements.append(requirement) extras_requirements_text = "" - for e in extras_requirements.keys(): - extras_requirements_text += f"{e} = [\n" - for r in extras_requirements[e]: + for key, value in extras_requirements.items(): + extras_requirements_text += f"{key} = [\n" + for r in value: extras_requirements_text += f"'{r}',\n" extras_requirements_text += "]\n" template = dedent( @@ -100,7 +100,8 @@ def _make_fake_package( build_path, "--outdir", packageDir, - ] + ], + check=True, ) else: # make cython sdist @@ -128,7 +129,8 @@ def _make_fake_package( build_path, "--outdir", packageDir, - ] + ], + check=True, ) diff --git a/pyodide_build/tests/test_pywasmcross.py b/pyodide_build/tests/test_pywasmcross.py index 23a5f774..6b7381c2 100644 --- a/pyodide_build/tests/test_pywasmcross.py +++ b/pyodide_build/tests/test_pywasmcross.py @@ -208,8 +208,12 @@ def test_exports_node(tmp_path): """ (tmp_path / "f1.c").write_text(template % (1, 1, 1)) (tmp_path / "f2.c").write_text(template % (2, 2, 2)) - subprocess.run(["emcc", "-c", tmp_path / "f1.c", "-o", tmp_path / "f1.o", "-fPIC"]) - subprocess.run(["emcc", "-c", tmp_path / "f2.c", "-o", tmp_path / "f2.o", "-fPIC"]) + subprocess.run( + ["emcc", "-c", tmp_path / "f1.c", "-o", tmp_path / "f1.o", "-fPIC"], check=True + ) + subprocess.run( + ["emcc", "-c", tmp_path / "f2.c", "-o", tmp_path / "f2.o", "-fPIC"], check=True + ) assert set(calculate_exports([str(tmp_path / "f1.o")], True)) == {"g1", "h1"} assert set( calculate_exports([str(tmp_path / "f1.o"), str(tmp_path / "f2.o")], True) @@ -222,7 +226,8 @@ def test_exports_node(tmp_path): # Currently if the object file contains bitcode we can't tell what the # symbol visibility is. subprocess.run( - ["emcc", "-c", tmp_path / "f1.c", "-o", tmp_path / "f1.o", "-fPIC", "-flto"] + ["emcc", "-c", tmp_path / "f1.c", "-o", tmp_path / "f1.o", "-fPIC", "-flto"], + check=True, ) assert set(calculate_exports([str(tmp_path / "f1.o")], True)) == {"f1", "g1", "h1"} diff --git a/pyodide_build/xbuildenv.py b/pyodide_build/xbuildenv.py index 3559b7d8..e09cadda 100644 --- a/pyodide_build/xbuildenv.py +++ b/pyodide_build/xbuildenv.py @@ -310,6 +310,7 @@ def _install_cross_build_packages( ], capture_output=True, encoding="utf8", + check=False, ) if result.returncode != 0: diff --git a/pyproject.toml b/pyproject.toml index bc2cfc23..b8b8fd0e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -112,27 +112,30 @@ ignore_missing_imports = true [tool.ruff] +lint.exclude = ["pyodide_build/vendor/*"] lint.select = [ + "ASYNC", # flake8-async "B0", # bugbear (all B0* checks enabled by default) "B904", # bugbear (Within an except clause, raise exceptions with raise ... from err) "B905", # bugbear (zip() without an explicit strict= parameter set.) + "C4", # flake8-comprehensions "C9", # mccabe complexity "E", # pycodestyles - "W", # pycodestyles "F", # pyflakes + "G004", # f-string logging should be avoided "I", # isort + "PERF", # Perflint "PGH", # pygrep-hooks - "PLC", # pylint conventions - "PLE", # pylint errors + "PL", # Pylint "UP", # pyupgrade - "G004", # f-string logging should be avoided + "W", # pycodestyles ] lint.logger-objects = ["pyodide_build.logger.logger"] -lint.ignore = ["E402", "E501", "E731", "E741", "UP038"] +lint.ignore = ["E402", "E501", "E731", "E741", "PERF401", "PLW2901", "UP038"] # line-length = 219 # E501: Recommended goal is 88 to match black -target-version = "py311" +target-version = "py312" [tool.ruff.lint.per-file-ignores] "src/py/_pyodide/_base.py" = [ @@ -144,9 +147,6 @@ target-version = "py311" "src/tests/test_typeconversions.py" = [ "PGH001", # No builtin `eval()` allowed ] -"tools/*" = [ - "B008", # Do not perform function call `typer.Optional` in argument defaults -] [tool.ruff.lint.flake8-bugbear] extend-immutable-calls = ["typer.Argument", "typer.Option"] @@ -163,5 +163,12 @@ known-third-party = [ [tool.ruff.lint.mccabe] max-complexity = 31 # C901: Recommended goal is 10 +[tool.ruff.lint.pylint] +allow-magic-value-types = ["bytes", "int", "str"] +max-args = 16 # Default is 5 +max-branches = 27 # Default is 12 +max-returns = 12 # Default is 6 +max-statements = 58 # Default is 50 + [tool.ruff.lint.flake8-tidy-imports] ban-relative-imports = "all"