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

feat: use std::path::Path for fs backend #1100

Merged
merged 6 commits into from
Feb 7, 2023

Conversation

baszalmstra
Copy link
Contributor

@baszalmstra baszalmstra commented Dec 24, 2022

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

As discussed in #137 this PR changes the internals of the fs backend to use std::path::Path. This enables using all the path types that std::path::Path supports like Windows disk drive prefixes and network drives.

This is my first contribution! Im looking forward to your feedback!

src/services/fs/dir_stream.rs Outdated Show resolved Hide resolved
src/services/fs/dir_stream.rs Outdated Show resolved Hide resolved
src/services/fs/dir_stream.rs Outdated Show resolved Hide resolved
src/services/fs/backend.rs Show resolved Hide resolved
src/services/fs/backend.rs Outdated Show resolved Hide resolved
@baszalmstra
Copy link
Contributor Author

The unit_oli test also seems to fail. Additionally, when no root is specified the fs backend was previously by default rooted at /. However, that doesn't exist on windows. What would be a good alternative?

@Xuanwo
Copy link
Member

Xuanwo commented Jan 3, 2023

when no root is specified the fs backend was previously by default rooted at /.

On windows, fs backend must specify a root-like C:/. We can implement different test for windows.

@Xuanwo
Copy link
Member

Xuanwo commented Jan 26, 2023

Hi, in PR #1221, I make opendal's test buildable on windows.

@wolfv
Copy link

wolfv commented Jan 26, 2023

Nice @Xuanwo !

@baszalmstra
Copy link
Contributor Author

Awesome thanks! I will integrate!

@baszalmstra
Copy link
Contributor Author

I redid the whole thing from scratch. I got all opendal's tests working (at least locally).

The only thing I don't know how to solve exactly is using an empty root for the Fs backend. I currently implemented that this will use the current directory because that's consistent on all platforms. Previously this would default to / but on windows, no such directory actually exists and there is also no sensible default. Would it be C:\? Should it be the drive of the current directory? You also can't use an "empty" root because C:\bla.txt is not a valid path for opendal.

Curious about what you think!

@Xuanwo
Copy link
Member

Xuanwo commented Feb 6, 2023

I currently implemented that this will use the current directory because that's consistent on all platforms.

LGTM!

@Xuanwo
Copy link
Member

Xuanwo commented Feb 6, 2023

This PR looks nice now, please update with main branch and make our CI passed on windows platform (fs local test)

@Xuanwo
Copy link
Member

Xuanwo commented Feb 7, 2023

Thanks a lot!

@Xuanwo Xuanwo merged commit 86a5e34 into apache:main Feb 7, 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

Successfully merging this pull request may close these issues.

3 participants