-
Notifications
You must be signed in to change notification settings - Fork 22
Refactor cleanup to use RecipeBuilder methods #260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -97,13 +97,36 @@ class RecipeBuilder: | |||||
| A class to build a Pyodide meta.yaml recipe. | ||||||
| """ | ||||||
|
|
||||||
| @staticmethod | ||||||
| def get_default_recipe_dir(root: Path | None = None) -> Path: | ||||||
| """ | ||||||
| Get the default recipe directory for Pyodide packages. | ||||||
|
|
||||||
| Parameters | ||||||
| ---------- | ||||||
| root : Path | None | ||||||
| The Pyodide root directory. If None, searches for it starting from cwd. | ||||||
|
|
||||||
| Returns | ||||||
| ------- | ||||||
| Path | ||||||
| The default recipe directory (<root>/packages) | ||||||
| """ | ||||||
| if root is None: | ||||||
| from pyodide_build.build_env import search_pyodide_root | ||||||
|
|
||||||
| root = search_pyodide_root(Path.cwd()) or Path.cwd() | ||||||
| return root / "packages" | ||||||
|
|
||||||
| def __init__( | ||||||
| self, | ||||||
| recipe: str | Path, | ||||||
| build_args: BuildArgs, | ||||||
| build_dir: str | Path | None = None, | ||||||
| force_rebuild: bool = False, | ||||||
| continue_: bool = False, | ||||||
| *, | ||||||
| build_dir_base: str | Path | None = None, | ||||||
| ): | ||||||
| """ | ||||||
| Parameters | ||||||
|
|
@@ -120,6 +143,10 @@ def __init__( | |||||
| If True, the package will be rebuilt even if it is already up-to-date. | ||||||
| continue_ | ||||||
| If True, continue a build from the middle. For debugging. Implies "force_rebuild". | ||||||
| build_dir_base | ||||||
| Optional root path under which package build directories live: | ||||||
| <build_dir_base>/<package>/build. If provided, overrides the default | ||||||
| per-package location, but can still be overridden directly with build_dir. | ||||||
|
Comment on lines
+146
to
+149
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please explain why we need this extra parameter? |
||||||
| """ | ||||||
| recipe = Path(recipe).resolve() | ||||||
| self.pkg_root, self.recipe = _load_recipe(recipe) | ||||||
|
|
@@ -133,9 +160,13 @@ def __init__( | |||||
| self.package_type = self.build_metadata.package_type | ||||||
| self.is_wheel = self.package_type in ["package", "cpython_module"] | ||||||
|
|
||||||
| self.build_dir = ( | ||||||
| Path(build_dir).resolve() if build_dir else self.pkg_root / "build" | ||||||
| ) | ||||||
| # Priority: build_dir > build_dir_base > default | ||||||
| if build_dir: | ||||||
| self.build_dir = Path(build_dir).resolve() | ||||||
| elif build_dir_base: | ||||||
| self.build_dir = Path(build_dir_base).resolve() / self.name / "build" | ||||||
| else: | ||||||
| self.build_dir = self.pkg_root / "build" | ||||||
| if len(str(self.build_dir).split(maxsplit=1)) > 1: | ||||||
| raise ValueError( | ||||||
| "PIP_CONSTRAINT contains spaces so pip will misinterpret it. Make sure the path to the package build directory has no spaces.\n" | ||||||
|
|
@@ -154,10 +185,53 @@ def __init__( | |||||
| # where Pyodide will look for the built artifacts when building pyodide-lock.json. | ||||||
| # after building packages, artifacts in src_dist_dir will be copied to dist_dir | ||||||
| self.dist_dir = self.pkg_root / "dist" | ||||||
| self.build_log_path = self.pkg_root / "build.log" | ||||||
| self.build_args = build_args | ||||||
| self.force_rebuild = force_rebuild or continue_ | ||||||
| self.continue_ = continue_ | ||||||
|
|
||||||
| def cleanup_paths(self, *, include_dist: bool = False) -> list[Path]: | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Let's prefix with underscore as it is used internally only.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. I'll fix it. |
||||||
| """ | ||||||
| Return filesystem paths that should be removed to clean the build artifacts. | ||||||
|
|
||||||
| Parameters | ||||||
| ---------- | ||||||
| include_dist : bool | ||||||
| If True, include the dist directory in the cleanup paths. | ||||||
|
|
||||||
| Returns | ||||||
| ------- | ||||||
| list[Path] | ||||||
| List of paths to be removed during cleanup. | ||||||
| """ | ||||||
| paths: list[Path] = [self.build_dir, self.build_log_path] | ||||||
| if include_dist: | ||||||
| paths.append(self.dist_dir) | ||||||
| return paths | ||||||
|
|
||||||
| def clean(self, *, include_dist: bool = False) -> None: | ||||||
| """ | ||||||
| Clean build artifacts for this package. | ||||||
|
|
||||||
| Parameters | ||||||
| ---------- | ||||||
| include_dist : bool | ||||||
| If True, also remove the dist directory. | ||||||
| """ | ||||||
| for path in self.cleanup_paths(include_dist=include_dist): | ||||||
| try: | ||||||
| if path.is_dir(): | ||||||
| if path.exists(): | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
is_dir implies that the path exists.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. That was my oversight. I’ll fix it. |
||||||
| logger.info("Removing %s", str(path)) | ||||||
| shutil.rmtree(path, ignore_errors=True) | ||||||
| elif path.is_file(): | ||||||
| logger.info("Removing %s", str(path)) | ||||||
| path.unlink(missing_ok=True) | ||||||
| else: | ||||||
| logger.debug("Path does not exist: %s", str(path)) | ||||||
| except Exception as exc: | ||||||
| logger.debug("Failed to remove %s: %s", str(path), exc, exc_info=True) | ||||||
|
|
||||||
| @classmethod | ||||||
| def get_builder( | ||||||
| cls, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,10 @@ | |
| from collections.abc import Iterable | ||
| from pathlib import Path | ||
|
|
||
| from pyodide_build.build_env import BuildArgs | ||
| from pyodide_build.logger import logger | ||
| from pyodide_build.recipe import loader | ||
| from pyodide_build.recipe.builder import RecipeBuilder | ||
|
|
||
|
|
||
| def resolve_targets( | ||
|
|
@@ -27,27 +29,6 @@ def resolve_targets( | |
| return list(recipes.keys()) | ||
|
|
||
|
|
||
| def _locate_cleanup_paths_for_package( | ||
| recipe_dir: Path, | ||
| build_dir_base: Path, | ||
| package: str, | ||
| *, | ||
| include_dist: bool = False, | ||
| ) -> list[Path]: | ||
| """ | ||
| Locate filesystem paths to remove for a single package. | ||
| """ | ||
| paths: list[Path] = [] | ||
| # Per-package build directory | ||
| paths.append(build_dir_base / package / "build") | ||
| # Per-package build log | ||
| paths.append(recipe_dir / package / "build.log") | ||
| # Per-package dist directory (optional) | ||
| if include_dist: | ||
| paths.append(recipe_dir / package / "dist") | ||
| return paths | ||
|
|
||
|
|
||
| def _remove_path(path: Path) -> None: | ||
| """ | ||
| Remove a file or directory if it exists. Best-effort, ignore errors. | ||
|
|
@@ -85,17 +66,19 @@ def clean_recipes( | |
| if not recipe_dir.is_dir(): | ||
| raise FileNotFoundError(f"Recipe directory {recipe_dir} not found") | ||
|
|
||
| build_base = build_dir or recipe_dir | ||
| build_dir_base = build_dir or recipe_dir | ||
|
|
||
| selected = resolve_targets( | ||
| recipe_dir, targets, include_always_tag=include_always_tag | ||
| ) | ||
|
|
||
| for pkg in selected: | ||
| for path in _locate_cleanup_paths_for_package( | ||
| recipe_dir, build_base, pkg, include_dist=include_dist | ||
| ): | ||
| _remove_path(path) | ||
| builder = RecipeBuilder( | ||
| recipe_dir / pkg, | ||
| BuildArgs(), | ||
| build_dir_base=build_dir_base, | ||
| ) | ||
| builder.clean(include_dist=include_dist) | ||
|
|
||
| if include_dist and install_dir is not None: | ||
| _remove_path(install_dir) | ||
|
Comment on lines
83
to
84
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove these for now as we are not using it in our CLI. I don't want to keep the same function both here and in the recipe builder.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I'll remove it. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer not to add this staticmethod just to call it in our CLI. The recipe builder should take the minimum information as a parameter in its constructor, then calculate and store the information internally.
Unfortunately, our existing CLI might not be implemented in that way (we are passing a bunch of information in
build-recipesCLI as well), but I don't think this approach helps mitigate it.Instead, maybe let's calculate only the recipe directory as-is in the orignal code (using
search_pyodide_rootdirectly), and pass other information as None. Then the RecipeBuilder should be responsible for calculating those information internally.