Opened 7 years ago
Closed 7 years ago
#15441 closed defect (fixed)
Clean up ecl SIGCHLD configuration
Reported by: | vbraun | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.1 |
Component: | packages: standard | Keywords: | ecl signal handling sigchld |
Cc: | Snark, jdemeyer, fbissey | Merged in: | |
Authors: | Volker Braun, Julien Puydt | Reviewers: | Jean-Pierre Flori |
Report Upstream: | N/A | Work issues: | |
Branch: | u/vbraun/ecl_signal_beautification (Commits) | Commit: | 7615258e393c111f19b44b7ce85c934462ff6df9 |
Dependencies: | Stopgaps: |
Description
Switch from patching ecl to using ecl_set_option(ECL_OPT_TRAP_SIGCHLD, 0)
.
This ticket was originally a part of #14636.
Change History (9)
comment:1 Changed 7 years ago by
- Branch set to u/vbraun/ecl_signal_beautification
- Cc Snark jdemeyer fbissey added
- Commit set to 5ecc849ee55ca78627fe5e5b3de43eb80679a08f
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
I suspected that this is related to the observed test failure on eno/skynet (#15440) but it is actually not. But its better than what we have so we should ship it.
comment:3 Changed 7 years ago by
Haven't been active in sage for a bit. I needed to back off a little bit. I have to agree that looks much nicer and you added a nice looking doctest.
comment:4 Changed 7 years ago by
- Status changed from needs_review to needs_work
The patch to use ecl_set_option looks good, but the doctest patch looks very wrong : it doesn't check that ECL-in-sage doesn't use SIGCHLD, but rather that SIGCHLD isn't used at all!
comment:5 Changed 7 years ago by
I don't understand what you are trying to say. It tests both that neither Sage nor ECL set a SIGCHLD handler, i.e. that it remains NULL.
comment:6 Changed 7 years ago by
- Commit changed from 5ecc849ee55ca78627fe5e5b3de43eb80679a08f to 7615258e393c111f19b44b7ce85c934462ff6df9
Branch pushed to git repo; I updated commit sha1. New commits:
7615258 | clarify test that no SIGCHLD handler is installed |
comment:8 Changed 7 years ago by
- Keywords ecl signal handling sigchld added
- Reviewers set to Jean-Pierre Flori
- Status changed from needs_review to positive_review
Looks fine to me. Note I did not run the testsuite, but let's leave that to the patchbot. And hopefully we'll also update ECL at some point, the sage-on-gentoo folk did that though it was not painless.
comment:9 Changed 7 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
New commits: