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

improve handling of file URIs #5892

Merged
merged 2 commits into from Feb 24, 2019

Conversation

@benoit-pierre
Copy link
Member

@benoit-pierre benoit-pierre commented Oct 17, 2018

  • 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
Copy link
Member

@cjerdonek cjerdonek left a comment

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?

Loading

tests/unit/test_download.py Outdated Show resolved Hide resolved
Loading
assert url_to_path('file://unc/as/path') == r'\\unc\as\path'
assert url_to_path('file:c:/path/to/file') == r'C:\path\to\file'
assert url_to_path('file://localhost/c:/tmp/file') == r'C:\tmp\file'
Copy link
Member

@cjerdonek cjerdonek Oct 30, 2018

Choose a reason for hiding this comment

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

See my other ordering comment.

Loading

Copy link
Member

@cjerdonek cjerdonek Oct 30, 2018

Choose a reason for hiding this comment

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

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.

Loading

@xavfernandez xavfernandez reopened this Feb 11, 2019
@xavfernandez
Copy link
Member

@xavfernandez xavfernandez commented Feb 11, 2019

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

Loading

@cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Feb 11, 2019

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

Loading

@xavfernandez xavfernandez force-pushed the improve_handling_of_file_uris branch from fe7584b to 5cd50d2 Feb 11, 2019
Copy link
Member

@cjerdonek cjerdonek left a comment

Some additional comments.

Loading

tests/unit/test_download.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_download.py Outdated Show resolved Hide resolved
Loading
src/pip/_internal/download.py Outdated Show resolved Hide resolved
Loading
@xavfernandez xavfernandez force-pushed the improve_handling_of_file_uris branch 3 times, most recently from b1e2ed0 to bb5c83f Feb 11, 2019
@xavfernandez
Copy link
Member

@xavfernandez xavfernandez commented Feb 12, 2019

@cjerdonek I think everything should now be fine ?

Loading

@cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Feb 12, 2019

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.

Loading

@xavfernandez xavfernandez force-pushed the improve_handling_of_file_uris branch 2 times, most recently from b079a47 to b482b4b Feb 12, 2019
@xavfernandez xavfernandez force-pushed the improve_handling_of_file_uris branch from bfdfdd6 to 8556b7f Feb 12, 2019
@xavfernandez
Copy link
Member

@xavfernandez xavfernandez commented Feb 12, 2019

ping @cjerdonek :)

Loading

@cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Feb 12, 2019

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)?

Loading

Copy link
Member

@cjerdonek cjerdonek left a comment

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

Loading

@cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Feb 13, 2019

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."

Loading

@cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Feb 13, 2019

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

Loading

@xavfernandez
Copy link
Member

@xavfernandez xavfernandez commented Feb 24, 2019

@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 :)

Loading

@xavfernandez xavfernandez merged commit c286c55 into pypa:master Feb 24, 2019
22 checks passed
Loading
@xavfernandez
Copy link
Member

@xavfernandez xavfernandez commented Feb 24, 2019

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

Loading

@benoit-pierre benoit-pierre deleted the improve_handling_of_file_uris branch Feb 24, 2019
@pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Feb 26, 2019

Thanks everyone who worked on this!

Loading

opencontrail-ci-admin pushed a commit to Juniper/contrail-controller that referenced this issue May 16, 2019
Currently Opserver UT is failing in R3.2 branch because of following issues.
1. In latest release of pip, url_to_path handling has changed via pypa/pip#5892.
   This is breaking Opserver UT in R3.2. But It works in latest master.
   Fix - Make R3.2 opserver/Sconsript same as master.

2. Cassandra 2.1.9 is not spawning by mockcassandra.
   Fix - This cassandra version is affected by https://issues.apache.org/jira/browse/CASSANDRA-11661.
   Upgrade cassandra version to 2.2.12. This is same as cassandra version running on 3.2.13 deployments.
   Mockcassandra is using pycassa to connect to cassandra on thrift port. So, set start_rpc TRUE in cassandra yaml.

Change-Id: I0d62d6a6487f284cc56bb904da6d5ba5ba191833
closes-jira-bug: CEM-5615
@lock
Copy link

@lock lock bot commented May 28, 2019

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.

Loading

@lock lock bot added the S: auto-locked label May 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants