Skip to content

Conversation

@idanmiara
Copy link
Contributor

@idanmiara idanmiara commented Mar 29, 2021

Overview

Resolved all the issues that prevented the tests to pass on Windows.

Contribution Agreement

(as per https://github.com/geopython/pywps/blob/master/CONTRIBUTING.rst#contributions-and-licensing)

  • I'd like to contribute [feature X|bugfix Y|docs|something else] to PyWPS. I confirm that my contributions to PyWPS will be compatible with the PyWPS license guidelines at the time of contribution.
  • I have already previously agreed to the PyWPS Contributions and Licensing Guidelines

@idanmiara idanmiara force-pushed the pywps-4.4-win branch 4 times, most recently from cf0b381 to 9f9e073 Compare March 29, 2021 21:30
@idanmiara
Copy link
Contributor Author

Not sure why 3.6 failed and 3.7 got canceled...

@idanmiara
Copy link
Contributor Author

@cehbrecht Could you review please?

Copy link
Collaborator

@Zeitsperre Zeitsperre left a comment

Choose a reason for hiding this comment

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

Barring a few changes, looks good. I leave @cehbrecht to do the approval.

def uri_to_path(uri) -> str:
p = urlparse(uri)
path = p.path
if os.name == 'nt':
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might not be enough to determine if the OS/Platform is actually Windows. I would leverage platform.system() for a few windows-like keys. See: https://stackoverflow.com/a/54837707/7322852

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might not be enough to determine if the OS/Platform is actually Windows. I would leverage platform.system() for a few windows-like keys. See: https://stackoverflow.com/a/54837707/7322852

I looked at the post, I don't mind to follow your recommendation, but I couldn't find any system (windows-like or not) that have os.name != 'nt' and platform.system() == 'windows'

Copy link
Collaborator

Choose a reason for hiding this comment

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

I bring it up here since depending on the subsystem that the script is running in (msys2, cygwin, WSL2, etc.), this check might not return what we intend it to. There's also a possibility that the pathlib.Path function handles the file path changes in a better way (see: https://docs.python.org/3/library/pathlib.html#pathlib.PureWindowsPath) e.g.:

from pathlib import Path, PureWindowsPath, PurePosixPath
p = Path("D/path/to/a/file")
p.as_posix()
Out[5]: 'D/path/to/a/file'
str(PureWindowsPath(p))
Out[9]: 'D\\path\\to\\a\\file'
PurePosixPath(PureWindowsPath(p))
Out[15]: PurePosixPath('D/path/to/a/file')

To be fair, I haven't done much in Windows. Just presenting some options.

Copy link
Contributor Author

@idanmiara idanmiara Mar 31, 2021

Choose a reason for hiding this comment

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

Thanks for bringing this up.
Actually I've used it here, but the only thing I couldn't figure out is how to get a proper WindowsPath from a file uri:

> p = r'c:\abc.txt'
> wp = WindowsPath(p)
> wp
WindowsPath('c:/abc.txt')
> wp.as_uri()
'file:///c:/abc.txt'
> from urllib.parse import urlparse
> parse = urlparse(uri)
> parse.path
'/c:/abc.txt'
> Path(parse.path)
WindowsPath('/c:/abc.txt')

How can I get rid of this leading slash without actually trimming it if ran on windows ?

@idanmiara
Copy link
Contributor Author

Barring a few changes, looks good. I leave @cehbrecht to do the approval.

Thanks for your review!

@Zeitsperre
Copy link
Collaborator

PyWPS seems to be failing for Python3.6. Looking into this now.

@idanmiara idanmiara force-pushed the pywps-4.4-win branch 4 times, most recently from 0214ee0 to 29c787c Compare March 31, 2021 19:57
@idanmiara
Copy link
Contributor Author

Finally it's ready!

@coveralls
Copy link

coveralls commented Apr 1, 2021

Coverage Status

Coverage remained the same at 0.0% when pulling 1577cfa on talos-gis:pywps-4.4-win into b9fca6e on geopython:pywps-4.4.

@Zeitsperre Zeitsperre self-requested a review April 6, 2021 15:03
@cehbrecht
Copy link
Collaborator

I'm confused by the diff ... why is tests/processes/metalinkprocess.py shown as deleted and then added?

@Zeitsperre
Copy link
Collaborator

Zeitsperre commented Apr 7, 2021

Oh! It's because the example process was originally in the docs and then was referred to within the processes. How strange! Good catch. This is a much better approach.

@idanmiara
Copy link
Contributor Author

idanmiara commented Apr 7, 2021

I'm confused by the diff ... why is tests/processes/metalinkprocess.py shown as deleted and then added?

As @Zeitsperre wrote, there was a symlink between the file at the docs to the source dir.
Symlinks are not compatible with Windows, so I've moved the file to the source dir and referenced it from the docs instead of a symlink.
I've tried to do it in one commit but git does not change the status of the file from symlink to regular file, so I ended up removing the symlink and then moving the file and update the rst reference

Copy link
Collaborator

@cehbrecht cehbrecht left a comment

Choose a reason for hiding this comment

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

Only checked on linux ... it still works :)

