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

Cross-platform path handling #47

Merged
merged 2 commits into from
Sep 7, 2020
Merged

Cross-platform path handling #47

merged 2 commits into from
Sep 7, 2020

Conversation

joshmoore
Copy link
Member

Removes the zarr_path variable from BaseZarrLocation, using a
pathlib.Path field in LocalZarrLocation and a URL string in
RemoteZarrLocation to properly handle the creation of subpaths across
multiple platforms. In general, use of os.path and especially
os.path.join should be avoided in favor of delegating to methods on the
zarr location objects.

Removes the `zarr_path` variable from BaseZarrLocation, using a
pathlib.Path field in LocalZarrLocation and a URL string in
RemoteZarrLocation to properly handle the creation of subpaths across
multiple platforms. In general, use of os.path and especially
os.path.join should be avoided in favor of delegating to methods on the
zarr location objects.
@joshmoore
Copy link
Member Author

Whew. Finally. So a summary of steps taken and lessons learned to get the build passing:

  • extra git clone steps (at least of gl-ci-helpers on Windows) fail regularly.
  • action/checkout can't be used separately -- it will delete the previous check.
  • Qt on Windows and Ubuntu are going to take more work.
    But ultimately, I'm fairly confident that GH actions are now properly testing the the cross-platform support.

@tlambert03
Copy link
Contributor

Qt on Windows and Ubuntu are going to take more work.
But ultimately, I'm fairly confident that GH actions are now properly testing the the cross-platform support.

not sure if it will be useful to you... but I have a branch of napari working with GH actions on all platforms (there may of course be other OMERO specific stuff that still need to be figured out): https://github.com/tlambert03/napari/blob/feature/github-tests/.github/workflows/tests.yml

@joshmoore
Copy link
Member Author

Thanks, @tlambert03. I'll review the diff and see if I can get the Qt working. 👍

GIT_TRACE_PACKET: 1
run: >
git clone --depth 1 git://github.com/vtkiorg/gl-ci-helpers.git ||
git clone --depth 1 git://github.com/vtkiorg/gl-ci-helpers.git
Copy link
Member

Choose a reason for hiding this comment

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

It makes sense that these two lines clone the same thing right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the || is the important bit. GHA doesn't currently have "retry" functionality.

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

I have close to little experience with pathlib but from a quick reading, it looks like it fulfills the Windows/Unix path support and has the benefit of being a core Python library.

Tests are passing, the code rewriting looks generally sensible although I haven't looked at the details.

A side question is whether we would foresee switching to pathlib in omero-py as a replacement for the path.py fork?

@joshmoore
Copy link
Member Author

A side question is whether we would foresee switching to pathlib in omero-py as a replacement for the path.py fork?

I could definitely see that. Happy for this to be something of a test run.

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