Fix math-readline script

[dimpase]

sage: mathematica(1)

hangs indefinitely if Mathematica is not installed.

The reason is that internally the script $SAGE_LOCAL/bin/math-readline is called by Expect(), and the latter hangs. This script should check whether the math command has finished: currently the math-readline script keeps running even after the subprocess has exited.

CC: @nathanncohen @Etn40ff @NathanDunfield @seblabbe

Component: scripts

Author: Jeroen Demeyer

Branch/Commit: a3c75ab

Reviewer: Nathann Cohen, Sébastien Labbé, Salvatore Stella, Dima Pasechnik

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

[dimpase]

Commit: 6b9a115

[dimpase]

Branch: public/18908

[dimpase]

comment:2

moving the discussion from the closed #16703 here.
I propose the patch in the attached public/18908 branch.
I don't have a system with mathematica installed, I only tested it works nicely without math present, both stand-alone and as well as producing the right result when calling mathematica(10) from Sage.

[nathanncohen]

comment:3

Works for me, though I don't have mathematica either. Can someone who has it run the optional mathematica doctests with it?

Nathann

[dimpase]

comment:4

Replying to @nathanncohen:

Works for me, though I don't have mathematica either. Can someone who has it run the optional mathematica doctests with it?

works for me with Mathematica 9, I don't seem to have Mathematica 10 though.
(well, doctests fail due to numerical noise etc, but the patch itself looks good)

[dimpase]

comment:5

I'll try to fix doctests for Mathematica 9 then...

[nathanncohen]

comment:6

They may get pretty annoying when #18904 will be merged ;-)

[dimpase]

comment:7

basically a lot of output is texified, or semi-texified, for a reason I don't get. Typically I see things like

File "src/sage/interfaces/mathematica.py", line 44, in sage.interfaces.mathematica
Failed example:
    mathematica('Factor[x^2-1]')          # optional - mathematica
Expected:
    (-1 + x)*(1 + x)
Got:
    (x-1) (x+1)
**********************************************************************
File "src/sage/interfaces/mathematica.py", line 46, in sage.interfaces.mathematica
Failed example:
    mathematica('Range[3]')               # optional - mathematica
Expected:
    {1, 2, 3}
Got:
    \{1,2,3\}

Now, the hard question: where does one have the place to describe the (pseudo)package type,
i.e. optional vs experimental?!

[seblabbe]

comment:8

Replying to @dimpase:

I'll try to fix doctests for Mathematica 9 then...

see also #18888 which tries to fix doctests for Mathematica 10 to avoid conflicts...

[Etn40ff]

comment:9

I have mathematica 10.1 but the machine is currently out of service. It should be back, hopefully, late next week; I can run the doctest then.

[dimpase]

comment:10

Replying to @seblabbe:

Replying to @dimpase:

I'll try to fix doctests for Mathematica 9 then...

see also #18888 which tries to fix doctests for Mathematica 10 to avoid conflicts...

Ah, OK, so let us not fix any doctests here then, do it on #18888 instead.

[seblabbe]

comment:11

I have Mathematica 10.0 for Linux x86 (64-bit) installed on my machine. I installed the branch. I did make start (I think sage -b does not update the script right?). And it seems to work:

sage: mathematica(10)
10

To make sure that the new code was really tested after the make start, I added some print:

diff --git a/src/bin/math-readline b/src/bin/math-readline
index e6d40ef..ead0980 100755
--- a/src/bin/math-readline
+++ b/src/bin/math-readline
@@ -11,6 +11,11 @@ try:
 except:
     sys.exit('ERROR: executable math not found')
 
+print "NEW CODE IS BEING TESTED!!!"
+print "NEW CODE IS BEING TESTED!!!"
+print "NEW CODE IS BEING TESTED!!!"
+print "NEW CODE IS BEING TESTED!!!"
+
 f1 = subprocess.Popen('math', shell=True, stdin=subprocess.PIPE).stdin
 f1.flush()
 try:

did make start, run sage, but then, I still get

sage: mathematica(10)
10

So is the new code really run? Is it okay if the print are not shown?

[nathanncohen]

comment:12

Perhaps you should run 'make' in Sage's ROOT folder before trying it again. This could copy the updated math-realine from src/bin/ to local/bin/.

[seblabbe]

comment:13

I do see this line at the end of make start :

cp /home/labbe/Applications/sage-git/src/bin/math-readline /home/labbe/Applications/sage-git/local/bin/math-readline

[Etn40ff]

comment:14

Replying to @seblabbe:

So is the new code really run? Is it okay if the print are not shown?

I think it is, you should not get the output of math-readline within sage. It should be stripped as it is stripped the header

Mathematica 10.0 for Linux x86 (64-bit)
Copyright 1988-2014 Wolfram Research, Inc.

[seblabbe]

comment:15

Ok, so then, positive review!

[seblabbe]

