Skip to content
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

Remove vendored pip and add Metadata class #87

Merged
merged 12 commits into from
Oct 5, 2023

Conversation

ryanking13
Copy link
Member

@ryanking13 ryanking13 commented Sep 22, 2023

This PR is a preparation to support PEP 658: Serve Distribution Metadata in the Simple Repository API.

In order to support PEP 658, we need a class that can parse the Distribution Metadata file. For now, this it is implemented inside the vendored pip.

The vendored pip code contains a bunch of codes that we are not using anymore, and it does not expose an API for parsing a single metadata file. So this PR removes the vendored pip, and instead adds a class Metadata that handles parsing the metadata file and calculating requirements.

@ryanking13 ryanking13 changed the title pip and add Metadata class Remove vendored pip and add Metadata class Sep 22, 2023
"""Unsupported wheel."""


def wheel_dist_info_dir(source: ZipFile, name: str) -> str:
Copy link
Member Author

Choose a reason for hiding this comment

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

I think there is not much reason to import this from pyodide. I move it to metadata.py.

@rth
Copy link
Member

rth commented Sep 22, 2023

re, and it does not expose an API for parsing a single metadata file

Thanks that sounds reasonable. I'm just wondering whether we could more explicitly separate what comes from micropip and what is a class that you added. So that say it would be easier to diff against upstream changes in the future if we decide to sync with changes to those functions in pip. Unless it's a loose adaptation and essentially an independent code base.

@ryanking13
Copy link
Member Author

Unless it's a loose adaptation and essentially an independent code base.

I think this is kind of the way this PR is implemented. I did some copy-paste, but also did some rewrite as well. So if you are okay I would like to merge this as is. Maybe I'll open a follow-up PR that moves methods that come from pip without any modification to the separate file so we can easily distinguish them.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks! A couple of comments otherwise LGTM.

@@ -0,0 +1,158 @@
Metadata-Version: 2.1
Copy link
Member

Choose a reason for hiding this comment

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

Can you gzip test data so they don't produce noise when grepping please?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I will.

_requires: list[Requirement] | None = None # List of requirements.

# Note: `_project_name`` is taken from the wheel metadata, while `name` is taken from the wheel filename or metadata of the package index.
# They are mostly the same, but can be different in some weird cases (e.g. a user manually renaming the wheel file), so just to be safe we store both.
_project_name: str | None = None # Project name.
Copy link
Member

Choose a reason for hiding this comment

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

So we would no longer get the name from the wheel metadata. then? I feel it's a safer source than the filename.

Copy link
Member Author

Choose a reason for hiding this comment

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

no longer get the name from the wheel metadata. then?

Interestingly, we didn't calculate the name from the metadata even before.

The project_name was calculated by just escaping the name that was passed by the caller.

self.project_name = safe_name(project_name or "Unknown")

I moved this part to __post_init__ method. So the behavior is not changed in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll open a follow-up PR to parse the package name from the metadata file.

@ryanking13 ryanking13 merged commit f975747 into pyodide:main Oct 5, 2023
6 checks passed
@ryanking13 ryanking13 deleted the metadata-class branch October 5, 2023 11:38
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