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 vbraun

  • Branch set to u/vbraun/ecl_signal_beautification
  • Cc Snark jdemeyer fbissey added
  • Commit set to 5ecc849ee55ca78627fe5e5b3de43eb80679a08f
  • Status changed from new to needs_review

New commits:

​5ecc849test that no SIGCHLD handler was installed
​ecdc36dUse ecl_set_option instead of patching to disable ECL_OPT_TRAP_SIGCHLD

comment:2 Changed 7 years ago by vbraun

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 fbissey

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 Snark

  • 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 vbraun

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 git

  • Commit changed from 5ecc849ee55ca78627fe5e5b3de43eb80679a08f to 7615258e393c111f19b44b7ce85c934462ff6df9

Branch pushed to git repo; I updated commit sha1. New commits:

​7615258clarify test that no SIGCHLD handler is installed

comment:7 Changed 7 years ago by vbraun

  • Status changed from needs_work to needs_review

Is this clearer?

comment:8 Changed 7 years ago by jpflori

  • 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 vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.