Opened 5 years ago

Closed 5 years ago

#21135 closed defect (fixed)

octave >= 4.0 launches GUI by default

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-7.4
Component: interact Keywords:
Cc: chapoton, charpent Merged in:
Authors: Dima Pasechnik Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 1ed88ae (Commits, GitHub, GitLab) Commit: 1ed88aef15cae31754965a9a6eb2de55667ca5c4
Dependencies: Stopgaps:

Status badges

Description (last modified by vdelecroix)

With recent octave versions (>= 4.0)

sage: octave('1')  # hangs forever

The reason is that newer octave launch the GUI interface by default (there is an option --no-gui to get rid of it). See https://www.gnu.org/software/octave/NEWS-4.0.html

Apparently, they also changed the prompt from > to >>.

$ octave --no-gui --silent
>> 1
ans =  1

but this is not documented in the changelog.


Note: to test this branch, you should

  • apply the branch
  • do sage -b
  • run the octave tests with
    $ export SAGE_ROOT=$(sage -root)
    $ sage -t --optional=sage,octave $SAGE_ROOT/src/sage/interfaces/octave.py
    

Change History (57)

comment:1 Changed 5 years ago by vdelecroix

  • Description modified (diff)

comment:2 Changed 5 years ago by vdelecroix

  • Description modified (diff)
  • Summary changed from newer octave launch GUI by default to octave >= 4.0 launches GUI by default

comment:3 Changed 5 years ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Branch set to u/vdelecroix/21135
  • Commit set to 138920133555e167a5776756f39e9ebbcd4cee57
  • Status changed from new to needs_review

New commits:

138920121135: fix octave interface for version >= 4.0

comment:4 Changed 5 years ago by vdelecroix

  • Description modified (diff)

comment:5 Changed 5 years ago by vdelecroix

  • Description modified (diff)

comment:6 Changed 5 years ago by dimpase

Did you see this: https://github.com/sagemathinc/smc-sage/commit/f524d58c3db07856140f5557aa57cfa21a5895b3 ?

(an update to octave interface used on SMC)

comment:7 follow-up: Changed 5 years ago by vdelecroix

Or more precisely, https://github.com/billpage/sage-octave. But I do not see your point. Either we stick to sage/interfaces/octave.py or we use some external octave.py but only one at a time. If you want to port the enhancement from sage-octave into Sage I will be glad to review it.

comment:8 in reply to: ↑ 7 Changed 5 years ago by dimpase

Replying to vdelecroix:

Or more precisely, https://github.com/billpage/sage-octave. But I do not see your point. Either we stick to sage/interfaces/octave.py or we use some external octave.py but only one at a time. If you want to port the enhancement from sage-octave into Sage I will be glad to review it.

my point was that it looks like a reasonable improvement for src/sage/interfaces/octave.py

I'll try to merge it with the branch on this ticket.

comment:9 Changed 5 years ago by dimpase

for the record, your branch tests fine with Octave 3.8.1.

comment:10 Changed 5 years ago by dimpase

by the way, do you understand the need to create Sage matrices in _matrix as elements of MatrixSpace rather than just matrix ?

comment:11 Changed 5 years ago by vdelecroix

Why not? And please do not try to touch to everything as otherwise it will be impossible to review.

comment:12 Changed 5 years ago by dimpase

The difference is not big, but there is one place I don't understand, and moreover it does not work with complex matrices. Namely, in _matrix there is the following change:

         from sage.matrix.all import MatrixSpace
-        return MatrixSpace(R, nrows, ncols)(w)
+        s = str(self).strip()
+        v = s.split('\n ')
+        nrows = len(v)
+        if nrows == 0:
+            return MatrixSpace(R,0,0)(0)
+        ncols = len(v[0].split())
+        M = MatrixSpace(R, nrows, ncols)
+        v = sum([[x for x in w.split()] for w in v], [])
+        return M(v)

moreover it doesn't work on SMC either, so I'd just skip it, unless I understand when one should return 0.

comment:13 Changed 5 years ago by vdelecroix

Anyway, this whole octave interface is very broken. Namely floating point are passed through strings which is wrong from the begining. The approach taken in Oct2Py (that is writing files with complete information and building numpy matrices from them) is much better! Though a bit slow.

comment:14 Changed 5 years ago by dimpase

  • Branch changed from u/vdelecroix/21135 to public/21135
  • Commit changed from 138920133555e167a5776756f39e9ebbcd4cee57 to 70e2c125ad26e98e53d5f50bc9cafda93605bd92

This is the mix of your changes and Bill Page's changes that seems to make sense to me.


New commits:

4ac06e6improved octave interface from https://github.com/billpage/sage-octave
70e2c12update docs, ensure clean exit (so that sage-cleaner exists cleanly)

comment:15 Changed 5 years ago by vdelecroix

Let me have a look. (however, I discussed the approach taken here using the version of octave in #20382. Julian claims that it was not a reasonable strategy)

comment:16 Changed 5 years ago by vdelecroix

Why did you reintroduce sage-native-execute? This is useless.

comment:17 follow-up: Changed 5 years ago by vdelecroix

Why did you remove the --no-gui that is exactly the point of this ticket!?

comment:18 in reply to: ↑ 17 ; follow-up: Changed 5 years ago by dimpase

Replying to vdelecroix:

Why did you remove the --no-gui that is exactly the point of this ticket!?

Oops, sorry, I tested on 4.0.0, and it worked... :-) OK, let me put it back in.

comment:19 follow-up: Changed 5 years ago by vdelecroix

BTW could you check what gives $ octave --no-gui with your 3.X.Y version? (I would expect a return code 1)

comment:20 in reply to: ↑ 18 ; follow-up: Changed 5 years ago by vdelecroix

Replying to dimpase:

Replying to vdelecroix:

Why did you remove the --no-gui that is exactly the point of this ticket!?

Oops, sorry, I tested on 4.0.0, and it worked... :-) OK, let me put it back in.

I don't mind if it works but you should document what you did and remove the --no-gui stuff that is now ignored.

comment:21 Changed 5 years ago by vdelecroix

Note that for me the following line

$ octave --no-line-editing --silent --eval 'PS2(PS1());more off' --persist

does launch the GUI.

comment:22 Changed 5 years ago by vdelecroix

You should add a linebreak before _eval_line

comment:23 follow-up: Changed 5 years ago by vdelecroix

What is the point of print("CntrlC: Interrupting %s..."%self) in _keyboard_interrupt?

comment:24 Changed 5 years ago by vdelecroix

This

raise pexpect.ExceptionPexpect( "THIS IS A BUG -- PLEASE REPORT. This should never happen.\n" + msg)

would better be a RuntimeError.

comment:25 follow-up: Changed 5 years ago by vdelecroix

Why

            #self._expect.expect(self._prompt)
            #self._expect.expect(self._prompt)

in _keyboard_interrupt?

comment:26 in reply to: ↑ 19 Changed 5 years ago by dimpase

Replying to vdelecroix:

BTW could you check what gives $ octave --no-gui with your 3.X.Y version? (I would expect a return code 1)

looks as if it just works:

$ octave --no-gui
GNU Octave, version 3.8.1
Copyright (C) 2014 John W. Eaton and others.
This is free software; see the source code for copying conditions.
There is ABSOLUTELY NO WARRANTY; not even for MERCHANTABILITY or
FITNESS FOR A PARTICULAR PURPOSE.  For details, type 'warranty'.

Octave was configured for "x86_64-pc-linux-gnu".

Additional information about Octave is available at http://www.octave.org.

Please contribute if you find this software useful.
For more information, visit http://www.octave.org/get-involved.html

Read http://www.octave.org/bugs.html to learn how to submit bug reports.
For information about changes from previous versions, type 'news'.

octave:1> 

comment:27 in reply to: ↑ 25 Changed 5 years ago by dimpase

Replying to vdelecroix:

Why

            #self._expect.expect(self._prompt)
            #self._expect.expect(self._prompt)

in _keyboard_interrupt?

that's not me... OK, sure, it'd be removed.

comment:28 in reply to: ↑ 20 Changed 5 years ago by dimpase

Replying to vdelecroix:

Replying to dimpase:

Replying to vdelecroix:

Why did you remove the --no-gui that is exactly the point of this ticket!?

Oops, sorry, I tested on 4.0.0, and it worked... :-) OK, let me put it back in.

I don't mind if it works but you should document what you did and remove the --no-gui stuff that is now ignored.

there is also an issue with DISPLAY --- if it is unset, then --no-gui comes into force implicitly...

comment:29 in reply to: ↑ 23 Changed 5 years ago by dimpase

Replying to vdelecroix:

What is the point of print("CntrlC: Interrupting %s..."%self) in _keyboard_interrupt?

I presume this is meant to show this message, as interrupting might take a while...

comment:30 Changed 5 years ago by git

  • Commit changed from 70e2c125ad26e98e53d5f50bc9cafda93605bd92 to 0f9c86abad95c432d1dca10a26b7aba8525e7b5d

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

0f9c86ause octave-cli and the cleanup as suggested; fix octave_console()

comment:31 Changed 5 years ago by dimpase

octave-cli is the more local thing to call...

comment:32 Changed 5 years ago by git

  • Commit changed from 0f9c86abad95c432d1dca10a26b7aba8525e7b5d to e01d07511d6e6f2bd870313fa365374355bf06f4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e01d075use octave-cli and the cleanup as suggested; fix octave_console()

comment:33 Changed 5 years ago by dimpase

My proposal (in the current ticket branch) is to call octave-cli instead of octave, which always launches octave-gui (according to my tests on Fedora 23 with octave 4.0.1). octave-cli is available for octave versions 3 and 4, so this is a simplification too.

comment:34 Changed 5 years ago by dimpase

  • Milestone changed from sage-7.3 to sage-7.4

comment:35 Changed 5 years ago by vdelecroix

  • Authors changed from Vincent Delecroix to Dima Pasechnik
  • Status changed from needs_review to needs_work

If you use octave-cli then you can get rid of --no-gui.

As you do not use the prompt variable you would better remove it.

Since your code does not depend on octave version you would better get rid of my get_octave_version.

comment:36 follow-up: Changed 5 years ago by charpent

  • Cc charpent added

This bug forbids full checking (by make ptestlong) of a proposed patch, since such a test currently fails when octave>4.0.0 is installed : octave starts a GUI, doesn't return and the doctest times out.

FYI, the following diff (a four-letters patch...) against the current (7.4beta1) sage is sufficient to have sage passing the tests for external interfaces :

diff --git a/src/sage/interfaces/octave.py b/src/sage/interfaces/octave.py
index cfbbceb..5b2ce36 100644
--- a/src/sage/interfaces/octave.py
+++ b/src/sage/interfaces/octave.py
@@ -184,7 +184,7 @@ class Octave(Expect):
         Expect.__init__(self,
                         name = 'octave',
                         prompt = '>',
-                        command = "sage-native-execute octave --no-line-editing --silent",
+                        command = "sage-native-execute octave-cli --no-line-editing --silent",
                         server = server,
                         server_tmpdir = server_tmpdir,
                         script_subdirectory = script_subdirectory,

Proof (I wanted to check fricas 1.2.7 before reviewing #21231) :

charpent@SAP5057241:/usr/local/sage-7$ sage -t --optional=sage,octave,fricas $(sage -root)/src/sage/doctest/external.py 
Running doctests with ID 2016-08-19-16-01-10-87c81a5b.
Git branch: quickoctavefix
Using --optional=fricas,octave,sage
Doctesting 1 file.
sage -t --warn-long 70.0 src/sage/doctest/external.py
    [38 tests, 38.24 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 38.3 seconds
    cpu time: 0.6 seconds
    cumulative wall time: 38.2 seconds

Could we please have something reviewable, useable (= allowing for make ptestlong to succeed when checking something else...) and dispatch aesthetic disputationes to another ticket ?

comment:37 in reply to: ↑ 36 ; follow-up: Changed 5 years ago by dimpase

Replying to charpent:

This bug forbids full checking (by make ptestlong) of a proposed patch, since such a test currently fails when octave>4.0.0 is installed : octave starts a GUI, doesn't return and the doctest times out.

where in the branch do you see octave called? octave-cli is called, yes. Unless you have SAGE_OCTAVE_COMMAND set. Certainly if you have it set to octave it will do not what you want. Unset it, then the tests should pass, I think.

Could we please have something reviewable, useable (= allowing for make ptestlong to succeed when checking something else...) and dispatch aesthetic disputationes to another ticket ?

Well, does this branch actually work for you? If not, I would like to know which test it fails, and the version of octave.

Last edited 5 years ago by dimpase (previous) (diff)

comment:38 in reply to: ↑ 37 ; follow-up: Changed 5 years ago by charpent

My mail answer to the Trac server didn't make it, it seems...

Replying to dimpase:

Comment (by dimpase):

Replying to charpent:

[ Snip ]

This bug forbids full checking (by make ptestlong) of a proposed patch,

since such a test currently fails when octave>4.0.0 is installed : octave starts a GUI, doesn't return and the doctest times out.

where in the branch do you see octave called? octave-cli is called, yes. Unless you have SAGE_OCTAVE_COMMAND set. Certainly if you have it set to octave it will do not what you want. Unset it, then the tests should pass, I think.

Think again :

charpent@SAP5057241:~$ sage -sh

Starting subshell with Sage environment variables set.  Don't forget
to exit when you are done.  Beware:
 * Do not do anything with other copies of Sage on your system.
 * Do not use this for installing Sage packages using "sage -i" or for
   running "make" at Sage's root directory.  These should be done
   outside the Sage shell.

Bypassing shell configuration files...

Note: SAGE_ROOT=/usr/local/sage-7
(sage-sh) charpent@SAP5057241:~$ echo $SAGE_OCTAVE_COMMAND

(sage-sh) charpent@SAP5057241:~$ which octave
/usr/bin/octave
(sage-sh) charpent@SAP5057241:~$ octave

[ Octave starts its GUI. I exit from it]

(sage-sh) charpent@SAP5057241:~$ exit
exit
Exited Sage subshell.
charpent@SAP5057241:~$ echo $SAGE_OCTAVE_COMMAND

charpent@SAP5057241:~$

Ergo SAGE_OCTAVE_COMMAND is not set neither in the Sage shell neither out of it.

A "make ptestlong" on this machine FAILS with a timeout on ...doctest/external.py", leaving an unclosable octave gui (you have to kill -TERM it...). From my check of 7.4beta0 :

...
sage -t --long --warn-long 50.8 src/sage/doctest/external.py
    Timed out
**********************************************************************
...
**********************************************************************
----------------------------------------------------------------------
sage -t --long --warn-long 50.8 src/sage/doctest/external.py  # Timed out
----------------------------------------------------------------------
Total time for all tests: 8702.9 seconds
    cpu time: 11743.1 seconds
    cumulative wall time: 14507.8 seconds

The octave documentation does not hint at any way of making the CLI interface the default (all it does provide is the --no-gui option and the octave-cli binary). Therefore, this fix is somewhat urgent for people having octave>4.0.0 installed...

[ Snip again... ]

Could we please have something reviewable, useable (= allowing for make ptestlong to succeed when checking something else...) and dispatch aesthetic disputationes to another ticket ?

I am devasted to have to insist...

Emmanuel Charpentier

comment:39 in reply to: ↑ 38 ; follow-up: Changed 5 years ago by dimpase

Replying to charpent:

I can only conjecture that you have not pulled the right branch, public/21135 before running your tests.

Could you please re-check? The change you proposed is equivalent to the branch in the sense that no octave is called, only octave-cli.

As well, could you please check that octave-cli does not launch GUI for you.

comment:40 Changed 5 years ago by git

  • Commit changed from e01d07511d6e6f2bd870313fa365374355bf06f4 to 1713d749ed0de36dc2ead83cfdb2b9a4ca2c000d

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

181974fMerge branch 'public/21135' of git://trac.sagemath.org/sage into octafix
1713d74removed unused prompt and unneeded option

comment:41 in reply to: ↑ 39 ; follow-up: Changed 5 years ago by charpent

Replying to dimpase:

Replying to charpent:

I can only conjecture that you have not pulled the right branch, public/21135 before running your tests.

I have NOT pulled #21135 : my diff was against the current (pristine (unmodified)) 7.4beta1. My point was that a four-letters patch was enough to solve my current difficulty (i. e. breaking something that used to work), and that the other proposed modificatins (which aim at enhancing something that used to work) are a separate issue...

Could you please re-check? The change you proposed is equivalent to the branch in the sense that no octave is called, only octave-cli.

No point. See above.

As well, could you please check that octave-cli does not launch GUI for you.

It does not.

HTH...


New commits:

181974fMerge branch 'public/21135' of git://trac.sagemath.org/sage into octafix
1713d74removed unused prompt and unneeded option

comment:42 Changed 5 years ago by dimpase

  • Status changed from needs_work to needs_review

well, let's go with the review then! I left the new version function in, just in case. It does no harm.

comment:43 in reply to: ↑ 41 Changed 5 years ago by dimpase

Replying to charpent:

Replying to dimpase:

Replying to charpent:

I can only conjecture that you have not pulled the right branch, public/21135 before running your tests.

I have NOT pulled #21135 : my diff was against the current (pristine (unmodified)) 7.4beta1. My point was that a four-letters patch was enough to solve my current difficulty (i. e. breaking something that used to work), and that the other proposed modificatins (which aim at enhancing something that used to work) are a separate issue...

My apologies, I thought you were saying that you tried this branch and it didn't work. I'm very stressed today, in a bad way...

comment:44 Changed 5 years ago by vdelecroix

  • Status changed from needs_review to needs_work

You did not address comment:35. And there is no need to check for the version in __init__.

comment:45 Changed 5 years ago by git

  • Commit changed from 1713d749ed0de36dc2ead83cfdb2b9a4ca2c000d to d9be1de1bd63dd0bf2aa1a9cf4e47f69c5e2194c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d9be1deremoved unused prompt and unneeded option (improved)

comment:46 Changed 5 years ago by dimpase

  • Status changed from needs_work to needs_review

OK, sorry for being unable to see that in

if A:
   foo
if B:
   xfoo

one has foo==foo... Duh. Fixed.

comment:47 Changed 5 years ago by vdelecroix

Could you remove the function get_octave_version that was introduced in my first commit completely?

comment:48 Changed 5 years ago by vdelecroix

(it is fine to keep octave_version with the deprecation introduced in the same commit)

comment:49 Changed 5 years ago by git

  • Commit changed from d9be1de1bd63dd0bf2aa1a9cf4e47f69c5e2194c to fb30497f15445323af55b7e0579861fe48d7886a

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fb30497removed unused prompt and unneeded option (improved x 2)

comment:50 Changed 5 years ago by dimpase

done.

comment:51 Changed 5 years ago by vdelecroix

all right. I am testing it.

comment:52 Changed 5 years ago by vdelecroix

to make it cleaner, I would remove

options = " --no-line-editing --silent"

and change

command = command + options + " --eval 'PS2(PS1());more off' --persist",

to

command = command + " --no-line-editing --silent--eval 'PS2(PS1());more off' --persist",

comment:53 Changed 5 years ago by git

  • Commit changed from fb30497f15445323af55b7e0579861fe48d7886a to 1ed88aef15cae31754965a9a6eb2de55667ca5c4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

1ed88aeremoved unused prompt and unneeded option (improved x 3)

comment:54 Changed 5 years ago by dimpase

ok.

comment:55 Changed 5 years ago by vdelecroix

octave process leave some annoying "octave-workspace" file. Would be cool to get rid of that when exiting octave. (Though not for this ticket)

comment:56 Changed 5 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

Thanks!

comment:57 Changed 5 years ago by vbraun

  • Branch changed from public/21135 to 1ed88aef15cae31754965a9a6eb2de55667ca5c4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.