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.

Sign up for GitHub

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

Jump to bottom

Use threaded console, adapt for Windows #7906

Merged
merged 5 commits into from
Jun 6, 2023
Merged

Use threaded console, adapt for Windows #7906

merged 5 commits into from
Jun 6, 2023

Conversation

nojb
Copy link
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 rgrinberg and Alizter 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
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
@@ -61,7 +61,7 @@ let make (module Base : S) : (module Dune_console.Backend) =
let start () =
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
Member

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
Collaborator Author

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
Collaborator Author

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
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
Collaborator Author

nojb commented Jun 6, 2023

Please remember to run ocamlformat.

Thanks for the reminder, done.

@emillon
Copy link
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
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
Collaborator

Alizter commented Jun 6, 2023

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

@emillon
Copy link
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
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
21 of 22 checks passed
@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.

None yet
4 participants
@nojb @emillon @Alizter @rgrinberg