path: appending local paths to external paths

[Alizter]

Rather than using Filename.concat we use / as a directory separator on all platforms for paths that we construct. We already do this for local paths, so appending to external paths shouldn't be any different. This makes path behaviour homogeneous across platforms.

Importantly on Windows something like:

C:\foo\bar + ./baz/zaz

used to be

C:\foo\bar\./baz/zaz

and is now

C:\foo\bar/baz/zaz

Notice that the prefix is untouched, since we don't control it. When scrubbing prefixes with BUILD_PATH_PREFIX_MAP this will mean that $PREFIX/baz/zaz will be the same on all platforms. That will be addressed in a later PR.

We do however touch the prefix if it ends in a trailing directory separator. Before appending we make sure that this separator is stripped if it exists.

• changelog
• check docs and fixup
• update rocq tests
• update oxcaml tests
• update other tests
• test(path): trailing slashes when appending #14279

Addresses parts of

• Tests fail on Windows with: Cannot decode build prefix map #10176

[Alizter]

@nojb You may also be interested in having a look since this relates to path constructions on Windows.

[Copilot]

Pull request overview

This PR makes path concatenation for external paths consistent across platforms by always using / when appending relative/local path fragments, and by normalizing away ./ and trailing separators in common cases.

Changes:

• Update Path_external.relative to join with / and strip leading ./ plus a trailing separator on the base path.
• Update stdune expect-tests and blackbox tests to reflect the new normalized output.
• Add a changelog entry documenting the behavior change.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
otherlibs/stdune/src/path_external.ml	Changes external path appending logic to normalize ./ and use / separators.
otherlibs/stdune/test/path_tests.ml	Updates expect-tests for external-relative normalization (./ removal and trailing slash handling).
otherlibs/stdune/test/path_external_build_tests.ml	Updates expectations to be platform-homogeneous for absolute filename/reach outputs.
test/blackbox-tests/test-cases/optional-executable.t	Adjusts expected printed binary paths to remove ./ segment.
doc/changes/fixed/14278.md	Documents the fix in the changelog.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[rgrinberg]

The PR says this is a fix, but what's the bug? That we use a different separate on windows?

[Alizter]

The PR says this is a fix, but what's the bug? That we use a different separate on windows?

That and /foo/ + ./bar becoming /foo/./bar.

[rgrinberg]

Is that a problem?

[Alizter]

Both choices of separator are arbitrary so strictly speaking we are not fixing anything. What we are doing however is choosing the more convenient option for dune. When BUILD_PATH_PREFIX_MAP works I would like:

  $ realdir foo
  $TESTCASE_ROOT/foo

rather than having two cases for Windows and otherwise.

[Alizter]

I've updated the PR title and commit message to say refactor now.

[rgrinberg]

Let's wait for @nojb or @dra27 to look this over.I don't know enough about windows to know whether this change is OK.

[nojb]

Not tested, but LGTM from a distance.

Of course, the logic remains a heuristic: if y starts with ././foo, we are still left with unnecessary components in the path, but if this behaviour matches between Windows and Unix, then that's already an improvement.

[nojb]

I'm unsure about this suggestion, but I always wonder if it isn't better (potentially less allocation) to use String.concat here:

Suggested change

	x ^ "/" ^ y)
	String.concat ~sep:"/" [x; y])

[Alizter]

That seems better thanks.

[Alizter]

This remains a draft until I can work out what is going wrong with promotion on Windows. It seems the entries haven't been added to the promotion database correctly.

[Alizter]

Fun! So Windows' CreateProcessW accepts forward slashes in paths, but when constructing a valid path such as C:\WINDOWS\system32/cmd.exe which is what Bin.which ends up with, we get an interesting situation. There is a bug with cmd.exe so that it checks all the command arguments, including "arg[0]" for its /c flag. The /cmd.exe confuses it and it fails. This happens only with (system) on Windows.

I will add special sanitation for paths involving cmd.exe since it only affect it.

[Alizter]

I'm still uncertain if we should be passing mixed paths to CreateProcessW even though it accepts it. Perhaps the correct thing to do would be to make sure all forward slashes are blackslashes before handing off to Spawn.

@nojb any recommendation?

[nojb]

I'm still uncertain if we should be passing mixed paths to CreateProcessW even though it accepts it. Perhaps the correct thing to do would be to make sure all forward slashes are blackslashes before handing off to Spawn.

@nojb any recommendation?

I think this makes sense. Backslashes are the native thing on Windows, so from a distance it indeed looks like a more principled approach.

[Alizter]

TODO:

• Take Allow Path.is_descendant to work on external paths. #14382 into account