ECL spkg : dirty workarounds?

[SnarkBoojum]

My debian/sage experiments have uncovered that sage's ecl package was compiled with a few tweaks:

(1) it is compiled with unicode disabled ; this is to avoid errors, like those which can be read about in this gentoo report. But that doesn't look like a real solution!

(2) SIGCHLD is disabled by directly patching the sources (patches/disable_sigchdl_handler.patch)

(3) threading is disabled ; I'll open another ticket for that, so let's that aside for now (and for this ticket)

To fix this ticket:
(1) use this ecl spkg (spkg diff) ;
(2) apply attachment: trac_14636_1.patch and attachment: trac_14636_2.patch to sagelib (in that order).

This fix is against sage 5.12.beta4.

CC: @jpflori

Component: packages: standard

Work Issues: add doctest

Reviewer: Volker Braun, Peter Bruin

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

[jdemeyer]

Replying to @SnarkBoojum:

(2) I'm annoyed by the fact that SIGCHLD was disabled by directly patching the sources

I don't see any patch doing this, please clarify...

[nexttime]

Replying to @SnarkBoojum:

(2) I'm annoyed by the fact that SIGCHLD was disabled by directly patching the sources -- wouldn't it be possible to change this by modifying an rc file or some such, whose path would be given to ecl at startup?

Which library does read start-up files? ;-)

[SnarkBoojum]

comment:3

@jdemeyer: disable_sigchdl_handler.patch is what you're looking for.

@nexttime: good catch! I had the impression that maxima was used as a library within ecl, but that ecl was used through its commandline. But indeed now that you ask, I see there is also a direct use of ecl-as-a-library. What saves the day though is that sage's ecl.pyx calls cl_boot itself, and hence it should be possible to modify the setting before that call. I should be able to do something about it.

[nbruin]

comment:4

(1) unicode: I suppose there are no fundamental obstacles for that. However, you'll have to essentially rewrite the ecllib wrapper. With Py3 this might become worthwhile, since that has unicode strings internally anyway (although undoubtedly stored with an encoding different from ecl's). Also, our main use, Maxima, is solidly pre-unicode, so I don't think there will be any benefits for us and probably extra problems from Maxima possibly getting unexpected data.

(2) signals: ECL has very complex and ingenious infrastructure to deal with signals, and it expects to handle signals itself (which is a bit odd for a library). With threads enabled there will be a separate thread for asynchronous signals, and Boehm repurposes some exotic signals to sync threads on GC events. We cannot afford to submit to ECL's way of handling signals, so we do some pretty horrible stuff with signals when starting ECL anyway (and every time we call into ECL for longer times!) Disabling SIGCHLD is probably the least of your worries. Also, I don't think ECL does anything with SIGCHLD events anyway (other than possibly installing its own handler to provide a hook for ecl programs and which by default ignores the signal)

[jdemeyer]

comment:5

Replying to @SnarkBoojum:

@jdemeyer: disable_sigchdl_handler.patch is what you're looking for.

Sorry, I was looking at an older version of the ECL spkg (this patch is a very recent addition).

[SnarkBoojum]

comment:6

@nbruin(1) where is the ticket about the python3 transition? As far as I know, a few sagenb deps (notably twisted and flask if I remember well) and some of sage's standard packages (for example pil) are a concern in that matter, but I can't find the relevant ticket.

@nbruin(2) I'll manage to deal with it : there's just an option to set in sage/libs/

[SnarkBoojum]

comment:7

I just found that sage/libs/ecl.pxd has a copy of ecl's src/h/external.h's ecl_option enum... lacking the ECL_OPT_TRAP_SIGCHLD item! That means all subsequent items in the list are off-by-one... how is that code even supposed to run correctly!?

[nbruin]

comment:8

Replying to @SnarkBoojum:

I just found that sage/libs/ecl.pxd has a copy of ecl's src/h/external.h's ecl_option enum... lacking the ECL_OPT_TRAP_SIGCHLD item! That means all subsequent items in the list are off-by-one... how is that code even supposed to run correctly!?

I assume that's an "extern" definition. It just gets translated literally into the C-source which then uses the values that external.h provides. Just as cdef typedef extern struct can afford to only mention the fields that are of interest, or even only with approximate types.

It may well be that particular member wasn't in ecl when the wrapper was written. Add it if you need it.

[jdemeyer]

comment:9

Replying to @SnarkBoojum:

@nbruin(1) where is the ticket about the python3 transition? As far as I know, a few sagenb deps (notably twisted and flask if I remember well) and some of sage's standard packages (for example pil) are a concern in that matter, but I can't find the relevant ticket.

I don't think there is a ticket yet, because it's obvious it's not going to happen anytime soon.

[SnarkBoojum]

comment:10

Replying to @jdemeyer:

Replying to @SnarkBoojum:

@nbruin(1) where is the ticket about the python3 transition? As far as I know, a few sagenb deps (notably twisted and flask if I remember well) and some of sage's standard packages (for example pil) are a concern in that matter, but I can't find the relevant ticket.

I don't think there is a ticket yet, because it's obvious it's not going to happen anytime soon.

A ticket isn't just about something which will happen just now ; it's a central place where ideas get gathered about an issue. In this case, a ticket listing what is holding back sage for the switch to python3 would probably be worthy.

[nbruin]

comment:11

Replying to @SnarkBoojum:

I just found that sage/libs/ecl.pxd has a copy of ecl's src/h/external.h's ecl_option enum... lacking the ECL_OPT_TRAP_SIGCHLD item!

Ah, indeed! You should just add that in ecl.pxd and do

    ecl_set_option(ECL_OPT_TRAP_SIGCHLD, 0)

before the boot call. Looked like Volker went for the quick-and-dirty solution there on #14055.

Note that even with that option, ECL still does install a SIGCHLD handler. However, we're not putting that back when we call ECL. Does SIGCHLD also need special treatment in eclsig.c? Jeroen is the expert on that file.

[jdemeyer]

comment:12

Replying to @nbruin:

Note that even with that option, ECL still does install a SIGCHLD handler.

Preferably, we don't have any SIGCHLD handler at all. Note that SIGCHLD is an unusual signal: it is the only signal which is ignored by default. Therefore, having a SIGCHLD handler which does nothing is still quite different from having no handler (and ignoring the signal).

Maybe it can be made to work with eclsig.c, but at least it requires some work.

[nbruin]

comment:13

Replying to @jdemeyer:

Preferably, we don't have any SIGCHLD handler at all. Note that SIGCHLD is an unusual signal: it is the only signal which is ignored by default. Therefore, having a SIGCHLD handler which does nothing is still quite different from having no handler (and ignoring the signal).

I wondered about that. It's not clear to me why in our situation it even makes a difference what we do to ECL_OPT_TRAP_SIGCHLD(but given that Volker's fix had effect apparently it does).

The Sage SIGCHLD should be the default or ignore handler. Right after calling cl_boot we reset all handlers back to the sage handlers, so it shouldn't matter what ECL installs. We only change back some select handlers in eclsig.c and SIGCHLD is not one of them. Is there something that ecl changes that doesn't get reverted by sigaction?

[SnarkBoojum]

Attachment: ecl-12.12.1.p4.spkg.gz

ECL spkg without the SIGCHLD patch to upstream

[SnarkBoojum]

Patch programmatically disabling SIGCHLD in sagelib's ECL code

[SnarkBoojum]

comment:14

Attachment: trac_14636_sigchld.patch.gz

[jdemeyer]

Work Issues: doctest

[jdemeyer]

comment:15

Please add the following doctest (sage/tests/interrupt.pyx is a good place for this test):

