-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Vendor pypa/packaging
into micropip
#178
Vendor pypa/packaging
into micropip
#178
Conversation
Thanks for working on this @agriyakhetarpal! Please ping me when it is ready to be reviewed. |
pypa/packaging
for micropip
pypa/packaging
into micropip
[[tool.mypy.overrides]] | ||
module = "micropip._vendored.*" | ||
warn_unreachable = false |
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've had to keep this as there is some unreachable code in src/packaging/
, which is not of concern for us, and TIL that the warn_unreachable
option is still raised for excluded files.
Thanks, @ryanking13! I think it is ready now. |
@@ -7,13 +7,12 @@ | |||
import warnings | |||
from typing import Any, Dict, List, Optional, Union, Literal, TypeAlias, TypedDict | |||
|
|||
import packaging.utils | |||
import micropip._vendored.packaging.utils as packaging_utils |
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.
(No need to address in this PR)
both _vendored
and externals
directory vendors external packages, so I think we can merge them into a single directory
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.
Thanks! Yes, I noticed that. I'll open a follow-up PR after this to merge them. I have no preference on the name of the folder, but it should be _externals
or _vendored
, so that it doesn't form a part of the public API.
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.
Thanks, I think _vendored
is a better name. At the beginning, the external
directory contained subset of pip
, and it had a _vendor
directory in it (ref).
MANIFEST.in
Outdated
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.
As packaging
is a part of the private API for micropip
, we don't need to graft
it. However, there is no nice way to exclude specific files without explicitly naming them. This is because MANIFEST.in
is processed from top to bottom, like an interpreted language, so we can't exclude the entire first and include only what we want.
The better way would be for us to switch to hatchling
in a follow-up item, which supports this kind of use case much better.
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.
Thanks! Yes, I agree that in terms of managing file inclusion/exclusion, other build backends like hatchling are better than setuptools. It can be a follow up item.
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've opened #182 for this. Thanks!
Ready for another look! |
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.
Thanks @agriyakhetarpal! Could you please update the changelog and make a release?
MANIFEST.in
Outdated
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.
Thanks! Yes, I agree that in terms of managing file inclusion/exclusion, other build backends like hatchling are better than setuptools. It can be a follow up item.
@@ -7,13 +7,12 @@ | |||
import warnings | |||
from typing import Any, Dict, List, Optional, Union, Literal, TypeAlias, TypedDict | |||
|
|||
import packaging.utils | |||
import micropip._vendored.packaging.utils as packaging_utils |
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.
Thanks, I think _vendored
is a better name. At the beginning, the external
directory contained subset of pip
, and it had a _vendor
directory in it (ref).
Thanks for the review, Gyeongjae! I'll merge this and move the |
Description
Closes #176, and stemmed off from pyodide/pyodide#5366.
This PR vendors the https://github.com/pypa/packaging project into
micropip
, as the issue above noted that it is difficult to install a newer version ofpackaging
into the environment, asmicropip
depends on it and installs it. This meanspackaging
can now be installed separately frommicropip
.Changes
from packaging import <...>
imports intomicropip._vendored.packaging.src.packaging import <...>
packaging
files are present in both the source distribution and the wheel, excluding non-package-related files, and including the relevant license files.