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

Ensure correct url format from local files #271

Closed
wants to merge 1 commit into from
Closed

Ensure correct url format from local files #271

wants to merge 1 commit into from

Conversation

volaya
Copy link
Contributor

@volaya volaya commented Mar 3, 2021

Not sure if others are having the same problem or I am missing something, but without this fix all tests fail in Windows.

The reason for it is in here:

https://github.com/stac-utils/pystac/blob/develop/pystac/stac_io.py#L19

The urlopen function does not understand a windows path such as c:\folder\file.ext, and raises an exception.

fixed to avoid issues with test paths
@@ -521,6 +522,7 @@ def from_file(cls, href):
"""
if not is_absolute_href(href):
href = make_absolute_href(href)
href = f"file:{pathname2url(href)}"
Copy link
Member

Choose a reason for hiding this comment

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

This breaks tests - perhaps a better approach would be to put this line directly in the STAC_IO method you point to in the body of the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try to work out a different way of doing this. In my case, all test pass with this fix, so I guess the behaviour in the CI and in my machine (or any windows machine, for that matter), might be different.

@lossyrob
Copy link
Member

lossyrob commented Mar 3, 2021

Hm, we've had issues with Windows paths in the past, but I thought we got past that. I don't believe this is broken for all cases of windows paths; maybe it's possible to put up a unit test that shows the problem, even when running on non-windows OS's? We use ntpath to test in unit tests, although there's an issue around using a different technique which isn't implemented yet.

@volaya
Copy link
Contributor Author

volaya commented Mar 3, 2021

I don't think that npath is the problem or affects this. The issue is with the path starting with the drive name and a colon (C:). With that, urlopen (in the line that i pointed out) understands that the drive name is the protocol of the url, and raises the exception. So the paths are correctly formed, that works fine, but without the "file://" prefix urlopen does not work.

@lossyrob
Copy link
Member

lossyrob commented Mar 3, 2021

Gotcha. Perhaps the case with a drive prefix can be handled prior to the https://github.com/stac-utils/pystac/blob/develop/pystac/stac_io.py#L19 line instead of how it's currently implemented to avoid the unit test failure.

@duckontheweb
Copy link
Contributor

@volaya I got the Windows CI builds running properly in #361 and noticed this issue as well. I solved it there by using pystac.utils._urlparse instead of urllib.parse.urlparse, since that method handles the Windows schemes. See d4e93ac for the change.

There still appear to be a number of other path-related issues on Windows, so I opened up #362 to track fixing the rest of those.

@duckontheweb duckontheweb mentioned this pull request Jun 1, 2021
4 tasks
@duckontheweb
Copy link
Contributor

@volaya I'm going to close this, as it's superseded by #361. Feel free to open an issue if you continued to see problems with any of the tests on Windows.

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.

3 participants