Skip to content

Refactor: Move setupPythonSearchPath to its own file#6709

Merged
hoodmane merged 1 commit intomainfrom
hoodmane/setup-search-path-own-file
May 1, 2026
Merged

Refactor: Move setupPythonSearchPath to its own file#6709
hoodmane merged 1 commit intomainfrom
hoodmane/setup-search-path-own-file

Conversation

@hoodmane
Copy link
Copy Markdown
Contributor

This function is long enough that I think it's more readable without having the code inline in python.ts

@hoodmane hoodmane requested review from a team as code owners April 30, 2026 22:52
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 30, 2026

Now let me carefully analyze the changes for any issues:

  1. loadPythonMod in util.ts -- The moduleName is interpolated into a Python string with '${moduleName}'. All callers pass string literals ('setup_python_search_path', 'introspection'), so no injection concern in practice. However, if a future caller passed a name with a single quote, it would break/inject. This is internal-only code though, so this is low-priority.

  2. Cleanup concern -- In the old code, setup.destroy() destroyed the callable. In the new code, mod.destroy() destroys the module. But mod.setup_python_search_path (the callable attribute) is not explicitly destroyed. Let me check whether the introspection module has the same pattern:

Looking at the introspection usage in python-entrypoint-helper.ts, the IntrospectionMod is stored long-term and never destroyed -- its methods are called repeatedly. So there's no inconsistency there.

For setupPythonSearchPath, the old code destroyed just the callable. The new code destroys the module. Destroying the module should release the module and all its attributes, so this seems correct -- but let me verify there's no leak of the callable reference.

Looking at python.ts:96-100:

mod.setup_python_search_path.callKwargs({...});
mod.destroy();

The mod.setup_python_search_path is accessed as a property, used inline, and never stored. mod.destroy() destroys the module proxy. This should be fine -- the callable is a sub-reference of the module, and destroying the module should handle it.

  1. Behavioral change in python-entrypoint-helper.ts: The old code used await import('pyodide-internal:introspection.py') (dynamic import), and the new code uses a static import import { default as introspectionSource } from 'pyodide-internal:introspection.py'. This changes the module from being lazily loaded to eagerly loaded at module parse time. This is a minor behavioral shift but shouldn't matter since the module is always needed anyway.

  2. getIntrospectionMod was previously async because of the dynamic import, and it remains async because of getPyodide() -- this is correct.

Overall this is a clean refactor. The Python logic is identical (same code moved to a .py file), the loadPythonMod utility correctly deduplicates the module-loading boilerplate, the types are declared properly, and the BUILD system picks up the new .py file via glob.

LGTM

github run

This function is long enough that I think it's more readable without having the
code inline in python.ts
@hoodmane hoodmane force-pushed the hoodmane/setup-search-path-own-file branch from d3146bc to 7600483 Compare May 1, 2026 00:00
Copy link
Copy Markdown
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but see question below.

Comment thread src/pyodide/internal/setup_python_search_path.py
@hoodmane hoodmane merged commit a28c303 into main May 1, 2026
27 checks passed
@hoodmane hoodmane deleted the hoodmane/setup-search-path-own-file branch May 1, 2026 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants