From 8e8f6716b3ad6ebedfb4469723fb7f32b40e0eef Mon Sep 17 00:00:00 2001 From: Ali Caglayan Date: Wed, 25 Mar 2026 12:03:21 +0100 Subject: [PATCH 1/2] path: appending local paths to external paths 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. The motivation is that both choices of separator are arbitrary, so strictly speaking we are not fixing a bug. What we are doing however is choosing the more convenient option for dune. When `BUILD_PATH_PREFIX_MAP` works we would like scrubbed paths like `$TESTCASE_ROOT/foo` to be the same on all platforms, rather than having two cases for Windows and otherwise. Addresses parts of https://github.com/ocaml/dune/issues/10176 Signed-off-by: Ali Caglayan --- doc/changes/fixed/14278.md | 2 ++ otherlibs/stdune/src/path_external.ml | 19 +++++++++++++- .../stdune/test/path_external_build_tests.ml | 25 ++++--------------- otherlibs/stdune/test/path_tests.ml | 12 ++++----- .../test-cases/optional-executable.t | 4 +-- 5 files changed, 32 insertions(+), 30 deletions(-) create mode 100644 doc/changes/fixed/14278.md diff --git a/doc/changes/fixed/14278.md b/doc/changes/fixed/14278.md new file mode 100644 index 00000000000..a233f8ce33f --- /dev/null +++ b/doc/changes/fixed/14278.md @@ -0,0 +1,2 @@ +- Use `/` as directory separator when appending local paths to external paths, + making path construction consistent across platforms. (#14278, @Alizter) diff --git a/otherlibs/stdune/src/path_external.ml b/otherlibs/stdune/src/path_external.ml index d97509a15e6..c74b74d64e1 100644 --- a/otherlibs/stdune/src/path_external.ml +++ b/otherlibs/stdune/src/path_external.ml @@ -27,7 +27,24 @@ let to_dyn t = Dyn.variant "External" [ Dyn.string t ] let relative x y = match y with | "." -> x - | _ -> Filename.concat x y + | _ -> + let y = + if String.length y >= 2 && y.[0] = '.' && is_dir_sep y.[1] + then String.drop y 2 + else y + in + (match y with + | "" | "." -> x + | _ -> + (* Strip a trailing directory separator from [x] so we don't produce + double slashes (e.g. "/root/" + "foo" -> "/root/foo"). We use + [is_dir_sep] so that on Windows a trailing '\' is also removed, + normalising the join to always use '/'. *) + let x = + let len = String.length x in + if len > 0 && is_dir_sep x.[len - 1] then String.take x (len - 1) else x + in + x ^ "/" ^ y) ;; let append_local t local = relative t (Local.to_string local) diff --git a/otherlibs/stdune/test/path_external_build_tests.ml b/otherlibs/stdune/test/path_external_build_tests.ml index 1409d12eea9..9413510b412 100644 --- a/otherlibs/stdune/test/path_external_build_tests.ml +++ b/otherlibs/stdune/test/path_external_build_tests.ml @@ -41,42 +41,27 @@ let%expect_test "Build.to_string relative" = let%expect_test "to_absolute_filename source path" = Path.to_absolute_filename (Path.relative Path.root "src/main.ml") |> print_endline; - check_on_win_or_unix - [%expect.output] - ~wind:{| /workspace\src/main.ml |} - ~unix:{| /workspace/src/main.ml |} + [%expect {| /workspace/src/main.ml |}] ;; let%expect_test "to_absolute_filename build path" = Path.to_absolute_filename (Path.relative Path.build_dir "foo/bar") |> print_endline; - check_on_win_or_unix - [%expect.output] - ~wind:{| /external-build\foo/bar |} - ~unix:{| /external-build/foo/bar |} + [%expect {| /external-build/foo/bar |}] ;; let%expect_test "reach build from source" = Path.reach (Path.relative Path.build_dir "foo") ~from:Path.root |> print_endline; - check_on_win_or_unix - [%expect.output] - ~wind:{| /external-build\foo |} - ~unix:{| /external-build/foo |} + [%expect {| /external-build/foo |}] ;; let%expect_test "reach source from build" = Path.reach (Path.relative Path.root "src") ~from:(Path.relative Path.build_dir "foo") |> print_endline; - check_on_win_or_unix - [%expect.output] - ~wind:{| /workspace\src |} - ~unix:{| /workspace/src |} + [%expect {| /workspace/src |}] ;; let%expect_test "reach_for_running build from source" = Path.reach_for_running (Path.relative Path.build_dir "foo/bar") ~from:Path.root |> print_endline; - check_on_win_or_unix - [%expect.output] - ~wind:{| /external-build\foo/bar |} - ~unix:{| /external-build/foo/bar |} + [%expect {| /external-build/foo/bar |}] ;; diff --git a/otherlibs/stdune/test/path_tests.ml b/otherlibs/stdune/test/path_tests.ml index 8352cfd6713..c7a3a130401 100644 --- a/otherlibs/stdune/test/path_tests.ml +++ b/otherlibs/stdune/test/path_tests.ml @@ -663,7 +663,7 @@ let%expect_test "external relative plain" = let%expect_test "external relative dot-slash multi" = external_relative "/root" "./foo/bar"; - [%expect {| /root/./foo/bar |}] + [%expect {| /root/foo/bar |}] ;; let%expect_test "external relative dot" = @@ -683,7 +683,7 @@ let%expect_test "external relative deep" = let%expect_test "external relative dot-slash single" = external_relative "/root" "./foo"; - [%expect {| /root/./foo |}] + [%expect {| /root/foo |}] ;; let%expect_test "external append_local multi" = @@ -698,7 +698,7 @@ let%expect_test "external append_local root" = let%expect_test "path relative external dot-slash" = relative (Path.of_string "/ext") "./foo/bar"; - [%expect {| External "/ext/./foo/bar" |}] + [%expect {| External "/ext/foo/bar" |}] ;; let%expect_test "path relative external plain" = @@ -711,14 +711,12 @@ let%expect_test "external relative trailing slash" = [%expect {| /root/foo/bar |}] ;; -(* CR-soon Alizter: should strip "./" and produce "/root/foo" *) let%expect_test "external relative trailing slash dot-slash" = external_relative "/root/" "./foo"; - [%expect {| /root/./foo |}] + [%expect {| /root/foo |}] ;; -(* CR-soon Alizter: should return "/root" *) let%expect_test "external relative dot-slash only" = external_relative "/root" "./"; - [%expect {| /root/./ |}] + [%expect {| /root |}] ;; diff --git a/test/blackbox-tests/test-cases/optional-executable.t b/test/blackbox-tests/test-cases/optional-executable.t index 530db7e8855..ccd9252eeff 100644 --- a/test/blackbox-tests/test-cases/optional-executable.t +++ b/test/blackbox-tests/test-cases/optional-executable.t @@ -167,7 +167,7 @@ Optional on the executable should be respected: > EOF $ PATH=./bin:$PATH dune build @run-x - binary path: $TESTCASE_ROOT/optional-binary-absent/./bin/dunetestbar + binary path: $TESTCASE_ROOT/optional-binary-absent/bin/dunetestbar In the same way as enabled_if: @@ -179,7 +179,7 @@ In the same way as enabled_if: > EOF $ PATH=./bin:$PATH dune build @run-x --force - binary path: $TESTCASE_ROOT/optional-binary-absent/./bin/dunetestbar + binary path: $TESTCASE_ROOT/optional-binary-absent/bin/dunetestbar $ cd .. From 68f660f53baa56b05cb9a2e046a8d2742ab4339f Mon Sep 17 00:00:00 2001 From: Ali Caglayan Date: Thu, 23 Apr 2026 13:06:01 +0200 Subject: [PATCH 2/2] fix(win): make sure cmd.exe is called with backslashes only Signed-off-by: Ali Caglayan --- otherlibs/stdune/src/env_path.ml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/otherlibs/stdune/src/env_path.ml b/otherlibs/stdune/src/env_path.ml index cb3616c46f9..b4b9cb4326d 100644 --- a/otherlibs/stdune/src/env_path.ml +++ b/otherlibs/stdune/src/env_path.ml @@ -28,6 +28,15 @@ let system_shell_exn = let bin = lazy (Bin.which ~path:(path Env.initial) cmd) in fun ~needed_to -> match Lazy.force bin with + | Some path when Sys.win32 -> + (* cmd.exe has a quirky property where it will scan all of its args for + /c when parsing its flags. Even though CreateProcessW accepts forward + slash paths, calling C:\Windows\system32/cmd.exe with mixed path + separators will cause issues because cmd.exe will believe /cmd.exe is + an argument and fail. In order to avoid this we explicitly replace the + forward slashes in the cmd.exe path with backslashes. Other programs + don't have this issue luckily. *) + Path.of_string (String.replace_char ~from:'/' ~to_:'\\' (Path.to_string path)), arg | Some path -> path, arg | None -> User_error.raise