ocamlmerlin.c: chdir to root also on Windows to continue working if cwd is deleted

[nojb]

On Windows, the process ocamlmerlin-server.exe is launched with the directory of the current source file as current working directory. This causes problems if one tries to move or delete that directory.

To avoid this problem, this PR makes sure to chdir to a different directory (TMPDIR) before launching ocamlmerlin-server.exe. Note that the same logic already exists for Unix:

merlin/src/frontend/ocamlmerlin/ocamlmerlin.c

Lines 314 to 317 in f1b877c

	/* Change directory to root, so that process still works if directory
	* is delete. */
	if (chdir("/") != 0)
	failwith_perror("chdir");

cc @dra27 @MisterDA

Fixes #1474

[voodoos]

@let-def was there a reason why we didn't chdir / in the Windows case like we do for unixes ?

[nojb]

@let-def was there a reason why we didn't chdir / in the Windows case like we do for unixes ?

What is / :) ?

[voodoos]

@nojb why not chdir to the root like in the unix cases ?

[voodoos]

What is / :) ?

Sorry, I was too lazy to write chdir("/") 😅

[nojb]

@nojb why not chdir to the root like in the unix cases ?

Yes, I also considered doing that by using PathCchStripToRoot, but did not see a clear win over GetTempPath. @MisterDA any opinions?

[nojb]

What is / :) ?

Sorry, I was too lazy to write chdir("/") 😅

I meant that / does not mean anything on Windows...

[MisterDA]

No strong opinion. Switching to the temporary path seems fine.

[voodoos]

I meant that / does not mean anything on Windows...

Oh right, each drive has its own root. I would have expected that cd'ing to \ cd's to the current drive's root, though.

[let-def]

I meant that / does not mean anything on Windows...

Correct me if I am wrong, I remember trying that on windows at some point, and I remember the effect was to move to the root of the current drive (so C:\ or D:\). Though it might have been cygwin doing some magic, I can't tell and it is foggy. (And is it safe to assume that C:\ is always valid ?)

However I don't know why we were not doing this on Windows before.

[nojb]

@voodoos

I meant that / does not mean anything on Windows...

Oh right, each drive has its own root. I would have expected that cd'ing to \ cd's to the current drive's root, though.

@let-def

I meant that / does not mean anything on Windows...

Correct me if I am wrong, I remember trying that on windows at some point, and I remember the effect was to move to the root of the current drive (so C:\ or D:\).

Oops, you are both right; I misspoke.

Though it might have been cygwin doing some magic, I can't tell and it is foggy. (And is it safe to assume that C:\ is always valid ?)

If it works, using chdir("\\") may indeed be better as it is closer to what is done for Unix. I'll test it and (if it works) will update the PR accordingly.

[nojb]

If it works, using chdir("\\") may indeed be better as it is closer to what is done for Unix. I'll test it and (if it works) will update the PR accordingly.

Tried it, seems to work fine, and it is simpler. Updated the PR.

@MisterDA do you mind taking a second look, since you already did a first pass over the PR? Thanks!

[MisterDA]

I'm not sure I knew you could chdir to root, like this, that's good to know.
This version seems actually better than the last one, chdir is a CRT function and will set errno and failwith_perror uses perror and errno. The previous version was using Win32 functions, I don't think they set errno since the error code has to be retrieved with GetLastError (in that sense failwith_perror, a wrapper over CRT functions, is wrong for all Win32 functions…).
Reported the problem at #1570.

[nojb]

Thanks for the review @MisterDA! I updated CHANGES. This is ready to go as far as I am concerned.

[voodoos]

The windows CI failure is a bit surprising.
There is a warning:

(cd _build/default/src/frontend/ocamlmerlin && D:\cygwin\bin\x86_64-w64-mingw32-gcc.exe -O2 -fno-strict-aliasing -fwrapv -mms-bitfields -oocamlmerlin.exe ocamlmerlin.c)
ocamlmerlin.c: In function 'start_server':
ocamlmerlin.c:293:7: warning: implicit declaration of function '_chdir'; did you mean 'chdir'? [-Wimplicit-function-declaration]
  293 |   if (_chdir("\\") == -1)
      |       ^~~~~~
      |       chdir
      ```
      
But also a number of different results in the test suite. I will have a look later.

[nojb]

There is a warning:

(cd _build/default/src/frontend/ocamlmerlin && D:\cygwin\bin\x86_64-w64-mingw32-gcc.exe -O2 -fno-strict-aliasing -fwrapv -mms-bitfields -oocamlmerlin.exe ocamlmerlin.c)
ocamlmerlin.c: In function 'start_server':
ocamlmerlin.c:293:7: warning: implicit declaration of function '_chdir'; did you mean 'chdir'? [-Wimplicit-function-declaration]
  293 |   if (_chdir("\\") == -1)
      |       ^~~~~~
      |       chdir
      ```

