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

Improve handling of Windows paths #762

Open
talex5 opened this issue Oct 4, 2024 · 3 comments
Open

Improve handling of Windows paths #762

talex5 opened this issue Oct 4, 2024 · 3 comments
Labels
api API design decision windows

Comments

@talex5
Copy link
Collaborator

talex5 commented Oct 4, 2024

In #124, @samwgoldman said:

For Flow, we have the need to share paths between platforms. These are relative paths, using the / path separator. We only support Windows systems that transparently support the / path separator, so we can just pass these paths into Windows APIs without transforming them.

However, Eio also needs to support absolute paths on Windows and it seems this doesn't work (see https://discuss.ocaml.org/t/how-to-specify-a-full-windows-path-in-eio/14880). It would be good to investigate why.

@dra27 commented:

I must confess that introducing yet another notation for a Windows path doesn’t look at all sensible to me! /C:/path is simply invalid, and if that’s ever printed (error messages, exceptions, etc.) it kinda just looks like Eio doesn’t know what it’s doing.

As mentioned in the original issue:

The Fpath library encodes lots of knowledge about Windows paths. It probably can't be used directly because its paths behave differently depending on the host platform (preventing e.g. a program running on Linux from creating paths that are to be used on Windows). The path rules should probably be per-filesystem instead.

I think we should give up on a uniform representation of paths and instead move some operations (e.g. split) to Fs.Pi.DIR, so that each file-system can provide its own implementation.

@talex5 talex5 added api API design decision windows labels Oct 4, 2024
@talex5 talex5 mentioned this issue Oct 4, 2024
9 tasks
@RichardChukwu
Copy link

Thanks for bringing up this issue, @talex5

I agree that we need to improve how Eio handles Windows paths. Right now, using relative paths with the / separator works for some cases, but we have issues with absolute paths on Windows that need fixing.

Things to Consider:

  1. Path Format: I understand @dra27's point about not wanting to add another way to represent Windows paths. We want to keep things clear, and a single format can help avoid confusion, especially in error messages.

  2. Filesystem-Specific Solutions: It might be a good idea to let each filesystem handle its own path operations. By moving functions like splitting paths to Fs.Pi.DIR, we can keep the API clean and make it easier to support different systems.

  3. Using the Fpath Library: We should look into the Fpath library, but we need to make sure it fits our needs and doesn’t create more issues with path handling across different platforms.

Next Steps:
Investigation: I’ll start looking into how paths are currently managed in Eio and identify what’s causing problems on Windows.

Community Input: I’d love to hear any ideas from others on how we can solve this.

I’m looking forward to working together to improve Windows path support in Eio!

@samwgoldman
Copy link

Apologies for the misleading feedback I shared. Absolute paths never came up in Flow (in practice, all paths are relative to the process's working directory) and I wasn't aware that Windows APIs don't support / for absolute paths.

@talex5
Copy link
Collaborator Author

talex5 commented Oct 8, 2024

@samwgoldman not misleading at all; it was very useful to know we could ship 1.0 like that. I just tagged you in case you objected to a change of direction, but I think it won't affect you either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API design decision windows
Projects
None yet
Development

No branches or pull requests

3 participants