Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import fdopen patch for windows #330

Closed
wants to merge 3 commits into from
Closed

Conversation

hhugo
Copy link
Collaborator

@hhugo hhugo commented May 28, 2024

This is a straight import of the patch used to fix ocamlbuild on windows inside opam-repository.

Opened as a Draft for now.

I've imported it on top of #329 so that we can have a CI.
I've added a commit to make the testsuite pass again on my machine.

@hhugo
Copy link
Collaborator Author

hhugo commented Jun 23, 2024

  • Here is a list of changes present in this PR and not in master.
  1. handle long command on windows (above 8191 bytes), by moving the command to a separate shell script.
  2. use a leading space instead of '' for bash commands (it looks nicer)
  3. handle Unix.EPIPE gracefully when Sys.win32 in ocamlbuild_executor
  4. normalize path with forward slash
  5. set stdout to binary mode to avoid CRLF

We might want to import 1 and 2, I don't know about 3. We can probably ignore 4 and 5

@hhugo
Copy link
Collaborator Author

hhugo commented Jun 23, 2024

3 was fixed in OCaml 4.05

  • #7342, #797: fix Unix.read on pipes with no data left on Windows
    it previously raised an EPIPE error, it now returns 0 like other OSes
    (Jonathan Protzenko, review by Andreas Hauptmann and Damien Doligez)

@hhugo
Copy link
Collaborator Author

hhugo commented Jun 23, 2024

2 -> #343

@hhugo
Copy link
Collaborator Author

hhugo commented Jun 23, 2024

  1. handle long command on windows (above 8191 bytes), by moving the command to a separate shell script.

@dra27, now that we no longer rely on cmd.exe do we still have such a low limit ? A quick search online says that createProcess has a limit of 32767.

@hhugo
Copy link
Collaborator Author

hhugo commented Jun 24, 2024

  1. is implemented in Add a tests for long commands  #344

@hhugo hhugo deleted the fdopen-patch branch June 26, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant