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

Clean up ecl SIGCHLD configuration #15441

Closed
vbraun opened this issue Nov 21, 2013 · 13 comments
Closed

Clean up ecl SIGCHLD configuration #15441

vbraun opened this issue Nov 21, 2013 · 13 comments

Comments

@vbraun
Copy link
Member

vbraun commented Nov 21, 2013

Switch from patching ecl to using ecl_set_option(ECL_OPT_TRAP_SIGCHLD, 0).

This ticket was originally a part of #14636.

CC: @SnarkBoojum @jdemeyer @kiwifb

Component: packages: standard

Keywords: ecl signal handling sigchld

Author: Volker Braun, Julien Puydt

Branch/Commit: u/vbraun/ecl_signal_beautification @ 7615258

Reviewer: Jean-Pierre Flori

Issue created by migration from https://trac.sagemath.org/ticket/15441

@vbraun vbraun added this to the sage-6.1 milestone Nov 21, 2013
@vbraun
Copy link
Member Author

vbraun commented Nov 21, 2013

Commit: 5ecc849

@vbraun
Copy link
Member Author

vbraun commented Nov 21, 2013

Branch: u/vbraun/ecl_signal_beautification

@vbraun
Copy link
Member Author

vbraun commented Nov 21, 2013

New commits:

[5ecc849](https://github.com/sagemath/sagetrac-mirror/commit/5ecc849)test that no SIGCHLD handler was installed
[ecdc36d](https://github.com/sagemath/sagetrac-mirror/commit/ecdc36d)Use ecl_set_option instead of patching to disable ECL_OPT_TRAP_SIGCHLD

@vbraun
Copy link
Member Author

vbraun commented Nov 21, 2013

comment:2

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.

@kiwifb
Copy link
Member

kiwifb commented Nov 21, 2013

comment:3

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.

@SnarkBoojum
Copy link
Mannequin

SnarkBoojum mannequin commented Nov 21, 2013

comment:4

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!

@vbraun
Copy link
Member Author

vbraun commented Nov 21, 2013

comment:5

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2013

Changed commit from 5ecc849 to 7615258

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2013

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

[7615258](https://github.com/sagemath/sagetrac-mirror/commit/7615258)clarify test that no SIGCHLD handler is installed

@vbraun
Copy link
Member Author

vbraun commented Nov 21, 2013

comment:7

Is this clearer?

@jpflori
Copy link

jpflori commented Dec 25, 2013

Changed keywords from none to ecl signal handling sigchld

@jpflori
Copy link

jpflori commented Dec 25, 2013

comment:8

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.

@jpflori
Copy link

jpflori commented Dec 25, 2013

Reviewer: Jean-Pierre Flori

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants