Add a CLI command to create a zipfile of Python libraries #3411
Conversation
ryanking13
changed the title
ryanking13
changed the title
There was a problem hiding this comment.
Thanks @ryanking13 ! A few minor comments otherwise LGTM.
For the name maybe pyodide create-zipfile since that's the name of the Python stdlib module? Do we generally expect users to use this? If not maybe we shouldn't make it part of the public CLI API?
Also needs a changelog.
pyodide-build/pyodide_build/pyzip.py
Outdated
| Bundle Python standard libraries into a zip file. | ||
|
|
||
| The basic idea of this function is similar to the standard library's | ||
| [PyZipFile](https://github.com/python/cpython/blob/59665d0280c2299ea87e9af45cedc90656cb6f55/Lib/zipfile/__init__.py#L2011). |
There was a problem hiding this comment.
{ref}`zipfile.PyZipFile`
should work I think once we enable stdlib sphinx inter-linking in https://github.com/pyodide/pyodide/pull/3405/files#diff-85933aa74a2d66c3e4dcdf7a9ad8397f5a7971080d34ef1108296a7c6b69e7e3R74
| temp_dir = Path(temp_dir_str) | ||
| shutil.copytree(libdir, temp_dir, ignore=filterfunc, dirs_exist_ok=True) | ||
|
|
||
| shutil.make_archive(str(output), "zip", temp_dir) |
There was a problem hiding this comment.
Yeah it does, though I think it is not a very well documented behavior. (https://github.com/python/cpython/blob/31b639a99281bd9582e88137a3be2868c4154a88/Lib/shutil.py#L978)
pyodide-build/pyodide_build/pyzip.py
Outdated
| libdir : Path or str | ||
| Path to the directory containing the Python standard library. | ||
|
|
||
| output : Path or str, optional |
There was a problem hiding this comment.
Also now that #3405 is merged, you should be able to remove the types in docstrings and have them auto-generated from type annotations.
|
Thanks for the thorough review @rth!
I think users will not use this directly, but I am thinking of using this CLI in our makefile. Probably we can hide it from the help message and docs? |
| @@ -127,3 +132,52 @@ def test_config_get(cfg_name, env_var): | |||
| ) | |||
|
|
|||
| assert result.stdout.strip() == common.get_make_flag(env_var) | |||
|
|
|||
|
|
|||
| def test_fetch_or_build_pypi(selenium, tmp_path): | |||
There was a problem hiding this comment.
There is already a test named the same way on main. I wonder if it's badly merging with main in CI and we end up with 2 identically named tests one of which would be ignored. This would explain why coverage says it's not run in CI.
Could you merge the main in, please to see if it would help?
There was a problem hiding this comment.
Merged main to resolve conflict. Otherwise, nevermind my comment, I did see the function test on main locally, but not after syncing with upstream. Still unclear why codecov doesn't see coverage here, but merging anyway.
There was a problem hiding this comment.
Yeah, this test was moved to test_pypi.py in #3310. I think I made a mistake while merging main.
Description
This PR is a split-off from #3166.
This adds
pyodide create-zip(open to other names) CLI command into pyodide-build,which bundles a directory into a zipfile, so it can be imported.
Note that this is not part of our build system yet, I'll make a follow up PR after #3252 and #3253 are merged.
Checklists