monorepo icon indicating copy to clipboard operation
monorepo copied to clipboard

replace hardcoded posix paths with platform agnostic solution

Open samuelstroschein opened this issue 7 months ago • 10 comments

Problem

Hardcoded posix paths in the SDK lead to bugs on windows. Likely the reason for #2070

https://github.com/opral/monorepo/blob/b4d952e770d3838f43f693eae0b674b1b2946fac/inlang/source-code/sdk/src/migrations/maybeCreateFirstProjectId.ts#L19-L20

https://github.com/opral/monorepo/blob/b4d952e770d3838f43f693eae0b674b1b2946fac/inlang/source-code/sdk/src/migrations/maybeCreateFirstProjectId.ts#L27-L28

https://github.com/opral/monorepo/blob/b4d952e770d3838f43f693eae0b674b1b2946fac/inlang/source-code/sdk/src/loadProject.ts#L129-L130

https://github.com/opral/monorepo/blob/b4d952e770d3838f43f693eae0b674b1b2946fac/inlang/source-code/sdk/src/loadProject.ts#L144-L145

Proposal

Use normalizePath from lix-fs to ensure Windows compatibility.

samuelstroschein avatar Jan 22 '24 10:01 samuelstroschein

@samuelstroschein i finally got access to our windows vm and i cannot confirm this causes any issues, project ids work as expected on windows even with those paths. even native powershell has no issues with this: Screenshot 2024-01-22 at 22 43 50

janfjohannes avatar Jan 22 '24 20:01 janfjohannes

of course we do not support the old command shell but nearly all current software does not support that anymore.

janfjohannes avatar Jan 22 '24 20:01 janfjohannes

cc @jldec @felixhaeberle

janfjohannes avatar Jan 22 '24 20:01 janfjohannes

@janfjohannes Why the cc? Is there something to validate from my side? I constantly hit myself in the foot handling different kind of paths in VSC to an extend where I simply slap normalizePath to every path I get.

This is surprisingly effective. 😄 But def not elegant.

for example: https://github.com/opral/monorepo/blob/870143a220b092162536f3e1d296703c9dbb2159/inlang/source-code/ide-extension/src/utilities/fs/createFileSystemMapper.ts#L10-L38

felixhaeberle avatar Jan 22 '24 22:01 felixhaeberle

@janfjohannes - i expect that you're correct that the code mentioned in the issue where we concatenate paths like /project_id are probably not the root cause of the windows bugs.

What @felixhaeberle mentioned looks like a good suggestion because it could help avoid scenarios where we mix normalized and non-normalized paths. From my observations earlier today, this is the reason for at least one of the windows failures:

loadProject() calls normalize() before calling maybeCreateFirstProjectId(). This does not work as expected in Windows because the nodeishFs.stat can't find the normalized Windows path and thinks the .inlang directory passed in does not exist.

(i just confirmed that no project id is auto-generated in this windows test)

Screenshot 2024-01-22 at 22 23 51

Another possible vector for these bugs may be confusion (perhaps just on my part) around when to use "absolute" paths vs. relative, and where the paths are rooted (repo-root or fs-root or project-root or cwd)

jldec avatar Jan 22 '24 22:01 jldec

I'd like to create a new test for windows similar to this one to test the auto-project-id generator.

@janfjohannes is there an easy way to create a (windows equivalent) snapshot like the one used in that test?

jldec avatar Jan 22 '24 23:01 jldec

@jldec no all that tooling is posix only.

@felixhaeberle just pinged you as you have most windows experience, but also i dont 100% undestand what the windows issues are you experience with vscode extension if windows supprts unix file delimiters and the cli worked in my tests.

janfjohannes avatar Jan 23 '24 13:01 janfjohannes

@janfjohannes

if windows supports unix file delimiters

Probably that's right for windows, but the VSC extension gives platform specific path delimiters. This causes, when not correctly resolved through path.resolve or simply used without preprocessing through normalizePath, various issues down the road with the SDK. I can't name them all because most of the time I try to avoid them.

Am I correct that you say it should normally "just work" with not using normalizePath at all?

felixhaeberle avatar Jan 23 '24 14:01 felixhaeberle

The good news is that I can debug loadProject in windows

The ~bad~ other good news is that I can't repro a failure to create the project ID based on my theory from debugging the test above.

Using nodeishFs = node:fs/promises on Windows, a normalilzed path can be passed to nodeishFs.stat and it will find the path (@janfjohannes is right, both forward and backslash work).

I think the problem in the test from yesterday was that (unlike node) memoryFS doesn't handle windows paths.

Screenshot 2024-01-23 at 15 50 33

jldec avatar Jan 23 '24 16:01 jldec

@felixhaeberle i see, i think i need to get into the vscode extension more to fully understand our requirements there. not saying anything about normalizePath that does more than just touching delimiters

janfjohannes avatar Jan 24 '24 10:01 janfjohannes