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

Add windows support for fs #137

Closed
Xuanwo opened this issue Mar 10, 2022 · 5 comments
Closed

Add windows support for fs #137

Xuanwo opened this issue Mar 10, 2022 · 5 comments
Assignees

Comments

@Xuanwo
Copy link
Member

Xuanwo commented Mar 10, 2022

Make opendal works on windows.

Only fs needs some changes and tests. Other services should work now.

@Xuanwo Xuanwo changed the title Add windows support Add windows support for fs Sep 28, 2022
@baszalmstra
Copy link
Contributor

baszalmstra commented Dec 20, 2022

Hi! I just noticed that windows pathnames seem completely invalid. normalize_path seems to expect a unix style file path. This will always add a / in front of the root path. This makes it hard to use on Windows at all.

I would be happy to contribute to making this work on Windows. However, I would like a little bit of direction before I start doing that.

I think the biggest issue is that the normalize_root function doesn't work on Windows.

  • Is this a correct assumption?
  • How would you like to see this work on Windows?
  • Why did you opt not to use the Path type which already handles a lot of this out of the box? (Including network drives etc)

@Xuanwo
Copy link
Member Author

Xuanwo commented Dec 21, 2022

I would be happy to contribute to making this work on Windows.

Thank you so much for your attention and participation. ❤️

  • Is this a correct assumption?

No, normalize_root only works well on UNIX platforms. That's why we have this issue.

  • How would you like to see this work on Windows?

normalize_root should consider C:/ or D:/.

  • On windows platforms, fs's root must start will a valid disk name like C:/, D:/.
  • All \ should be replaced by /. So the final root will be C:/path/to/root/.
  • Object path should represent in path/to/file instead of path\\to\\file to ensure it's always the same on different platforms.
  • Why did you opt not to use the Path type which already handles a lot of this out of the box? (Including network drives etc)

Because OpenDAL needs to work well with different services and make sure they work like the same one. And on storage services like s3, gcs, file a/b/c is different from file a\b\c. So we can't depend on Path to build the request path.

And only local fs will be affected by Path construction, we don't need to introduce it for object storage services.

@baszalmstra
Copy link
Contributor

I understand why OpenDAL doesn't use Path throughout the library. But I think that for the fs backend specifically, it would make more sense to use it. Only in the internals of that backend (and maybe in the Builder::root?). The std::fs::* functions already all use Path anyway. That way you can deal with all the different weirdness of different filesystems paths out of the box.

So the user still uses String but this is converted to a Path in the fs backend. Paths returned by fs backend are converted from Path to normalized strings again. WDYT?

@Xuanwo
Copy link
Member Author

Xuanwo commented Dec 24, 2022

So the user still uses String but this is converted to a Path in the fs backend. Paths returned by fs backend are converted from Path to normalized strings again. WDYT?

Sounds good to me. Let's have a try.

@Xuanwo
Copy link
Member Author

Xuanwo commented Feb 11, 2023

Thanks @baszalmstra, this issue has been fixed.

@Xuanwo Xuanwo closed this as completed Feb 11, 2023
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

No branches or pull requests

2 participants