Note when you are working on a draft PR and trying to get the test suite to pass, it is better to work on a separate branch (after activating Appveyor on your fork), otherwise you are consuming CI time that could be used for non-draft PRs.

Also cc @shindere that might be interested by this PR making the build system works with path containing spaces.

after activating Appveyor on your fork

Have link how to enable it ? I was only able to set as new project but it didn't loaded the yml file so I have to fill the gui dashboard.

Edit: ok I found it I will create another branch to test without triggering this CI.

Ok squashed my commit --fixup and rebased on trunk. Trunk head is broken for windows but it succeeded before so it should keep passing.

(closed/re-opened to force GitHub to redo the merge commit and pull in the fix for AppVeyor)

Many thanks for your work, @Et7f3. I find it both brave and cool
that you started to work on this.

Regarding your first commit, I have another, configure-based fix that
I hope to implement soon. I'll submit a PR and post a link to it
to the current one.

I couldn't review the rest of your PR now but will try to do so starting
from next week.

Did you also try make install? Including installing to a directory
with spaces in its name? Does that owrk, too, and is the installed compiler
usable to compile files that have also spaces in their paths?

I haven't tried make install because I still need to fix 4 tests related to pp/ppx: Does they are expected to take only program (like we can read in some comment) or quoted command (we see command in the helpstring) ?

I came after failure of ocamlmklib wrt to bad quoting of arguments. This isn't addressed. OCaml was builded in sane path and used in path with space. So this PR only address build system. I wonder If I should rewrite to use utils and then it will fix automatically or other solution are preferable.
I tried to build with clang in my env (strangely command where called with gcc alias). I can try to build with CC=/some/path with/space/clang. I expect failure for that case.

I have updated my message on GitHub with better explanation.

ocamlmklib don't quote args:

I don't understand the question, I'm sorry.

I see the ocamlmklib call the linker and the linker Config.mkdll and the linker is also called in utils/ccomp.ml
So I was asking if ocamlmklib should instead of directly call the linker reuse the code in ccomp.ml

I ignored the four failings tests and tried make install
I choke on this line:

Here we want to check that all cmi we try to install are absent ? Why not test existence/emptiness of the folder ?

Do you intend to fix it here

I think I will only fix the build system in this PR so fix make install. And I will do another round to make it usable.
This check was added in 4.13.0-alpha1. I still see PR that have base before 4.13.0 so I should better keep it and try to find a workaround.

Ok workaround found so make install successfully install all pieces. But the generated ocaml doesn't seem to have the workaround shebang. Should I bootstrap ?

I rebased (I needed a buildable branch). This PR don't cause regression with test added on trunk. For the last three tests I have a dirty patch for unix but since quoting is shell dependent it don't work on cmd.

From 94f57affd438c21c9d587ff75f8138b18e92b2b7 Mon Sep 17 00:00:00 2001
From: Et7f3 <cadeaudeelie@gmail.com>
Date: Fri, 12 Nov 2021 22:55:57 +0100
Subject: [PATCH] fix(tests): Dirty hack to have all tests green.

---
 ocamltest/tsl_lexer.mll                                       | 2 ++
 testsuite/tests/parsing/broken_invariants.ml                  | 2 +-
 testsuite/tests/ppx-contexts/test.ml                          | 4 ++--
 .../tests/typing-misc/typecore_empty_polyvariant_error.ml     | 2 +-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/ocamltest/tsl_lexer.mll b/ocamltest/tsl_lexer.mll
index 3a7f91748..d9d3c397c 100644
--- a/ocamltest/tsl_lexer.mll
+++ b/ocamltest/tsl_lexer.mll
@@ -90,6 +90,8 @@ and string acc = parse
       string (acc ^ space) lexbuf }
   | '\\'
     {string (acc ^ "\\") lexbuf}
+  | '\\' '"'
+    {string (acc ^ "\"") lexbuf}
   | '"'
     {acc}
 and comment = parse
diff --git a/testsuite/tests/parsing/broken_invariants.ml b/testsuite/tests/parsing/broken_invariants.ml
index 29e09a191..74774a463 100644
--- a/testsuite/tests/parsing/broken_invariants.ml
+++ b/testsuite/tests/parsing/broken_invariants.ml
@@ -6,7 +6,7 @@
   program="ppx.exe"
   *** toplevel
   all_modules="broken_invariants.ml"
-  flags="-ppx '${ocamlrun} ${test_build_directory_prefix}/ocamlc.byte/ppx.exe'"
+  flags="-ppx '\"${ocamlrun}\" \"${test_build_directory_prefix}/ocamlc.byte/ppx.exe\"'"
 *)
 
 let empty_tuple = [%tuple];;
diff --git a/testsuite/tests/ppx-contexts/test.ml b/testsuite/tests/ppx-contexts/test.ml
index 75d942fdc..b642118a3 100644
--- a/testsuite/tests/ppx-contexts/test.ml
+++ b/testsuite/tests/ppx-contexts/test.ml
@@ -14,13 +14,13 @@ flags = "-thread \
          -principal \
          -alias-deps \
          -unboxed-types \
-         -ppx ${program}"
+         -ppx '\"${program}\"'"
 **** ocamlc.byte
 module = "test.ml"
 flags = "-g \
          -no-alias-deps \
          -no-unboxed-types \
-         -ppx ${program}"
+         -ppx '\"${program}\"'"
 ***** check-ocamlc.byte-output
 *)
 
diff --git a/testsuite/tests/typing-misc/typecore_empty_polyvariant_error.ml b/testsuite/tests/typing-misc/typecore_empty_polyvariant_error.ml
index 9b4624d3e..6a03fbcfe 100644
--- a/testsuite/tests/typing-misc/typecore_empty_polyvariant_error.ml
+++ b/testsuite/tests/typing-misc/typecore_empty_polyvariant_error.ml
@@ -6,7 +6,7 @@
   program="ppx.exe"
   *** toplevel
   all_modules="${test_file}"
-  flags="-ppx '${ocamlrun} ${test_build_directory_prefix}/ocamlc.byte/ppx.exe'"
+  flags="-ppx '\"${ocamlrun}\" \"${test_build_directory_prefix}/ocamlc.byte/ppx.exe\"'"
 *)
 
 type t = [%empty_polyvar];;
-- 
2.30.1 (Apple Git-130)

For the original issue it should be enough.

Et7f3 (2021/11/12 16:15 -0800):
@Et7f3 commented on this pull request. > + echo ' rm '"'"'$(INSTALL_LIBDIR)'"'"'/stdlib__[a-z].cm'; \ + echo 'and re-run make install'; \ This command might delete more file that necessary on Cygwin because we don't add shopt command. Also instead of re-run make install I could have printed the original command If makefile weren't recursive: https://stackoverflow.com/a/38754878/7227940 One more point for "Recursive Make Considered Harmful". Do you want I help to de-recursify Makefile ?
Yes I do agree that recursive makefiles are harmful and do wnat them to go away but it's a big task and other stuff must be done before so please let's this one be in peace for the moment, but many thanks for your interest in the compiler's build system.

Ok I will come back after 5.1

most conflict are fix that were upstreamed independently so it is easy to chose which one we need to keep. many runtime-errors come from this commit 24bed4c.
Do you prefer I rebase or merge trunk in my branch ?

Almost all test pass like before. I have a dirty hack for the last one.

Either is fine, although personally I prefer rebasing, please.

This two commits: 6c0c5c4 and 3dce845 seems superseded by 89fb5b7 that is already in merged. Do you think I can drop them or in some case it can be useful ?