Normalize case of context root under Win32

[nojb]

As discussed on Slack, the case of Sys.getcwd can vary from invocation to invocation of dune depending on the environment. This can result in gratuitous recompilations.

To avoid this, this PR proposes to normalize the case of Sys.getcwd (as exposed by Path) to lowercase (under Windows only).

cc @dra27 @rgrinberg

[rgrinberg]

One thing I didn't understand from the slack conversation is if its the drive letter that is case insensitive or the entire path? This change is only right if it's the latter.

[nojb]

One thing I didn't understand from the slack conversation is if its the drive letter that is case insensitive or the entire path? This change is only right if it's the latter.

I think it can be the whole path. I am trying to come up with a reproduction case.

[dra27]

It's true for the entire path, but I don't think you want to do this routinely - it can break if you happen to be on a case sensitive file system which is mounted and it looks suspicious when you start seeing c:\documents and settings\username\my documents. It's that comparisons want to be case insensitive.

Actually get the correct path involves a fairly simply wrapping of FindFirstFile IIRC.

[nojb]

It's true for the entire path, but I don't think you want to do this routinely - it can break if you happen to be on a case sensitive file system which is mounted and it looks suspicious when you start seeing c:\documents and settings\username\my documents. It's that comparisons want to be case insensitive.

But I think that changing Path so that comparisons are made case-insensitive on Windows is a big change, and it is not even clear to me which notion of "case-insensitive" we would be talking about...

Actually get the correct path involves a fairly simply wrapping of FindFirstFile IIRC.

This sounds simple enough. Do I understand your suggestions correctly to mean that we should call FindFirstFile on the result of Sys.getcwd?

[dra27]

It's true for the entire path, but I don't think you want to do this routinely - it can break if you happen to be on a case sensitive file system which is mounted and it looks suspicious when you start seeing c:\documents and settings\username\my documents. It's that comparisons want to be case insensitive.

But I think that changing Path so that comparisons are made case-insensitive on Windows is a big change, and it is not even clear to me which notion of "case-insensitive" we would be talking about...

Yeah, I realised that was likely to be the case... but equally the lowercase trick might just defer the problem to another day 🙂

Actually get the correct path involves a fairly simply wrapping of FindFirstFile IIRC.

This sounds simple enough. Do I understand your suggestions correctly to mean that we should call FindFirstFile on the result of Sys.getcwd?

Yes I believe that it will do the conversion - although FindFirstFile is expensive, you I think you'd want to check whether Sys.getcwd has changed and only if it has call FindFirstFile (i.e. store the pair (last_getcwd_result, last_normalised_getcwd_result) and just return snd if Sys.getcwd () returns fst.

[nojb]

@dra27 Coming back to this, the FindFirstFile trick would serve to normalize the different path components, but not the drive letter, right? For the drive letter, shouldn't we just decide to normalize to lowercase or uppercase?

[dra27]

I'm not sure - I'd expect that it would normalize it to uppercase, to be honest. It'd obviously be quite safe to do that in Dune to be sure. Some prior art (from a brief look around - the pull requests are old):

