improve handling of file URIs

[benoit-pierre]

• correctly handle file://localhost/... URIs, as per RFC 8089: file://localhost/some/path is equivalent to file:///some/path
• don't try to use UNC paths on Unix

[cjerdonek]

I added some review comments.

Also, can the problem this PR is fixing be seen from the CLI? I think it would probably be good to have at least one higher-level regression test, especially since this function is a core function used throughout the code base. For that reason, I'm surprised it doesn't cause more problems. What must one do to hit this issue in real life?

[cjerdonek]

See my other ordering comment.

[cjerdonek]

I also think it would be nice if the test cases that are supposed to run on all platforms are broken out into a third test. That way there will be less repetition of test cases, and it's easier to see which cases are the same on both platforms.

[xavfernandez]

@cjerdonek I think the change seems sound and the tests could be improved in an other PR.

[cjerdonek]

My preference would be for the review comments I made to be addressed with the PR.

[cjerdonek]

Some additional comments.

[xavfernandez]

@cjerdonek I think everything should now be fine ?

[cjerdonek]

Okay, thanks, @xavfernandez. I appreciate it. Now that I can see the tests more clearly, I have one more comment / question.

For each file URI that appears in our test cases, shouldn't we have a test for it on both Windows and non-Windows? The reason I ask is that it doesn't seem like we can guarantee that a given file URI will only be seen on a given platform, so it seems like pip should be resilient in the face of such input, or at least we should have an understanding of / be able to see how it behaves in that case (even if that behavior is to error out). In other words, I agree the test expectation should be platform-dependent, but it seems like the test inputs should be run on both platforms. One way to do that would be to have a single test with parameters (url, win_expected, non_win_expected), perhaps with a special value to signal ValueError. There are other ways, of course, but that's one way to make it easy to confirm that each url is being run on both platforms and will continue to be going forward.

[xavfernandez]

ping @cjerdonek :)

[cjerdonek]

Thanks, looks great!

By the way, I was just pinged about #6247 a couple hours ago. Do you know how this PR will affect that issue -- it looks like this means we won't be accepting on Unix systems what that poster is calling a relative file URI (will raise ValueError judging by the test cases)?

[cjerdonek]

Approving assuming you're okay with the implications for #6247. Thanks for picking this up, @xavfernandez!

[cjerdonek]

For future reference, this seems related to this issue #783.

Out of curiosity, does this PR solve an actual end-user issue, or is it more a matter of purity? The reason I ask is that I don't see an issue number referenced or description of a concrete use case. For example, even #783's latest comment says that it "seems to indicate that this is working now."

[cjerdonek]

(Btw, you should probably wait until after 19.0.3 is released before merging.)

[xavfernandez]

@cjerdonek it was more a matter of purity :)

But FWIW it means that:

$ pip install file://localhost/tmp/some_wheel-6.5-py2.py3-none-any.whl --no-index
Processing ./\\localhost/tmp/some_wheel-6.5-py2.py3-none-any.whl
Could not install packages due to an EnvironmentError: [Errno 2] No such file or directory: '/tmp/\\\\localhost//tmp/some_wheel-6.5-py2.py3-none-any.whl'`

would now work :)

[xavfernandez]

Thanks @benoit-pierre for the initial PR and thanks @cjerdonek for the thorough review :)

[pradyunsg]

Thanks everyone who worked on this!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.