Change OS.Dir.user and add OS.Dir.expand_tilde

favonia commented 2 years ago

Changes to Os.Dir.user

On all operating systems, HOME is consulted first. (Previously, the passwd is consulted first on non-Windows.)
On Windows, USERPROFILE is consulted next.
The error message on Windows will be about USERPROFILE, while the message on other operating systems will be about HOME. (I don't feel strongly about this, but it looks reasonable to me.)

Questions about OS.Dir.expand_tile

~~Maybe it should be placed in OS.Path because the path to be expanded can be a path to a file, not a directory? But OS.Dir already depends on OS.Path.~~ Answer: it is fine.

Status

This PR only partially addresses #77 and #93: I did not implement the mechanism to fetch FOLDERID_Profile on Windows as hinted by @dra27 in #77, and I want other utility functions planned in #93.

dbuenzli commented 2 years ago

Hello @favonia sorry for not reacting to your messages in the issue tracker it seems I missed them.

What's the rationale behind introducing home as an alias to user ? I'd rather not have multiple name for the same functionality.

Regarding tilde expansion I'd rather leave that to shells. But if you want to add it please make it POSIX compliant to avoid surprises and future bug reports (i.e. handle the ~username/ case). Also I would rather call it expand_tilde it's clearer (having it in OS.Dir is fine).

favonia commented 2 years ago

What's the rationale behind introducing home as an alias to user ? I'd rather not have multiple name for the same functionality.

Part of the reason to have home is to match expand_home, and I guess that point is moot if we use expand_tilde.

Regarding tilde expansion I'd rather leave that to shells. But if you want to add it please make it POSIX compliant to avoid surprises and future bug reports (i.e. handle the ~username/ case). Also I would rather call it expand_tilde it's clearer (having it in OS.Dir is fine).

Sure. I can easily make it POSIX-compliant. I did the limited version only because I thought others would think the full version is overkill.

dbuenzli commented 2 years ago

I did the limited version only because I thought others would think the full version is overkill.

Well I don't want to have to handle the bug report ~username/ is not working :-)

favonia commented 2 years ago

~~The current code is wrong because it would expand ~ in C:~. And I just learned that the internal representation of Fpath.t is a string. Will update the code accordingly.~~ All fixed!

dbuenzli commented 2 years ago

Could please a Windows guru like @dra27 or @jonahbeckford confirm the high-level Windows logic makes sense (even if imperfect).

jonahbeckford commented 2 years ago

TLDR: My opinion: I would only use HOME on Windows if the currently executing program is linked to cygwin1.dll (the Unix compatibility layer used by Cygwin and MSYS2). Simply checking for the existence of HOME will break some Windows users, and will in fact break Diskuv OCaml Windows users. And since checking for cygwin1.dll is difficult to detect, just use USERPROFILE exclusively on Windows.

The prior code already seems to look at HOME, so OS.Dir.user was probably breaking some Windows use cases already.

In what you see below, with-dkml is the wrapper from Diskuv OCaml that Opam is configured to use ... among other things, it delegates Unix commands (sh, env, etc.) to MSYS2 on Windows.

# It can run Unix commands

C:\Users\beckf> with-dkml env | Select-String ^HOME=
HOME=/home/beckf

C:\Users\beckf> with-dkml env | Select-String ^USERPROFILE=
USERPROFILE=C:\Users\beckf

# And it can run native Windows commands

C:\Users\beckf> where.exe where.exe
C:\Windows\System32\where.exe

C:\Users\beckf> with-dkml where.exe where.exe
C:\Windows\System32\where.exe

Now let's say you compile a "native" Windows executable that uses OS.Dir.user. By "native" I mean:

you use the Cygwin's "MinGW" GCC compiler, the Visual Studio compiler, or a clang compiler
you do not use Cygwin's standard GCC compiler (which would link your executable to cygwin1.dll)

Did you notice that HOME was a Unix path? It is required to be present so that cygwin1.dll linked programs (ex. sh build.sh, rm -rf _build) can function. But only cygwin1.dll knows how to map /home/beckf to C:\Users\beckf!

The only way a native Windows executable could ever resolve /home/beckf is to see if cygpath.exe is in the PATH, and then spawn cygpath.exe -aw /home/beckf. I highly doubt you want your bos users to have to do that.

dbuenzli commented 2 years ago

TLDR: My opinion: I would only use HOME on Windows if the currently executing program is linked to cygwin1.dll (the Unix compatibility layer used by Cygwin and MSYS2).

Thanks Jonah! Doesn't that coincide with Sys.cygwin = true ? (I'm always extremely confused by all possible setups on the Windows side of things :-)

jonahbeckford commented 2 years ago

Doesn't that coincide with Sys.cygwin = true ?

I think that is true. From what I understand from https://github.com/ocaml/ocaml/blob/f150a85e8453a77243d9b7fba4c4459df6745b68/configure#L13370-L13403, Sys.cygwin = true means the OCaml runtime was compiled under the Cygwin standard GCC compiler (*-*-cygwin*). The Cygwin MinGW GCC compiler (*-*-mingw32) should give false. And the Visual Studio compiled Diskuv OCaml system gives false as well.

So Sys.cygwin = true is a good precondition for checking HOME on Windows.

favonia commented 2 years ago

@jonahbeckford Thanks for the insight! I don't have a Windows machine so I could only rely on others. To clarify, with Cygwins I believe Sys.os_type will be "Cygwin" (instead of "Win32") and it will use the same logic for other Unix-like systems (which checks HOME and then reads passwd if that fails). So, if I understand your suggestion correctly, when Sys.os_type is "Win32", we should just check USERPROFILE (or even better, querying FOLDERID_Profile). I will implement this now.

favonia commented 2 years ago

@jonahbeckford @dbuenzli Done, with documentation updated.

jonahbeckford commented 2 years ago

Yes, the high-level logic looks right to me.

favonia commented 1 year ago

@dbuenzli I got two ideas:

I added user_of in this PR to get the home directory of a user (by its login name).
Maybe we could add an OS-independent expand_tilde to the fpath library instead? It will take a function of type string option -> (t, [`Msg of string]) result to get the replacement. Here's the intended usage:
```
Fpath.expand_tilde (function None -> user () | Some login_name -> user_of login_name) p
```
It is essentially the expand_tilde_gen function in this PR, but I think the code can be further improved with access to the underlying string representation. I also found a different usage of this OS-independent expansion in the library I am currently writing.

dbuenzli commented 1 year ago

Not very fond of the idea. Most applications just want the user directory, multi-user environments are rather on the downhill. I prefer what was before, simple and easy to use.

favonia commented 1 year ago

Not very fond of the idea. Most applications just want the user directory, multi-user environments are rather on the downhill. I prefer what was before, simple and easy to use.

Okay! I turned back the clock.

dbuenzli / bos