Reviewer: Nathann Cohen, Sébastien Labbé, Salvatore Stella

[seblabbe]

Author: Dima Pasechnik

[seblabbe]

comment:16

Wait, is there a way to doctest this fix?

mathematica(10)  # not mathematica
Some error message...

[nathanncohen]

comment:17

Wait, is there a way to doctest this fix?

No, though others have asked (Dima in particular).

You should not change the status of a ticket in positive_review to ask a question unrelated to what the ticket is supposed to address.

[dimpase]

comment:24

I am setting it back to positive review. Please feel free to submit a proper error report, not "wrong fix" bullsh*t, and then change the ticket status accordingly.

[Etn40ff]

comment:25

If I understand correctly what is being said by jdemeyer the problem is this:

$ math-readline
Mathematica 10.0 for Linux x86 (64-bit)
Copyright 1988-2014 Wolfram Research, Inc.

In[1]:= Exit

Traceback (most recent call last):
  File "/opt/sage/local/bin/math-readline", line 23, in <module>
    f1.writelines(line+'\n')
IOError: [Errno 32] Broken pipe

which is quite ugly.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

a3c75ab

Exit math-readline if child process exits

[sagetrac-git]

Changed commit from 6b9a115 to a3c75ab

[jdemeyer]

Changed author from Dima Pasechnik to Dima Pasechnik, Jeroen Demeyer

[jdemeyer]

comment:28

Sometimes it is easier to just write a patch and explain later rather than the other way around. And unfortunately there is no ticket status for "hang on, I'm working on a patch".

The real problem with the script was that, when the math child process exited (for whatever reason, including Mathematica not being installed), the math-readline script did not exit. The proper fix is to check for the exiting of the child process. This works no matter for which reason the child process exited (which could be calling Exit in Mathematica, which could be Mathematica crashing, which could be Mathematica not being installed).

[jdemeyer]

comment:29

I also made a few more fixes to the script:

1. actually using readline, which was the whole point of the script!
2. removing the empty write sys.stdout.write('') (what was the point of that?).
3. using write() instead of writelines() to write a single string.
4. removing the unneeded sys.exit() at the end.

[dimpase]

Changed author from Dima Pasechnik, Jeroen Demeyer to Jeroen Demeyer

[dimpase]

comment:32

Replying to @jdemeyer:

This is really the wrong fix.

FYI, I found this comment very annoying. Unless you want to alienate people, always say in such cases instead "I would like to improve this".

[jdemeyer]

comment:33

Replying to @dimpase:

Replying to @jdemeyer:

This is really the wrong fix.

FYI, I found this comment very annoying. Unless you want to alienate people, always say in such cases instead "I would like to improve this".

OK, point taken. What about "This is really not the correct fix, I'm having a look at how to improve it".

[dimpase]

comment:34

Replying to @jdemeyer:

Replying to @dimpase:

Replying to @jdemeyer:

This is really the wrong fix.

FYI, I found this comment very annoying. Unless you want to alienate people, always say in such cases instead "I would like to improve this".

OK, point taken. What about "This is really not the correct fix, I'm having a look at how to improve it"

s/correct/complete would do; indeed, saying that something is incorrect/wrong without giving an explanation could create a tension you don't need.

[jdemeyer]

comment:35

Replying to @dimpase:

saying that something is incorrect/wrong without giving an explanation could create a tension you don't need.

But it was really incorrect: the fact that the script hangs when math is not installed was a symptom of the problem, not the problem itself.

But at the time when I send that message, I didn't have a clear explanation of the real problem: I knew that the proposed solution was wrong, but I couldn't yet say what the right fix would be. Like I said: it's often easier to explain after you have a working patch.

And I had to act quickly because, these days, tickets which have positive_review are often closed very soon.

[dimpase]

comment:36

The script hanged without math installed, and hanged Sage in return. And that few lines that were in the fix solved this particular issue, obviously an improvement. Anything that is an improvement cannot be called incorrect.

[jdemeyer]

comment:37

Replying to @dimpase:

Anything that is an improvement cannot be called incorrect.

I completely disagree with this. An improvement which hides a bug is incorrect.

[dimpase]

comment:38

lgtm

[dimpase]

Changed reviewer from Nathann Cohen, Sébastien Labbé, Salvatore Stella to Nathann Cohen, Sébastien Labbé, Salvatore Stella, Dima Pasechnik

[dimpase]

comment:39

Replying to @jdemeyer:

Replying to @dimpase:

Anything that is an improvement cannot be called incorrect.

I completely disagree with this. An improvement which hides a bug is incorrect.

except that this one did not hide a bug, at least not any more than the original scrip did.

Anyhow, this is not the point; the point is that you should not brand anything at all "wrong", "incorrect" without a justification. (Unless you out to annoy people, and I hope that this is not the case).

[vbraun]

Changed branch from public/18908 to a3c75ab