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

Fix localpath handling for file-URLs on different OS, especially on Windows #7206

Merged
merged 6 commits into from
Nov 30, 2022

Conversation

christian-monch
Copy link
Contributor

@christian-monch christian-monch commented Nov 29, 2022

Fixes #7186

The current code base treats the path component of file-URLs not according to the conventions described in the relevant standard, i.e. RFC8089 in Appendix D.2 and Appendix E.2 (funnily enough Append E.2 defines some "nonstandard techniques for interacting with DOS- or Windows-like file paths and URIs").

As @mih pointed out, there is support for the conventions described in RFC8089 Appendix D.2 and RFC8089 Appendix E.2 in python, implemented in nturl2path, which is transparently used in urllib.request.url2pathname on Windows systems.

This PR uses urllib.request.url2pathname to convert the path of a file-URL to a local path. This changes the behavior of RI.localpath if an RI is an instance of URL, and the URL-scheme is file, In this case localpath will return a result from urllib.request.url2pathname. This means for example that the following holds:

  • On Posix-like systems: RI('file:/c:/Windows').localpath == '/c:/Windows'
  • On Windows-like systems: RI('file:/c:/Windows').localpath == 'C:\\Windows'

Note that the drive letter is converted to a canonical "upper case" representation.

This commit changes the semantics of RI.localpath
to return a path that is OS-specific, i.e. can be
used to access a local file
This commit introduces code to interpret the path
component of a URL correctly as local path, especially
to detect the drive-letter in Windows. This is
different from interpreting a non-URL path as local
path.
@christian-monch christian-monch added the semver-patch Increment the patch version when merged label Nov 29, 2022
datalad/support/network.py Outdated Show resolved Hide resolved
datalad/support/network.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Base: 88.89% // Head: 88.48% // Decreases project coverage by -0.41% ⚠️

Coverage data is based on head (2b11199) compared to base (c62151b).
Patch coverage: 95.83% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7206      +/-   ##
==========================================
- Coverage   88.89%   88.48%   -0.42%     
==========================================
  Files         355      355              
  Lines       46674    46686      +12     
  Branches     6351        0    -6351     
==========================================
- Hits        41493    41310     -183     
- Misses       5166     5376     +210     
+ Partials       15        0      -15     
Impacted Files Coverage Δ
datalad/support/network.py 87.31% <90.00%> (-2.87%) ⬇️
datalad/support/tests/test_network.py 100.00% <100.00%> (ø)
datalad/cmdline/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/interface/run_procedure.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/cmdline/helpers.py 0.00% <0.00%> (-78.27%) ⬇️
datalad/support/nda_.py 0.00% <0.00%> (-54.55%) ⬇️
datalad/__main__.py 39.65% <0.00%> (-29.32%) ⬇️
datalad/distribution/uninstall.py 63.26% <0.00%> (-28.58%) ⬇️
datalad/support/collections.py 84.37% <0.00%> (-9.38%) ⬇️
datalad/local/remove.py 89.55% <0.00%> (-7.47%) ⬇️
... and 25 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bpoldrack
Copy link
Member

Thx for attacking this, @christian-monch !

Just in case you are looking for more related stuff to iron out: There's support.network.get_local_file_urlspecial casing windows. It's more involved b/c it's also about what git vs git-annex can actually consume, but an educated look at it would be great!

@christian-monch
Copy link
Contributor Author

Just in case you are looking for more related stuff to iron out: There's support.network.get_local_file_urlspecial casing windows. It's more involved b/c it's also about what git vs git-annex can actually consume, but an educated look at it would be great!

Will take a look at it

@bpoldrack
Copy link
Member

Ok, cleaned up, all green - thank you @christian-monch!

@bpoldrack bpoldrack merged commit c868ae9 into datalad:maint Nov 30, 2022
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.17.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PathRI behavior incorrect/not helpful on Windows
3 participants