Skip to content

Use threaded console, adapt for Windows#7906

Merged
nojb merged 5 commits intoocaml:mainfrom
nojb:console_fix
Jun 6, 2023
Merged

Use threaded console, adapt for Windows#7906
nojb merged 5 commits intoocaml:mainfrom
nojb:console_fix

Conversation

@nojb
Copy link
Copy Markdown
Collaborator

@nojb nojb commented Jun 6, 2023

Fixes the issue discussed in #7418 (comment).

@nojb
Use threaded console, adapt for Windows
014757d 
Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>
@nojb nojb requested review from Alizter and rgrinberg June 6, 2023 14:06
@nojb nojb added this to the 3.8.1 milestone Jun 6, 2023
@nojb
CHANGES.md
b41b376 
Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>
rgrinberg
rgrinberg approved these changes Jun 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please remember to run ocamlformat.

Note that you should also mention that we're switching to the threaded console again for everyone.

Finally, I still don't understand why the unthreaded console is broken. It works just fine outside of Windows.

rgrinberg
rgrinberg reviewed Jun 6, 2023
View reviewed changes
src/dune_threaded_console/dune_threaded_console.ml Outdated
Base.start ();
Dune_engine.Scheduler.spawn_thread @@ fun () ->
ignore (Unix.sigprocmask SIG_UNBLOCK [ Signal.to_int Winch ] : int list);
if Sys.unix then ignore (Unix.sigprocmask SIG_UNBLOCK [ Signal.to_int Winch ] : int list);
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So cygwin doesn't emulate signals on windows?

Copy link
Copy Markdown
Collaborator Author

@nojb nojb Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, I never use Cygwin executables, but I can change the condition to not Sys.win32 if that sounds better.

Copy link
Copy Markdown
Collaborator Author

@nojb nojb Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the change.

nojb added 2 commits June 6, 2023 15:19
@nojb
Narrower condition
e008d39 
Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>
@nojb
CHANGES.md
d71259f 
Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>
@nojb
Copy link
Copy Markdown
Collaborator Author

nojb commented Jun 6, 2023 •
edited
Loading

Note that you should also mention that we're switching to the threaded console again for everyone.

Done.

Finally, I still don't understand why the unthreaded console is broken. It works just fine outside of Windows.

Indeed; needs to be looked into.

@nojb
make fmt
f276f90 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Copy Markdown
Collaborator Author

nojb commented Jun 6, 2023

Please remember to run ocamlformat.

Thanks for the reminder, done.

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Jun 6, 2023

3.8.1 has been released, I'm removing it from the milestone.

@emillon emillon removed this from the 3.8.1 milestone Jun 6, 2023
@Alizter Alizter mentioned this pull request Jun 6, 2023
Closed
7 tasks
@nojb
Copy link
Copy Markdown
Collaborator Author

nojb commented Jun 6, 2023

3.8.1 has been released, I'm removing it from the milestone.

Argh, I missed that. Is a 3.8.2 planned?

@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented Jun 6, 2023

@nojb I added it to a list of backports #7907

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Jun 6, 2023

Yes, we'll have to do one since there are more coq fixes.
But in any case please note that setting the milestone is only for the PR that get into the 3.8 branch (in other words, the backport), not the one that targets main.

@nojb
Copy link
Copy Markdown
Collaborator Author

nojb commented Jun 6, 2023

But in any case please note that setting the milestone is only for the PR that get into the 3.8 branch (in other words, the backport), not the one that targ

Got it, thanks!

@nojb nojb merged commit 5403f79 into ocaml:main Jun 6, 2023
@nojb nojb deleted the console_fix branch June 6, 2023 15:01
emillon pushed a commit to emillon/dune that referenced this pull request Jun 9, 2023
@nojb @emillon
Use threaded console, adapt for Windows (ocaml#7906)
ac9fd04 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
emillon pushed a commit to emillon/dune that referenced this pull request Jun 9, 2023
@nojb @emillon
Use threaded console, adapt for Windows (ocaml#7906)
1afe808 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
emillon added a commit that referenced this pull request Jun 9, 2023
@emillon @nojb
Use threaded console, adapt for Windows (#7906) (#7927)
6665a24 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Co-authored-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@emillon emillon mentioned this pull request Jun 16, 2023
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 16, 2023
@emillon
[new release] dune (15 packages) (3.8.2)
9832c0d 
CHANGES:

- Switch back to threaded console for all systems; fix unresponsive console on
  Windows (ocaml/dune#7906, @nojb)

- Respect `-p` / `--only-packages` for `melange.emit` artifacts (ocaml/dune#7849, @anmonteiro)

- Fix scanning of Coq installed files (@ejgallego, reported by
  @palmskog, ocaml/dune#7895 , fixes ocaml/dune#7893)

- Fix RPC buffer corruption issues due to multi threading. This issue was only
  reproducible with large RPC payloads (ocaml/dune#7418)

- Fix printing errors from excerpts whenever character offsets span multiple
  lines (ocaml/dune#7950, fixes ocaml/dune#7905, @rgrinberg)
@jonahbeckford jonahbeckford mentioned this pull request Jun 24, 2023
Closed
jonahbeckford added a commit to jonahbeckford/dune that referenced this pull request Jun 25, 2023
@jonahbeckford
Revert "Use threaded console, adapt for Windows (ocaml#7906) (ocaml#7927
74a2b68 
)"

This reverts commit 6665a24.

Probe if removing PR7906 fixes stalling described in ocaml#8043
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

@Alizter Alizter Awaiting requested review from Alizter

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nojb @emillon @Alizter @rgrinberg