• PowerShell API (not sure if it's current) solving exactly this problem: https://github.com/PowerShell/PowerShell/blob/1e940f55b1f265c9640d622937383208e13a849b/src/System.Management.Automation/namespaces/FileSystemProvider.cs#L124
• Proposal to add a similar function to .NET: https://github.com/dotnet/corefx/pull/2219/files

(links as code to avoid GitHub spamming other repos!)

[nojb]

I'm not sure - I'd expect that it would normalize it to uppercase, to be honest.

The problem I have is that FindFirstFile only returns the normalized basename, so one has to call it recursively to normalize every path component, but it fails when passed a path consisting of only a drive letter, eg c:/.

It'd obviously be quite safe to do that in Dune to be sure. Some prior art (from a brief look around - the pull requests are old):

• PowerShell API (not sure if it's current) solving exactly this problem: https://github.com/PowerShell/PowerShell/blob/1e940f55b1f265c9640d622937383208e13a849b/src/System.Management.Automation/namespaces/FileSystemProvider.cs#L124

Looks like it calls FindFirstFile recursively, will take a closer look.

• Proposal to add a similar function to .NET: https://github.com/dotnet/corefx/pull/2219/files

This one uses GetLongPathName; I tried that, it normalizes the path components, but not the drive letter.

[nojb]

Coming back to this as this issue is blocking for us as it causes a lot of needless recompilation. I pushed a simpler fix which just normalizes the context root to lowercase under Windows. This fixes the recompilation problem for us.

Would appreciate a review! cc @dra27 @rgrinberg

[rgrinberg]

Looks good to me. There are other uses of Sys.getcwd () in the codebase, but if this fix is enough then so be it.

This needs a CHANGES entry as well.

[nojb]

Looks good to me. There are other uses of Sys.getcwd () in the codebase, but if this fix is enough then so be it.

This needs a CHANGES entry as well.

Done. I also cleaned the patch up a bit by adding Path.External.lowercase_ascii. @dra27 any objections to merging this?

[nojb]

I believe @dra27 it busy these days, so unless someone objects, I am planning to merge this as it is in the next few hours since it solves an actual problem and the change is small.

[nojb]

We discussed the PR with @dra27 on Slack, and he didn't see anything blocking (even if maybe the patch can be improved later on), so I will merge as soon as the CI is green.

[emillon]

I was looking at how to make cram tests work better on windows and I was surprised to see that now the result of Path.to_absolute_filename passed to cygpath is not detected as a cygwin path anymore:

$ cygpath 'c:\ocaml64\home'
/cygdrive/c/ocaml64/home
$ cygpath 'c:\OCaml64\home'
/home

[nojb]

I was looking at how to make cram tests work better on windows and I was surprised to see that now the result of Path.to_absolute_filename passed to cygpath is not detected as a cygwin path anymore:

Uf for once I thought this was going to be painless, but I was wrong...

[nojb]

I was looking at how to make cram tests work better on windows and I was surprised to see that now the result of Path.to_absolute_filename passed to cygpath is not detected as a cygwin path anymore:

$ cygpath 'c:\ocaml64\home'
/cygdrive/c/ocaml64/home
$ cygpath 'c:\OCaml64\home'
/home

Just to make sure I understand: is c:\OCaml64\ the cygwin root?

[emillon]

Yes, the cygwin root is c:\OCaml64\ (that's the default in fdopen's installer)

[nojb]

I was looking at how to make cram tests work better on windows and I was surprised to see that now the result of Path.to_absolute_filename passed to cygpath is not detected as a cygwin path anymore:

To be clear, the problem you have is with one path (namely, the context root) when passed to Path.to_absolute_filename, right? Because the function Path.to_absolute_filename has not itself been modified in any way...

[emillon]

I first tried to change cram rules so that both the POSIX and Windows versions of cwd are replaced by $TESTCASE_ROOT. At the moment this is using to_absolute_filename:

dune/src/dune_rules/cram_exec.ml

Line 150 in 8330833

{ source = Path.to_absolute_filename cwd; target = "$TESTCASE_ROOT" }

In turn, this function uses abs_root (which has been lowercased):

dune/src/stdune/path.ml

Lines 588 to 593 in 8330833

	let to_absolute_filename t =
	match t with
	| External s -> External.to_string s
	| In_source_dir l ->
	External.to_string
	(External.relative (Lazy.force abs_root) (Local.to_string l))

Because of this, the value that's being replaced as $TESTCASE_ROOT is something like "lowercase\windows\path\to\root/_build/path/to/dir, and it does not match anything 😕. I think passing this path to cygpath would work to create a POSIX path, but it doesn't because of the lowercase section. I'm a bit less concerned about the mix of forward and backslashes since cygpath can also be used to build a clean Windows path.