Skip to content

Fixed wheel choice by interpreter version tag - #34

Merged
hoodmane merged 1 commit into
pyodide:mainfrom
mentalisttraceur:main
Dec 10, 2022
Merged

Fixed wheel choice by interpreter version tag#34
hoodmane merged 1 commit into
pyodide:mainfrom
mentalisttraceur:main

Conversation

@mentalisttraceur

Copy link
Copy Markdown
Contributor

Fixes #32 .

There seem to be unrelated test failures already in the automated test suite, but those failures are the same both before and after my patch, and the tests I added (and the one test I changed to factor out some now-reused code) are all passing.

@mentalisttraceur

mentalisttraceur commented Dec 10, 2022

Copy link
Copy Markdown
Contributor Author

Local testing was a struggle. I couldn't find any documentation on it, but between the various errors and tracebacks and the definition of the GitHub PR testing action, I figured out that I needed to

  1. create a dist directory in the repo root,
  2. download and unpack a Pyodide release into it (so it should look like ./dist/pyodide/...), then
  3. create a conftest.py with a DIST_PATH which is a pathlib object that points to that dist folder, and finally
  4. run pytest with --dist-dir=./dist/.

That got most of the tests running locally.

Eight of the tests I couldn't get running (and five of those are also broken in the github action, and are failing in PR #33 as well). They seem orthogonal to this patch and fail the same way both before and after my patch.

@mentalisttraceur

mentalisttraceur commented Dec 10, 2022

Copy link
Copy Markdown
Contributor Author

Since tests/test_micropip.py wasn't already using hypothesis, and since it would take much longer, I resisted the temptation to pull it in and write tests with Hypothesis strategies for generating creative variations of wheel versions and tags.

Maybe that's a PR for a later time!

@hoodmane

Copy link
Copy Markdown
Member

Hypothesis strategies for generating creative variations of wheel versions and tags.

Sounds great!

@rth rth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @mentalisttraceur for the fix and for writing tests!

Yes, we should definitely add contributing docs.

A couple of minor comments otherwise LGTM. Could you please merge upstream/main in; the CI issues were fixed in #35

Comment thread micropip/_micropip.py
Comment thread CHANGELOG.md
Comment thread CHANGELOG.md Outdated
Comment thread micropip/_micropip.py
@mentalisttraceur

Copy link
Copy Markdown
Contributor Author

Cheers! Requested changes made and tests passing. Let me know if I missed anything else. Thanks for the guidance, info, and taking care of your side of things so quickly!

@hoodmane

Copy link
Copy Markdown
Member

Thanks @mentalisttraceur!

@hoodmane
hoodmane merged commit 3c07775 into pyodide:main Dec 10, 2022
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.

micropip.install is mishandling wheel tags

3 participants