Add helper function for recipe clean up by teddygood · Pull Request #223 · pyodide/pyodide-build

teddygood · 2025-08-10T13:38:09Z

Closes #218

Summary

Adds clean subcommand to pyodide build-recipes for removing downloaded sources and intermediate build artifacts.
Preserves dist/ by default; can include with --include-dist.
Supports package names, tags (tag:<name>), or all packages (--all).
No dependency traversal.

Changes

Implement clean command with:
- Default: remove <pkg>/build and <pkg>/build.log.
- --include-dist: also remove <pkg>/dist and global install dir.
Target resolution via existing loader.
Added CLI tests (test_clean.py).

Usage

# Clean all (excludes dist/)
pyodide build-recipes clean

# Clean selected targets
pyodide build-recipes clean numpy tag:core

# Include dist/
pyodide build-recipes clean --include-dist

# Custom directories
pyodide build-recipes clean \
  --recipe-dir=PATH_TO_RECIPES \
  --build-dir=PATH_TO_BUILD \
  --install-dir=PATH_TO_DIST \
  PKG1 PKG2

for more information, see https://pre-commit.ci

ryanking13

Thanks for working on this @teddygood. I left some comments.

Because the CLI design would need some discussion, I think we can separate this PR. How about

first implementing some helper functions to clean up the recipe build artifacts,
then connect a CLI entrypoints to those functions.

While working on (1), you can probably also refactor some of our code to make it reusable. For instance, locating the log file or build directories are already implemented in recipe package, and hardcoding those directories are quite easy to break.

ryanking13 · 2025-08-12T08:40:23Z

pyodide_build/cli/build_recipes.py

 from pyodide_build import build_env
 from pyodide_build.build_env import BuildArgs, init_environment
 from pyodide_build.common import get_num_cores
+from pyodide_build.errors import ActionableError


There is no definition for ActionableError; maybe you forgot to define it, or the hallucination of AI code generators generated it?

Thanks for catching that. This commit was my mistake. I was originally working on issue #218, and after finishing that, I was addressing issue #220 but accidentally committed it to the wrong branch without checking out properly.

ryanking13 · 2025-08-12T08:48:58Z

pyodide_build/cli/build_recipes.py

+# Register subcommands to the Typer app
+app.command("no-deps")(build_recipes_no_deps)
+app.command("build")(build_recipes)


Well, I would like to avoid creating more subcommands.

