Enable CI on windows and fix it. #329

hhugo · 2024-05-27T14:31:35Z

This is a somewhat minimal list of changes to make the testsuite pass on windows.
If you look at the CI jobs, you should see that the testsuite pass under windows.

Includes:

part of quick windows fixes for 4.03 #70 62c40ca
part of Use forward slashes and quoted OCaml strings #323

Probably fix #316

gasche

This looks okay overall, see one question on the stderr rediction.

I'm not fond of the generalized use of explicit newline quoting in string literals in the testsuite, I find that it really hurts readability. Can we use quoted string literals instead?

gasche · 2024-05-27T20:42:39Z

testsuite/internal.ml

+      flag [\"library\"; \"ocaml\";           \"record_foo_stubs\"]\n\
+        (S ([A \"-cclib\"; A \"-lfoo_stubs\"]));\n\
+  | _ -> ()\n\
+  end\n\


Urgh, can you use quoted literals instead to avoid the many escapes here? (If there is a problem with \r\n somehow, we could inject a small helper function that removes the \r.)

I've pushed 0f2342a

gasche · 2024-05-27T20:49:01Z

src/my_std.ml

@@ -279,7 +279,7 @@ let sys_command =
  match Sys.os_type with
  | "Win32" -> fun cmd ->
      if cmd = "" then 0 else
-      let cmd = "bash --norc -c " ^ Filename.quote cmd in
+      let cmd = "bash --norc -c " ^ Filename.quote cmd ^ " 2>&1" in


Can you explain why we need this change?

I'm not happy with this change but it makes the testsuite pass.

I think the explanation is as follow:

if is_degraded or Sys.win32 is true, execute_many falls back to sys_command to implement execute_many.

otherwise, the logic from Ocamlbuild_executor.execute is used, which reads both stderr and stdout into a job_buffer.

The testuite sometime expect some output on stdout.

I'm still looking for a better solution.

I've moved the stderr redirection to command.ml where I think it belongs and added a small comment.

I went to look at the code of ocamlbuild_executor to understand what is going on. ocamlbuild_executor does not just redirect stderr into stdout, it lets the command write to different places and then on job termination it adds stdout, then stderr, to the job buffer, in that order. The change that you implemented does not match this behavior, it only works for commands that have only stdout or only stderr output.

Out of curiosity: in the execute_many definition, have you tried removing the "or we are on Windows" condition, to get the ocamlbuild_executor logic to run also on Windows? When ocamlbuild was first written, I think the Windows support in stdlib and Unix was weaker than it is now, there is a small chance that removing those alternate paths could work today.

(We could probably also get rid of the "is_degraded" notion, which I think was designed to let ocamlbuild build the compiler in constrained environments. ocamlbuild is not considered as build system for the OCaml compiler anymore, so maybe we could forget about this case altogether. But this is more work than I was considering putting in ocamlbuild again.)

I've tried removing the special case for windows in execute_many but it fails in various ways. It seems that the patch https://raw.githubusercontent.com/ocaml-opam/opam-repository-mingw/354a87b397856f2a70024c5c83fc5001074935b6/packages/ocamlbuild/ocamlbuild.0.14.2/files/ocamlbuild-0.14.2.patch is doing that better. I could look at merging that patch but I'd rather first have a working CI for windows

I went to look at the code of ocamlbuild_executor to understand what is going on. ocamlbuild_executor does not just redirect stderr into stdout, it lets the command write to different places and then on job termination it adds stdout, then stderr, to the job buffer, in that order. The change that you implemented does not match this behavior, it only works for commands that have only stdout or only stderr output.

Looking at the code quickly, my impression is that it can interleave stdout and stderr. In the main loop, you can see

chrfds, do_read ~loop:false;

which tries to read any both stdout and stderr and push the content to job_buffer.

Maybe I'm misunderstanding the logic.

Ok, this suggests that I misunderstood, and there may be no good way to ensure that we get the same output. In that context I am okay with your current choice even though it is sort of a hack.

Out of curiosity: in the execute_many definition, have you tried removing the "or we are on Windows" condition, to get the ocamlbuild_executor logic to run also on Windows? When ocamlbuild was first written, I think the Windows support in stdlib and Unix was weaker than it is now, there is a small chance that removing those alternate paths could work today.

(We could probably also get rid of the "is_degraded" notion, which I think was designed to let ocamlbuild build the compiler in constrained environments. ocamlbuild is not considered as build system for the OCaml compiler anymore, so maybe we could forget about this case altogether. But this is more work than I was considering putting in ocamlbuild again.)

Done in #333 and #332

kit-ty-kate

Looks fine on first glance with my minimal Windows experience.

cc-ing more experienced Windows users @dra27 @jonahbeckford

kit-ty-kate · 2024-05-28T12:04:20Z

src/shell.ml

@@ -39,7 +39,7 @@ let run args target =
  let cmd = String.concat " " (List.map quote_filename_if_needed args) in
  if !*My_unix.is_degraded || Sys.os_type = "Win32" then
    begin
-      Log.event cmd target Tags.empty;
+      if false then Log.event cmd target Tags.empty;


Why is this disabled?

Suggested change

if false then Log.event cmd target Tags.empty;

Log.event cmd target Tags.empty;

It make the testsuite fail because it output cmd on stdout.
I wonder if we should prefix the command with "+ " similar to what is done in non-degraded mode. The testsuite filters theses lines out already.

I've added the + prefix

testsuite/ocamlbuild_test.ml

gasche · 2024-05-28T13:37:17Z

Ideally what I would like to do is to get good Windows support without making the codebase worse than it already is. I think that the current PR you propose is okay in this respect, the changes are relatively minimal and at the same time justified, and the risk of regression for non-Windows users is low. I propose to merge it and build up from there.

On the other hand I am more worried about the fdopen patches in #330, which look like a mix of good ideas (I like getting rid of the "or is win32" case in the "is degraded?" test), inscrutable hacks, and things in the middle, and was never really tested on non-Windows systems if I understand correctly. Taking the good pieces out of it is an excellent idea, and maybe all of it is good and I just don't understand it yet. But making our codebase harder to understand (obscure changes with little explanations and no commit message) is getting convenience right now but also even more pain later, so I would not be in favor.

Note: I noticed the same Be consistent! comment in the present PR and in the .patch file somewhere. Are you the author of all the changes in the current PR? If not, are reused fragments properly credited and do they have a compatible software license?

hhugo · 2024-05-28T13:47:13Z

00b92ab was extracted from an old PR #70 -> 62c40ca because I was seeing the issue mentioned in this PR.

gasche · 2024-05-28T13:51:33Z

Could you cherry-pick that commit (or amend the author information on your own commit) to properly credit fdopen in the metadata?

… with My_unix (win32 only)

hhugo · 2024-05-28T14:00:40Z

Could you cherry-pick that commit (or amend the author information on your own commit) to properly credit fdopen in the metadata?

Done

gasche · 2024-05-28T14:15:58Z

@kit-ty-kate are you okay with merging this? My reasoning is: the patch looks okay, the potential for non-Windows regression is minimal, and I want to make it as easy as reasonably possible for @hhugo (or of course anyone else) to improve the state of ocamlbuild on Windows.

hhugo · 2024-05-28T14:27:32Z

(@dbuenzli, just for information, do you have any plan to drop your dependency on ocamlbuild ?, any ETA ?)

gasche · 2024-05-28T17:06:13Z

I went ahead and merged. Thanks @hhugo!

gasche · 2024-05-28T17:06:52Z

(It may be possible to close the issues that you helpfully linked, but at least the most recent one had more changes that were more contentious, and might deserve a discussion of their own. I hope that the part that you included solves most of the issues with less risk of regression.)

dbuenzli · 2024-05-28T18:21:42Z

any ETA ?

Not really, but I'd say within two or three years.

kit-ty-kate · 2024-05-28T19:14:38Z

@kit-ty-kate are you okay with merging this? My reasoning is: the patch looks okay, the potential for non-Windows regression is minimal, and I want to make it as easy as reasonably possible for @hhugo (or of course anyone else) to improve the state of ocamlbuild on Windows.

Merging is fine as long as we remember to not release before getting the double-check/approval of the two people i've cc'd above. I'd like to especially double-check the change made to plugin-lib/ocamlbuild_unix_plugin.ml and src/command.ml, which seem a bit fragile, especially as:

bash could be present in the environment, without it being linked to an MSYS2 or Cygwin installation (e.g. some programs like some git installation ship with bash.exe)
I'm not certain the quoting used is always correct with cmd.exe but I may be wrong. In particular I would like to understand this comment from 8 years ago: quick windows fixes for 4.03 #70 (comment)
~~Calls to bash uses --noprofile in Import fdopen patch for windows #330, is there a reason why this has not been done here too?~~ EDIT: nevermind i see why (-c vs interactive)

Hugo Heuzard and others added 5 commits May 27, 2024 15:31

Remove some deadcode

96d28c3

Add CI for windows

86c538c

error on illegal backslash

5575388

remove eol in strings

cb50dd5

remove absolute path in makefile

433b15b

gasche reviewed May 27, 2024

View reviewed changes

hhugo force-pushed the gh-actions-win branch 2 times, most recently from 76c30c1 to 5e26de4 Compare May 27, 2024 23:16

hhugo mentioned this pull request May 28, 2024

patched ocamlbuild for windows ocaml-opam/opam-repository-mingw#22

Closed

kit-ty-kate reviewed May 28, 2024

View reviewed changes

hhugo mentioned this pull request May 28, 2024

Import fdopen patch for windows #330

Closed

gasche approved these changes May 28, 2024

View reviewed changes

hhugo and others added 11 commits May 28, 2024 15:57

fix mkdir -p on windows

201a27d

Fix setting of build_dir

1b0bd6f

fix some tests for windows

f889adb

Unix.stat doesnt always work on windows (links)

d8f992b

tests: enable automatic crlf translation

84c8eab

redirect stderr to stdout, to follow execute_many behavior

12147fa

Testsuite: remove implicit newlines in strings

ec0c1f5

Use quoted string to improve readability

d5723e0

make test pass on windows

3f01658

fix ci

1d8e87a

use bash also inside ocamlbuild_unix_plugin in order to be consistent…

384e11f

… with My_unix (win32 only)

hhugo force-pushed the gh-actions-win branch from e01437d to 384e11f Compare May 28, 2024 14:00

gasche merged commit 6c6d471 into ocaml:master May 28, 2024
20 checks passed

hhugo deleted the gh-actions-win branch May 29, 2024 10:41

hhugo mentioned this pull request May 29, 2024

quick windows fixes for 4.03 #70

Closed

hhugo mentioned this pull request Jun 12, 2024

Failed to install ocamlbuild and ocaml-lsp-server on Windows ocaml/opam-repository#26063

Closed

dra27 mentioned this pull request Jun 26, 2024

Use forward slashes and quoted OCaml strings #323

Closed

hhugo mentioned this pull request Jul 4, 2024

OCamlbuild.0.15.0 ocaml/opam-repository#26164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable CI on windows and fix it. #329

Enable CI on windows and fix it. #329

hhugo commented May 27, 2024 •

edited

Loading

gasche left a comment

gasche May 27, 2024

hhugo May 27, 2024

gasche May 27, 2024

hhugo May 27, 2024 •

edited

Loading

hhugo May 27, 2024

gasche May 28, 2024

gasche May 28, 2024

hhugo May 28, 2024

hhugo May 28, 2024

gasche May 28, 2024

hhugo May 29, 2024

kit-ty-kate left a comment

kit-ty-kate May 28, 2024

hhugo May 28, 2024

hhugo May 28, 2024

gasche commented May 28, 2024 •

edited

Loading

hhugo commented May 28, 2024 •

edited

Loading

gasche commented May 28, 2024

hhugo commented May 28, 2024

gasche commented May 28, 2024

hhugo commented May 28, 2024

gasche commented May 28, 2024

gasche commented May 28, 2024

dbuenzli commented May 28, 2024

kit-ty-kate commented May 28, 2024 •

edited

Loading

	if false then Log.event cmd target Tags.empty;
	Log.event cmd target Tags.empty;

Enable CI on windows and fix it. #329

Enable CI on windows and fix it. #329

Conversation

hhugo commented May 27, 2024 • edited Loading

gasche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhugo May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kit-ty-kate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasche commented May 28, 2024 • edited Loading

hhugo commented May 28, 2024 • edited Loading

gasche commented May 28, 2024

hhugo commented May 28, 2024

gasche commented May 28, 2024

hhugo commented May 28, 2024

gasche commented May 28, 2024

gasche commented May 28, 2024

dbuenzli commented May 28, 2024

kit-ty-kate commented May 28, 2024 • edited Loading

hhugo commented May 27, 2024 •

edited

Loading

hhugo May 27, 2024 •

edited

Loading

gasche commented May 28, 2024 •

edited

Loading

hhugo commented May 28, 2024 •

edited

Loading

kit-ty-kate commented May 28, 2024 •

edited

Loading