deephaven / deephaven-core

Deephaven Community Core
Other
242 stars 78 forks source link

File explorer shows error under windows #5068

Open niloc132 opened 7 months ago

niloc132 commented 7 months ago

Description

Launching the web IDE on a server running on Windows (without WSL) will show an error in the File Explorer tab.

Steps to reproduce

  1. Start DHE on a windows machine, without docker or WSL. For example, to start from python, see https://deephaven.io/core/docs/tutorials/pip-install/ or https://deephaven.io/core/docs/how-to-guides/jupyter/, etc
  2. Launch the web IDE by navigating to http://localhost:10000/ide/ in a web browser
  3. Switch to the File Explorer tab

Expected results Files that are available in the notebook directory should be visible.

Actual results image

Additional details and attachments Reported slack thread https://deephavencommunity.slack.com/archives/C036HT73T43/p1705948352845709

Versions

mattrunyon commented 7 months ago

This could also be considered an engine issue. Depends on where we want to normalize the paths. The web UI expects unix paths right now

mofojed commented 7 months ago

Yea, I would like to abstract the Web UI away from the actual file system implementation. Why push this issue up to the client?

mofojed commented 6 months ago

@niloc132 Why push this up to the client? Web UI/client shouldn't care about the underlying system implementation.

niloc132 commented 6 months ago

Agreed - the client is hardcoded to require /s, and shouldn't be, and as a result only supports servers running on *nix. If a user wants to run on windows, we should support windows path/naming/etc rules, so they can rely on paths being consistent?

niloc132 commented 6 months ago

Or are you saying that the server should normalize all paths to something system agnostic (what, like old Mac OS : delimiters?), or never use any separator tokens, but just return list of path strings?

The server is transparent for the host, and doesn't rewrite what the server expects - just passes them to the client - why not have the client be equally transparent, rather than be opinionated (and this wrong from the user's perspective on at least one OS)?

niloc132 commented 5 months ago

Trying to work through client or server changes, and how that would serve the user, I'm backing up to "what is this for":

Getting more specific for "how do users interact with this today" - the service itself is very generically defined (both in gRPC and in the JS API) to be about storage, but effectively, this is about "notebooks and layouts" today:

(Side note: if you try to rename a file to include an illegal character, the save button will be disabled, and removing the illegal character will not re-enable the save button)

Fixing this from the server side would require normalizing away platform-specific separator characters (/ and \) into either just one character (I assume /?), or rebuild paths to be sequences of file names (e.g. replace "notebook/data/start_kafka.py" with ["notebook", "data", "start_kafka.py"]). The server then would need to also escape or replace the other possible separator chars to ensure that they are sent to the client in a way that the user would expect (on windows, if you had a file foo\bar/baz.txt, it must be sent to the client as "a file in foo is called bar/baz.txt", except the / char must be replaced with something else? vice versa on linux/mac).

Fixing this from the client would require the client being aware of the current system's separator string - this can be provided by changing client.configuration.list to include the key file.separator. Then, after configurations are loaded, the client can exclude that character from file names (though splitting on that character shouldn't be necessary for traversing the filesystem hierarchy? just substring the parent path plus one).

Both seem reasonable to me - the question mostly is "are users expected to know what system they are running on when writing notebooks, but expected to ignore that when naming them?" Either approach, I see an easy argument that the leading / character should be removed (or made non-required at least), but I don't care for assuming that everything is linux for some operations, but not for others (unless we expect other operations to normalize away OS as well...).

mofojed commented 5 months ago

I'm suggesting we canonicalize separators like many other APIs do when dealing with Windows files, e.g. .NET: https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#canonicalize-separators Even cmd accepts slashes now. In Windows cmd, if I enter cd ./foo\bar (mixing the two), it will convert the slash and go into ./foo/bar. Go clients will have a similar issue, in that they'd need to know the underlying OS path separator (so we'd need to add APIs for that, or APIs to build a path).

mofojed commented 5 months ago

More formally, I expect the Path to match a path defined in the URI spec: https://datatracker.ietf.org/doc/html/rfc3986#section-3.3

niloc132 commented 1 month ago

Accidentally closed, needs another commit to fully resolve it.