@cehbrecht
Copy link
Collaborator

@idanmiara @Zeitsperre I might use pywps.util instead of pywps.uri ... I leave the vote to you :) I will merge then.

@idanmiara
Copy link
Contributor Author

@idanmiara @Zeitsperre I might use pywps.util instead of pywps.uri

done.
Is the CI broken now ?

@Zeitsperre
Copy link
Collaborator

Zeitsperre commented Apr 8, 2021

@idanmiara It is but not because of you. The fix is quite simple if you want to add it. In .github/workflows/main.yml:

sudo apt-get -y install gdal-bin libgdal-dev libnetcdf-dev libhdf5-dev

Should be changed to:

sudo apt-get update && sudo apt-get -y install gdal-bin libgdal-dev libnetcdf-dev libhdf5-dev

I don't think either of us has access to the GithHub action. @cehbrecht ?

@cehbrecht
Copy link
Collaborator

@idanmiara It is but not because of you. The fix is quite simple if you want to add it. In .github/workflows/main.yml:

sudo apt-get -y install gdal-bin libgdal-dev libnetcdf-dev libhdf5-dev

Should be changed to:

sudo apt-get update && sudo apt-get -y install gdal-bin libgdal-dev libnetcdf-dev libhdf5-dev

I don't think either of us has access to the GithHub action. @cehbrecht ?

you can update it in this PR changing the workflow:
https://github.com/geopython/pywps/blob/pywps-4.4/.github/workflows/main.yml

@Zeitsperre
Copy link
Collaborator

@idanmiara It is but not because of you. The fix is quite simple if you want to add it. In .github/workflows/main.yml:

sudo apt-get -y install gdal-bin libgdal-dev libnetcdf-dev libhdf5-dev

Should be changed to:

sudo apt-get update && sudo apt-get -y install gdal-bin libgdal-dev libnetcdf-dev libhdf5-dev

I don't think either of us has access to the GithHub action. @cehbrecht ?

you can update it in this PR changing the workflow:
https://github.com/geopython/pywps/blob/pywps-4.4/.github/workflows/main.yml

Of course! I was looking at the master and got very confused as to why I couldn't find it. I forget that we don't base directly off master for this project.

@idanmiara
Copy link
Contributor Author

@idanmiara It is but not because of you. The fix is quite simple if you want to add it. In .github/workflows/main.yml:

sudo apt-get -y install gdal-bin libgdal-dev libnetcdf-dev libhdf5-dev

Should be changed to:

sudo apt-get update && sudo apt-get -y install gdal-bin libgdal-dev libnetcdf-dev libhdf5-dev

I don't think either of us has access to the GithHub action. @cehbrecht ?

you can update it in this PR changing the workflow:
https://github.com/geopython/pywps/blob/pywps-4.4/.github/workflows/main.yml

Of course! I was looking at the master and got very confused as to why I couldn't find it. I forget that we don't base directly off master for this project.

done, now it works :)

@cehbrecht cehbrecht merged commit 5f486f1 into geopython:pywps-4.4 Apr 12, 2021
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.

4 participants