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

Fix behaviour when stdout gets closed #4901

Merged
merged 3 commits into from
Feb 3, 2022
Merged

Conversation

AltGr
Copy link
Member

@AltGr AltGr commented Nov 9, 2021

Restore the sigpipe handler before exiting, to prevent the finaliser
from crashing on flushes.

Closes #4216: the behaviour on that example is now to exit with code
141, and without an error message.

@kit-ty-kate
Copy link
Member

Could you add a one or two line cram test to the testsuite?

@AltGr
Copy link
Member Author

AltGr commented Nov 9, 2021

Hmm I can try but we'll have to resort to using shell-pipes and that probably will break on Windows. @dra27 what do you think ?

@rjbou rjbou added this to PR in progress in Opam 2.2.0 via automation Nov 12, 2021
@rjbou rjbou added this to the 2.2.0~alpha milestone Nov 12, 2021
@kit-ty-kate
Copy link
Member

Tests showing the bug added in #5022

kit-ty-kate added a commit to kit-ty-kate/opam that referenced this pull request Jan 22, 2022
kit-ty-kate added a commit to kit-ty-kate/opam that referenced this pull request Jan 22, 2022
@kit-ty-kate kit-ty-kate added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Jan 22, 2022
kit-ty-kate added a commit to kit-ty-kate/opam that referenced this pull request Jan 25, 2022
@kit-ty-kate kit-ty-kate removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Jan 26, 2022
@kit-ty-kate
Copy link
Member

This needs to be applied for the tests to pass

commit d17de7079cb277081274308fafc4ef9a5afa8164 (HEAD -> broken-pipe)
Author: Kate <kit.ty.kate@disroot.org>
Date:   Wed Jan 26 09:50:03 2022 +0000

    Show that https://github.com/ocaml/opam/issues/4216 is now fixed

diff --git a/tests/cram/unix/list.t b/tests/cram/unix/list.t
index 113085860..99c2fdf0b 100644
--- a/tests/cram/unix/list.t
+++ b/tests/cram/unix/list.t
@@ -1,6 +1,5 @@
 Test https://github.com/ocaml/opam/issues/4216
   $ export OPAMROOT=./opamroot
   $ opam init --bare -n git+https://github.com/ocaml/opam-repository.git#0ec9bbf73a91b8e90b15bb11735859c9552a9888 > /dev/null
-  $ OCAMLRUNPARAM=-b opam list --depends-on dune -s --all-versions | head -n1
+  $ opam list --depends-on dune -s --all-versions | head -n1
   0install.2.14
-  Fatal error: exception Sys_error("Broken pipe")

Restore the sigpipe handler before exiting, to prevent the finaliser
from crashing on flushes.

Closes ocaml#4216
@rjbou rjbou moved this from PR in progress to PR to review in Opam 2.2.0 Feb 2, 2022
@AltGr
Copy link
Member Author

AltGr commented Feb 2, 2022

Thanks for the rebase/test update @rjbou

@kit-ty-kate kit-ty-kate moved this from PR to review to PR finalised (merge with CI) in Opam 2.2.0 Feb 2, 2022
@kit-ty-kate
Copy link
Member

Thanks!

@kit-ty-kate kit-ty-kate merged commit 6594901 into ocaml:master Feb 3, 2022
Opam 2.2.0 automation moved this from PR finalised (merge with CI) to Done Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Opam 2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

Sys_error("Broken pipe") printed to console on opam list
3 participants