"""
Show that `SIGCHLD` is completely ignored by default. If the process
`p` finishes, there should be no `SIGCHLD` signal, so ``select()`` will
simply time out::

    sage: from select import select
    sage: import subprocess

    sage: p = subprocess.Popen(["sleep", "1"])  # long time
    sage: select([], [], [], 1.5)               # long time
    ([], [], [])
    sage: p.poll()                              # long time
    0

We now do the same but after installing a dummy `SIGCHLD`
handler::

    sage: import signal
    sage: def dummy_handler(a,b):
    ....:    pass
    sage: signal.signal(signal.SIGCHLD, dummy_handler)  # random
    sage: p = subprocess.Popen(["sleep", "1"])  # long time
    sage: select([], [], [], 1.5)               # long time
    Traceback (most recent call last):
    ...
    error: (4, 'Interrupted system call')
    sage: p.poll()                              # long time
    0

Reset the `SIGCHLD` handler::

    sage: signal.signal(signal.SIGCHLD, signal.SIG_IGN)  # random
"""

[jdemeyer]

comment:16

There is another complication with the test: the doctesting framework itself messes with SIGCHLD, so it needs slightly more work... let me think about it.

[nbruin]

comment:17

Unicode: See #12985. When I implemented ecl.pyx unicode support in ecl was not the default, so it was natural to skip it. I haven't kept up with its development, so I'm of little help for unicode-specific things. It looks like they got something close to working on #12985, though.

Given that all of calculus is designed to work with str, not with unicode, I think there is very little benefit in converting the ecl interface in being fully unicode aware (other than not having to turn off unicode support when building ecl), so the approach there (letting ecl work with unicode strings but convert them to ascii as soon as they leave ecl) is probably the quickest to get going.

[SnarkBoojum]

comment:65

No failing doctest ; now src/ is generated by a patched spkg-src.

[SnarkBoojum]

comment:66

It's still ok with 5.12.beta4.

[jdemeyer]

comment:67

Replying to @SnarkBoojum:

Again, I'm just moving something from patching upstream to calling an api...

Again, that's only a claim. A doctest can prove you are right.

[SnarkBoojum]

comment:68

Replying to @jdemeyer:

Replying to @SnarkBoojum:

Again, I'm just moving something from patching upstream to calling an api...

Again, that's only a claim. A doctest can prove you are right.

OOOHHH!!!! You want me to ADD a doctest!

I'm not sure it makes much sense to follow a line "a=0" by a doctest "a==0", but if you want I'll add one.

[SnarkBoojum]

Work Issues: add doctest

[kiwifb]

comment:69

I see that trac_14636_2.patch is the same (up to a slight edit in the commit) than the patch in #12985 so incorporating this ticket would make it a duplicate.
I think you should go ahead and add the doctest.

[vbraun]

comment:70

In the spirit of having one ticket per issue I've broken off the part about using and doctesting ecl_set_option(ECL_OPT_TRAP_SIGCHLD, 0) into a new ticket at #15441

[vbraun]

Changed author from Julien Puydt to Volker Braun

[vbraun]

comment:71

Duplicate of #12985, so close one of them. This one.

[gagern]

comment:72

Replying to @SnarkBoojum:

François, if you have a way to have a no-patch-ecl+lightly-patched-maxima combination to work, perhaps it would be best to push it into sage and be done with this ticket?

I've just pushed the build formula used on sage-on-gentoo into a branch associated with ticket:16178.

[gagern]

comment:73

Replying to @jpflori:

FYI ECL 13.5.1 has been released:

• https://sourceforge.net/mailarchive/message.php?msg_id=30906130

With [ticket:16178 the new maxima build system] included in sage 6.2, do you want to aim for an ECL update for 6.3?

[pjbruin]

Reviewer: Volker Braun, Peter Bruin

[pjbruin]

Changed author from Volker Braun to none

[pjbruin]

comment:74

Since the relevant changes were made in #12985 and #15441, we should close this as a duplicate.