progrium / env86

Embeddable v86 virtual machines
MIT License
35 stars 1 forks source link

Paths are broken on Windows #6

Open gysddn opened 1 day ago

gysddn commented 1 day ago

Go fs.FS functions expect a path argument with slashes when referencing within a file system. The problem is, on Windows, path/filepath returns OS native paths, resulting Invalid argument error in many places.

env86 pull github.com/progrium/alpine

will result:

2024/11/27 20:32:51 main.go:69: stat C:\Users\user\.env86\github.com\progrium\alpine\latest: invalid argument

Here, the one reason, unrelated, is the incorrect root. It should be C:\\ on Windows and that prefix should be trimmed properly. But the more important problem is that even if those things are right, the call to fs.Exists will be:

fsys: fs.FS (C:\)
path: Users\user\.env86\github.com\progrium\alpine\latest

Which is not right.

It's a simple fix for this function, but path/filepath is used throughout this project and in toolkit-go. There will be a lot of places where paths passes will raise errors.

Another example is in image.go. Same problem.

progrium commented 1 day ago

I figured it was something like this. And just in case it's not clear, fs.FS etc are not io/fs but our wrapped version in toolkit-go. But ok, let's break this down.

First, globalImage assumes Unix-style conventions using a .env86 in the user home directory. This should be explicitly changed to also support the Windows equivalent of a directory in the %AppData% directory when on Windows.

I'm pretty sure path (non-Windows paths) and path/filepath (supports Windows paths) functions are used all over in both places when working with filesystems, and also code that assumes forward slashes (not using filepath.Separator) outside of those (sometimes used with them).

Can we make sure filepath and filepath.Separator are used in env86 and then in toolkit-go only in codepaths needed by env86, and then file an issue in toolkit-go for the rest.

gysddn commented 1 day ago

Can we make sure filepath and filepath.Separator are used in env86 and then in toolkit-go only in codepaths needed by env86, and then file an issue in toolkit-go for the rest.

Just to confirm here, I am to replace instances of filepath with path while calculating relative paths within a file system, correct?

I figured it was something like this. And just in case it's not clear, fs.FS etc are not io/fs but our wrapped version in toolkit-go. But ok, let's break this down.

Yeah sorry, I meant io/fs.

progrium commented 23 hours ago

Just to confirm here, I am to replace instances of filepath with path while calculating relative paths within a file system, correct?

Maybe you meant the reverse? Replacing instance of path with filepath when working with the filesystem will ensure portability to Windows.

gysddn commented 22 hours ago

Yes. But the issue here is that, on Windows these paths are passes to io/fs functions which expect unix style relative paths with forward slashes under a given fs, no matter the platfrom. I probably didn't phrase it in the best way but, the main issue here is that Windows native paths with backslashes cause Invalid argument errors on fs functions.

filepath usage, as you said ensures portability everywhere, i.e. os.DirFS. Except io/fs which is what causing these problems on Windows, so my proposal was to track down only the places where forward slashes were expected and calculate those using path.

progrium commented 22 hours ago

Ah, you are right, io/fs only works with forward slashes and I just learned here that Windows works with forward slashes so it seems we shouldn't use filepath if we're using io/fs. So okay, what you said makes sense.

Also, just to make sure I was clear before, in most places where io/fs would be used, we actually use toolkit-go/engine/fs which wraps io/fs by using type aliases. So sometimes it might be io/fs code and sometimes it would be our code. If we use filepath in toolkit-go we should use path there as well.