It may have been a missing include. I pushed a possible fix.

But also a number of different results in the test suite. I will have a look later.

Thanks!

[voodoos]

So, one of the failing test is calling:

   $ ocamlmerlin server errors -filename test.ml -cmi-path sub < test.ml

And it looks like with that PR the argument -cmi-path sub is ignored (it prints an error saying that some module is not in path).

Not sure what is causing that, there nothing obvious in the diff.

[voodoos]

Right, maybe there is something wrong with how Merlin treat the relative path given as an argument now that the server is not spawned at the same place. @let-def does that ring a 🔔 ?

[nojb]

Right, maybe there is something wrong with how Merlin treat the relative path given as an argument now that the server is not spawned at the same place. @let-def does that ring a 🔔 ?

I think I now what is going on. In the Unix code, we only change the working directory for the child process, but in the present verseion of this PR we are changing the working directory also for the parent process.

I am working on a fix.

[dra27]

FWIW (sorry for late entry), chdir("\\") isn't strictly as on Unix... it's not impossible to be on a network or removable drive, and then Merlin is part of the "Windows cannot eject this drive right now" problem (i.e. this could give you G:\ which could be unmounted, whereas / cannot for the most part be unmounted) 🙂 Given that there's no need to write to it, I'd go with GetSystemDirectory which is guaranteed to exist on a non-removeable drive, or the GetTempPath2 but we're in hair-splitting territory!

[dra27]

Oo, except GetTempPath2 is really recent (if that hasn't already been spotted), so GetSystemDirectory is probably the better one.

[nojb]

Thanks @dra27 for the suggestion!

Switched to GetSystemDirectory; also, I made it so that only the current directory of the child process is modified (and not the parent's). This should fix the test suite errors. I also had to do a tiny fix to the code in New_merlin to restore the "old" working directory after handling a request.

Review welcome! cc @voodoos @dra27 @MisterDA

[nojb]

(By the way, this PR fixes #1474.)

[nojb]

Friendly ping.

[voodoos]

So, do I understand well that without these changes we update the server cwd before running a command without restoring it after that. Which means that the first cd to / is useless ?

Could you add a test illustrating this change's effect ?

[nojb]

So, do I understand well that without these changes we update the server cwd before running a command without restoring it after that.

Yes.

Which means that the first cd to / is useless ?

No, it is still necessary: otherwise after handling a request the server is left on the directory where it was first launched, and this can cause problems.

Could you add a test illustrating this change's effect ?

I'm not sure how I would go about it, since we are talking about correctly (re)setting the "current directory" of the server process, but there should be no observable difference for the user. But if you have any suggestions, I'm happy to give it a try.

[voodoos]

Arf, I just realized I edited your comment instead of replying to it... My answer:

Which means that the first cd to / is useless ?

No, it is still necessary: otherwise after handling a request the server is left on the directory where it was first > launched, and this can cause problems.

Sorry, my formulation was unclear, I meant "so the current (without these changes) cd to / done in the Unix case is useless because of this issue".

Could you add a test illustrating this change's effect ?

I'm not sure how I would go about it, since we are talking about correctly (re)setting the "current directory" of the server process, but there should be no observable difference for the user. But if you have any suggestions, I'm happy to give it a try.

Maybe by triggering the original issue: start the server, move the dir, perform a query, observe a crash ?

[nojb]

Sorry, my formulation was unclear, I meant "so the current (without these changes) cd to / done in the Unix case is useless because of this issue".

Yes, exactly.

Maybe by triggering the original issue: start the server, move the dir, perform a query, observe a crash ?

I'll give it a try and report back.

[nojb]

I pushed a test that shows that the current working directory was not being restored correctly before. I didn't try to exhibit a server misbehaviour because this was harder, it depends on the operating system, etc.

[voodoos]

Great, thank you !

[voodoos]

Thanks a lot @nojb !