It was my bad that in the original issue (#218), I suggested adding a clean subcommand under the build-recipes. As there can be (at actually is) a Python package named clean, I think having a subcommand clean under the build-recipes can be confusing.

Instead, maybe let's define it as a top-level subcommand, so that we can extend the clean command itself.

pyodide clean # clean all build artifacts pyodide clean recipes # clean only recipe-related artifacts pyodide clean <something else>

@hoodmane @agriyakhetarpal any thoughts on the CLI design?

What you are suggesting sounds good to me.

Yes, having the possibility to extend pyodide clean SGTM, too.

ryanking13 · 2025-08-12T08:52:55Z

pyodide_build/cli/build_recipes.py

+        # Legacy usage: `pyodide build-recipes` without subcommand will build all packages
+        build_recipes(
+            [],  # Build all packages when no subcommand is specified


This will be a breaking change, and I would like to avoid that. As I commented in the above comment, let's not introduce another subcommand.

I’ll drop this commit and remove the subcommand-related changes from this PR so the current behavior stays the same.

ryanking13 · 2025-08-12T08:58:04Z

pyodide_build/cli/build_recipes.py

+
+
+@app.command("clean")
+def clean(


To make it easier to write unittests (and also to reduce the dependency on the CLI package) let's split the functions.

The CLI function should only be responsible for handling arguments and filling out the default values.

Then, it should call the actual function that does the cleanup process.

The function that does the clean up can also be separated to multiple small functions that can be tested separately.

I’ll split the CLI from the cleanup logic. The CLI will only parse args/defaults and call small, testable helpers (e.g., resolve targets, remove build/log, optional dist, optional global dist). Unit tests will focus on the helpers.

teddygood · 2025-08-13T07:36:33Z

Thanks for the feedback! To proceed, here’s a small plan - please let me know if this looks good:

Keep this PR limited to helper functions + unit tests (no CLI changes).
- Reuse existing recipe utilities (no hardcoded paths).
- Default: preserve dist; add an option in helpers to include dist when needed.
- No dependency traversal; don’t implicitly include the “always” tag.
Drop the unrelated commit (9673975) and remove subcommand/callback changes so current behavior stays the same.
In a follow-up PR, wire the helpers to a top-level CLI:
- pyodide clean / pyodide clean recipes [<pkg>|tag:<name>]
- --include-dist to opt in removing per-package and global install dist.

If this direction sounds good, I’ll push the updates for this PR accordingly. Thanks!

ryanking13 · 2025-08-13T12:30:36Z

Thanks for summarizing the plans.

Reuse existing recipe utilities (no hardcoded paths).

Yes, sounds good. Actually, some paths might be hardcoded in recipe classes as well, but I would like to avoid adding more of hardcoded paths. If you think you can refactor some code to make it more reusable, feel free to go ahead and refactor it. But please do not mix refactoring and feature implementation, unless the change is very trivial. Try to do it step-by-step.

Default: preserve dist; add an option in helpers to include dist when needed.

Yes. Note that there are two dist directories, one for global dist directory that stores all the wheels and another for storing local wheels per package.

No dependency traversal; don’t implicitly include the “always” tag.

This part can be a matter of choice. We can change the decision at any time later. So when implementing the cleanup feature, this part should be easy to modify.

In a follow-up PR, wire the helpers to a top-level CLI:

Yes, sounds good.

--include-dist to opt in removing per-package and global install dist.

Maybe something like --all would be better. Users do not need to know about the name dist. Anyway, the name can be changed easily, so let's discuss the name in a separate PR.

for more information, see https://pre-commit.ci

ryanking13 · 2025-08-16T00:51:16Z

pyodide_build/tests/test_clean.py

Let's remove this file in this PR

Thanks. I forgot to remove it. I’ll remove it in this PR.

ryanking13 · 2025-08-16T00:52:07Z

pyodide_build/recipe/cleanup.py

@@ -0,0 +1,118 @@
+from __future__ import annotations


I don't think this is required. Was there some issue with type annotations without this?

Thanks. There was no issue with type annotations. I added it out of habit. I’ll remove it.

ryanking13 · 2025-08-16T01:00:01Z

pyodide_build/recipe/cleanup.py

+    return list(recipes.keys())
+
+
+def remove_package_build(recipe_dir: Path, build_dir_base: Path, package: str) -> bool:


All the remove_... functions are doing the similar thing with duplicate codes, so it would be nice to refactor the functions to be more general.

I think we can have two functions that:

Removes any file or directory given as a parameter

Locate a set of files to remove

Then, the function 1 would have a more simple signature, and can be used in a more general purposes.

If we have more files to delete in the future, we can update function 2 to include those files.

Implementing as suggested I will introduce _remove_path(path) to delete files or directories and _locate_cleanup_paths_for_package(...) to compute the paths to remove then update clean_recipes(...) to use these helpers and remove the duplicated remove_* functions If we need to delete more artifacts later we will only update the locator function.

ryanking13 · 2025-08-16T01:01:00Z

pyodide_build/recipe/cleanup.py

+    """
+    Clean recipe build artifacts and optionally dist directories.
+
+    Returns the number of items removed.


I am not sure if this function should return the number of items removed. It doesn't seem very useful to me.

Agreed. I will remove the return value and make the function void, since the count is not particularly useful and no callers rely on it.

ryanking13 · 2025-08-16T01:01:53Z

pyodide_build/recipe/cleanup.py

+def perform_recipe_cleanup(
+    *,


I think we can make recipe_dir and targets as positional parameters, instead of making everything keyword-only parameters.

I will make recipe_dir and targets positional and keep the rest keyword-only in clean_recipes.

ryanking13 · 2025-08-16T01:02:59Z

pyodide_build/tests/test_cleanup_helpers.py

+        _make_pkg_with_artifacts(pkg)
+
+    removed = perform_recipe_cleanup(
+        recipe_dir=RECIPE_DIR,


You don't need to use a real recipe directory for this. I think you can utilize tmp_path.

I think to avoid using a real recipe directory we should use pytest tmp_path to create an isolated temporary recipe directory and generate minimal meta.yaml files so that the loader can discover packages Is this approach okay for these tests?

Yes, I think it is okay.

ryanking13 · 2025-08-16T01:03:49Z

pyodide_build/recipe/cleanup.py

+    return False
+
+
+def perform_recipe_cleanup(


naming: How about simply clean_recipes?

Makes sense. I will rename it to clean_recipes.

…tions

…n signature

for more information, see https://pre-commit.ci

ryanking13

Thanks! I left some minor comments, but it looks good overall.

ryanking13 · 2025-08-18T10:15:27Z

pyodide_build/recipe/cleanup.py

+    """
+    try:
+        if path.is_dir():
+            import shutil


(friendly question) Is there a specific reason you wanted to import shutil dynamically? If it were to optimize the speed, I think we don't need to care much about the import speed in this case.

Otherwise, let's move this import statement to the top of the file.

Thanks for pointing this out. I recently started using Cursor and didn’t review the import carefully, sorry about that. You’re right that there’s no need to import it dynamically, I’ll move it to the top as suggested.

ryanking13 · 2025-08-18T10:17:17Z

pyodide_build/recipe/cleanup.py

+            logger.info("Removing %s", str(path))
+            path.unlink(missing_ok=True)  # type: ignore[call-arg]
+        else:
+            # Path does not exist; nothing to do


Maybe let's print a debug message in this case. So we can debug the issue when it does not work well.

Thanks, that makes sense. I’ll add a debug log for this case so it’s easier to trace when nothing happens.

ryanking13 · 2025-08-18T10:17:31Z

pyodide_build/recipe/cleanup.py

+            # Path does not exist; nothing to do
+            return
+    except Exception:
+        # Best-effort cleanup; ignore failures


Sure, I’ll add a debug log here too.

ryanking13 · 2025-08-18T10:24:27Z

pyodide_build/recipe/cleanup.py

+    # 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")


(No action required for this PR)

As I mentioned in the previous comment (#223 (review)), I think this part can be improved to reuse other existing code (or to be more object-oriented).

For instance, we might be able to do something like

builder = RecipeBuilder(recipe_dir, ...) return [builder.build_dir, builder.log_file, ...]

or maybe the RecipeBuilder class can be responsible for cleaning up the files itself.

builder = RecipeBuilder(recipe_dir, ...) builder.clean()

Of course, our RecipeBuilder class does not implement such attributes or methods yet, so you don't need to address it in this PR.

If you are interested, feel free to refactor it in a follow-up PR.

Sure, I’ll work on this refactor in a follow-up PR. Thanks for the suggestion.

ryanking13 · 2025-08-18T10:28:25Z

pyodide_build/tests/test_cleanup_helpers.py

I think this can be renamed to test_cleanup now.

Sure, I’ll rename it.

for more information, see https://pre-commit.ci

ryanking13

Thanks! I updated the PR title as the content of the PR changed. Otherwise looks good to me.

teddygood and others added 6 commits August 10, 2025 21:57

Add clean subcommand to build-recipes CLI

0ff23e2

Switch entry point to build-recipes:app

7f61073

Add tests for build-recipes clean

9fde664

[pre-commit.ci] auto fixes from pre-commit.com hooks

c453bcd

for more information, see https://pre-commit.ci

Fix import paths: use relative imports for internal modules

9673975

[pre-commit.ci] auto fixes from pre-commit.com hooks

8969128

for more information, see https://pre-commit.ci

ryanking13 reviewed Aug 12, 2025

View reviewed changes

teddygood and others added 9 commits August 14, 2025 16:45

Revert pre-commit auto fixes

5442ed9

Revert relative import changes in build_recipes

8332565

Restore build-recipes entry point to function

6c447c9

Remove Typer app and clean subcommand from build_recipes

7a3a66f

Add recipe cleanup helpers

4527508

Add tests for cleanup helpers

3dce42b

[pre-commit.ci] auto fixes from pre-commit.com hooks

de5fba3

for more information, see https://pre-commit.ci

Fix build_recipes syntax

0b490b8

Format build_recipes

f7b0fb1

ryanking13 reviewed Aug 16, 2025

View reviewed changes

teddygood and others added 4 commits August 16, 2025 17:19

Remove unused cleanup CLI test and drop from __future__ import annota…

b625470

…tions

Refactor recipe cleanup generalize removal logic and simplify functio…

5d4d09a

…n signature

Update tests for clean_recipes

ba7f1b5

[pre-commit.ci] auto fixes from pre-commit.com hooks

74de8b6

for more information, see https://pre-commit.ci

ryanking13 reviewed Aug 18, 2025

View reviewed changes

teddygood and others added 5 commits August 21, 2025 22:18

Add debug logs and move import to top

9c8c5b2

[pre-commit.ci] auto fixes from pre-commit.com hooks

370332d

for more information, see https://pre-commit.ci

Add debug logs and move import to top

9357d5c

Rename cleanup tests

53bcfa3

[pre-commit.ci] auto fixes from pre-commit.com hooks

8b2e4b5

for more information, see https://pre-commit.ci

ryanking13 changed the title ~~Add clean subcommand to pyodide build-recipes CLI~~ Add helper function for recipe clean up Aug 22, 2025

ryanking13 approved these changes Aug 22, 2025

View reviewed changes

ryanking13 merged commit 48a980d into pyodide:main Aug 22, 2025
6 checks passed

teddygood mentioned this pull request Nov 4, 2025

Add CLI command pyodide clean recipes #254

Merged

teddygood mentioned this pull request Nov 24, 2025

Refactor cleanup to use RecipeBuilder methods #260

Merged

		return list(recipes.keys())


		def remove_package_build(recipe_dir: Path, build_dir_base: Path, package: str) -> bool:

Conversation

teddygood commented Aug 10, 2025

Summary

Changes

Usage

Uh oh!

ryanking13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teddygood commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanking13 commented Aug 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teddygood Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanking13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

teddygood commented Aug 13, 2025 •

edited

Loading

teddygood Aug 16, 2025 •

edited

Loading