Changed condition for package name to be considered a url to endswith(".whl")

[hoodmane]

Resolves #867.
Some minor updates to loading_packages / micropip docs.
Docs are still confusing but I think that's not worth working on too much
until we deal with #870

[rth]

Thanks @hoodmane ! A few comments below

[rth]

Also please add an entry to the docs/changelog.md file with a link to this PR.

[hoodmane]

It would be good to add a test on selenium that actually loads a package from a wheel at a relative url. I wasn't sure how to do this though: I would probably want to take a trivial "hello world" wheel, but I have to get the wheel onto the url somehow. Even for documentation purposes it would be nice to have a test file that loads the same package using each method.

[hoodmane]

Oh I see I can just use a tiny modification of the snowball stemmer test.

[rth]

Please ping me when those tests are added.

[hoodmane]

@rth Okay I added a couple of tests. One just tests add_requirement without selenium and it works, the other does a full test of micropip.install("./snowballstemmer-2.0.0-py2.py3-none-any.whl"). The second one doesn't work, but I've been having trouble debugging it because inside the promise of micropip.install writes to standard out seem to get thrown away. Or something.

I guess I should take this as encouragement to try more monkeypatching to test more than add-requirement. I found that to be surprisingly difficult when I tried it a few days ago.

[hoodmane]

Okay I added another selenium-free test that monkey patches out micropip._get_url. Unfortunately it passes too.

[rth]

I think the failure for test_install_custom_relative_url is to be expected because the relative path is not correct. It is ./snowballstemmer.. when using the web_server_tst_data fixture (that serves files in src/tests/data/. However the selenium_standalone / selenium fixtures serve files in './build' so unless you temporarily copy the wheel there (and make sure it's later deleted including on test failure) that won't work. Since relative paths higher than the root dir for the webserver are disallowed.

I agree those errors are hard to debug, and I think we should try to use pytest-httpserver more (#823). However that won't help here, because we still serve packages for tests with a SimpleHTTPRequestHandler

Edit: once test_install_custom_relative_url passes, I think we can remove test_relative_url_2 (unless you want to spend more time investigating why it fails).

[rth]

Suggested change

	def test_relative_url_2():
	def test_relative_url():

[dalcde]

What is the status of this PR?

[hoodmane]

I think the HTTP request is set up in way that does not handle relative urls correctly. I don't know why, I had a hard time diagnosing it. Maybe if I came back to it now I would have an easier time with it since I'm more familiar with the tools available.

While we're at it, could you weigh in on #870?

[dalcde]

As rth mentioned, the easiest solution would be to symlink src/tests/data/snow... to build/ at the beginning of the test, and remove it afterwards.

[dalcde]

In case you missed it I submitted a PR to your branch that fixes the tests

[hoodmane]

Did not notice, thanks.

[dalcde]

I don't think it matters much in this case but would be good to merge in master too

[hoodmane]

@rth @dalcde I guess this is ready to merge now?

[rth]

Thanks @hoodmane ! LGTM.

Just one last thing, could you please add a changelog entry to 0.17.0 (might need to merge master in to see that section).

[hoodmane]

Isn't it traditional to call the changelog section [unreleased] and only change the name to a specific version number as part of the release process? This is at least the semver recommended approach.

[rth]

Indeed feel free to change it. In general we should probably follow https://keepachangelog.com/en/1.0.0/ better.