bugGNU Octave - Bugs: bug #53034, Octave GUI fails to open when...

 
 

bug #53034: Octave GUI fails to open when stdin closed. Patch included

Submitter:  None
Submitted:  Thu 01 Feb 2018 01:37:05 PM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  jmcclure Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * dev
Operating System:  * GNU/Linux Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 20 Mar 2018 02:58:06 AM UTC, comment #16: 

I think this can be considered fixed. For the sake of consistency, as derived from this discussion, Octave 4.4 will act as a command interpreter with no GUI unless otherwise directed. The only way to start the Octave GUI will be "octave --gui", or "octave --force-gui" to remain backwards compatible. The state of stdin has no bearing now on whether the GUI is started.

Mike Miller <mtmiller>
Group Member
Tue 06 Feb 2018 06:57:37 AM UTC, comment #15: 

I just caught up on some additional comments on the Arch forum. I think the most meaningful point is that it may be counter-intuitive that 'octave' only automatically starts the GUI when it is run from a terminal, while it doesn't start the GUI when it is not run from a terminal, which may be when some users want the GUI most.

While octave is obviously not the only command interpreter to automatically switch to an interactive REPL, and is also not the only program to automatically switch between a graphical and a text-based mode, it may be one of the only ones that tries to juggle all of those roles at once from a single command. Maybe octave tries to be too clever for its own good.

One might pose the question: if most Octave GUI users must start it with 'octave --force-gui' from some kind of a launcher or menu, then what set of users are served by starting the GUI automatically from 'octave' in a terminal?

Mike Miller <mtmiller>
Group Member
Tue 06 Feb 2018 05:39:02 AM UTC, comment #14: 

Yes, I think that is a decent summary, thanks jwe.

We have had additional discussion over on the Arch forums, and I would venture to say we've had a slight breakthrough in communications. I have been able to explain that we want octave to behave primarily as a command interpreter, with behavior similar to the bash or python interpreters.

If octave can somehow determine that the user intends to run octave "interactively", whatever that means, then octave should start a REPL (which may be a GUI-based REPL). So far the only way we have to test for implicit interactivity is to check that stdin is a terminal.

Now that octave has included a GUI for a few releases, we can certainly revisit and have a discussion on

  • what the default behavior should be in different execution environments,
  • whether the 'octave-gui' executable should be in PATH or obscured as it is now,
  • whether 'octave --force-gui' is better or worse than a separate 'octave-gui' command,
  • whether the GUI should become the implicit default and we should introduce a new command or command-line option for batch processing,
  • and so on.


Mike Miller <mtmiller>
Group Member
Tue 06 Feb 2018 05:18:47 AM UTC, comment #13: 

Taking a step back, I think maybe we should define what we want to do.  As I understand it, we'd like to have octave start "from window manager launchers" (whatever that means) and also we want to be able to execute any of the following at a shell prompt and have them work as described:


## Read command from named file:
octave FILE

## Read commands from stdin, redirected from a file:
octave < FILE

## Read commands from a pipe:
echo "some commands" | octave

## Start Octave interactively, reading input from a terminal:
octave


The check on "isatty (stdin)" works perfectly well to distinguish reading stdin redirected from a file or from a pipe vs reading commands interactively from a terminal, at least in the cases shown above.

As I understand it, Mike is saying that the way some window manager launchers start programs like Octave is indistinguishable  from one or more of the above, at least using methods we know about.  So what can we do to detect the case of Octave being started by some window manager launcher?  It must also allow all of the ways of running Octave shell commands to continue to work.  If it's possible to do that, then we can eliminate the --force-gui option.  Otherwise, I think we are stuck with it, because we are not going to give up the ability to run commands as above.

Or, am I misunderstanding the issues here?

Finally, there's no need to be rude.  Telling (whether explicitly, or implicitly by your rudeness) that you think we are idiots is not going to help fix the problem.

John W. Eaton <jwe>
Group administrator
Mon 05 Feb 2018 08:04:45 PM UTC, comment #12: 

Mike,

That is quite disengenuous.

The edit to include character devices is precisely what is making that code useless.

But congrats, now that it is completely useless, by all means include it - it will fit right in.

Anonymous
Mon 05 Feb 2018 06:42:33 PM UTC, comment #11: 

I am attaching a new patch that applies to both default and stable. This patch adds a more exhaustive set of checks on standard input, almost exactly as has been suggested by our friends on the Arch forums. Octave checks whether its standard input is open and valid, is a terminal, or is a regular file, pipe, socket, or character stream. The only change I made was the inclusion of character devices as valid inputs.

There is also plenty of debug output to show what checks are being performed and what the results are. Octave also queries the proc filesystem to try to determine what the file name of its input is.

I have tested this patch while attempting to run octave without the --force-gui option from various graphical launcher tools in the GNOME desktop, the i3 window manager, and the Openbox window manager.

In all cases I have tested, Octave without --force-gui does not start. So this additional checking has no practical effect. The debugging will hopefully demonstrate why.

In the GNOME desktop, running from either a .desktop file launcher or from the Alt+F2 "run something" prompt, the following is reported to the gnome session log:


DEBUG: is_readable_input: /proc/self/fd/0 -> /dev/null
DEBUG: is_readable_input -> true: is a valid input file type


In the Openbox window manager, running Octave from an openbox-menu custom entry, the following is shown in the session log:


DEBUG: is_readable_input: /proc/self/fd/0 -> /dev/null
DEBUG: is_readable_input -> true: is a valid input file type


The same is reported when I run octave from the 'gmrun' command line launcher, as used by the original poster on the Arch forums.

In the i3 window manager, running Octave from the 'dmenu' launcher/menuing system, the following is shown in the session log:


DEBUG: is_readable_input: /proc/self/fd/0 -> pipe:[2152967]
DEBUG: is_readable_input -> true: is a valid input file type


This difference is because dmenu executes a command by echoing the command line to the user's shell for shell interpolation. In this situation, octave inherts the standard input of bash, which is the pipe from the echo command. The same can be reproduced with


$ echo octave | bash
DEBUG: is_readable_input: /proc/self/fd/0 -> pipe:[2547526]
DEBUG: is_readable_input -> true: is a valid input file type


Under GNOME and Openbox, running 'octave' from a menu entry, graphical launcher, .desktop shortcut, all are indistinguishable from running octave as


$ octave < /dev/null


Using dmenu in the i3 window manager, running 'octave' is indistinguishable from running octave as


$ echo octave | bash


I don't have enough tuits at the moment to test more window managers, but I would be interested to see the results if others are willing to build octave with this patch and test under their favorite WM or DE.

So far I have not seen a situation where octave is started with its standard input closed.

(file #43179)

Mike Miller <mtmiller>
Group Member
Thu 01 Feb 2018 07:13:29 PM UTC, comment #10: 

I'm sorry but I think your reading of the GLib code is incorrect. If you read the GLib docs or actually try it yourself, you will see that the child process has stdin opened to read from /dev/null by default. There are options to have the child inherit the parent's stdin file descriptor or to pass one end of a newly created fifo, which the parent can then close if they want to. But all uses of close_and_invalidate having to do with stdin in GLib are simply internal cleanup.

But this is starting to digress.

To stay on topic, it would help us fix this issue if someone could provide a working patch that has actually been tested in a real windowing environment and that demonstrates a concrete improvement over the status quo.

Mike Miller <mtmiller>
Group Member
Thu 01 Feb 2018 06:55:34 PM UTC, comment #9: 

Mike is correct - and he is saying what I've said from the beginning.

But first some sad corrections: the g_spawn functions to not redirect /dev/null to stdin, that's nonsense.  They have options to redirect stdout and stderr to /dev/null, but stdin is simply closed by a call to close_and_invalidate which in turn calls g_close which in turn calls close on the file descriptor.

close_and_invalidate further ensures that the stdin filenumber is set to -1.

So my initial patch will make the currently horrible assumption slightly less horrible.  But like I said in the beginning, it is not the right solution.  There is no way to polish the turd of an idea of assuming that a failure of isatty means that stdin is a file.  That's just wrong from the start.

If you want to know if stdin is a file, just check if it's a file.  You may need a octave_isfile_wrapper or perhaps octave_isfilelike_wrapper if you need to ensure cross OS support, and I can't help with non-POSIX systems, but the POSIX version of octave_isfile_wrapper would just call stat, check for EBADF, and on success check a couple stat macros S_IS*

But shrug, I don't care how this goes.  I don't use octave and avoid gnu tools in general.  I thought I'd give you a heads up on what is plainly a very bad assumption in your code (that the failure of isatty implies it must be a file).  Do with it what you will.

Anonymous
Thu 01 Feb 2018 06:54:16 PM UTC, comment #8: 

When I apply the change I don't get a parse error, I just get an exit with success, same as without the patch applied.

Jesse, can you please provide a more complete patch that uses the fstat approach that you suggested? The patch that you have provided doesn't seem to actually change the logic in any meaningful way.

Mike Miller <mtmiller>
Group Member
Thu 01 Feb 2018 06:49:07 PM UTC, comment #7: 

Setting the SHELL variable made no difference. 

I agree that the syntax appears to be correct and I also tried


octave 0<&-


I made a test script launch.sh with


#!/bin/sh
exec 0<&-

octave


but still no luck.

Rik <rik5>
Group administrator
Thu 01 Feb 2018 06:44:26 PM UTC, comment #6: 

The full implementation here should check that fstat fails and that the value of errno is EBADF.


Mike Miller <mtmiller>
Group Member
Thu 01 Feb 2018 06:41:18 PM UTC, comment #5: 

No, I actually just wrote a little C test program and I don't think this change actually helps at all:


$ cat checkstdin.c
#include <stdio.h>

int
main (int argc, char *argv[])
{
  printf ("fileno (stdin) == %d\n", fileno (stdin));
  return 0;
}
$ ./checkstdin
fileno (stdin) == 0
$ ./checkstdin 0<&-
fileno (stdin) == 0


So I don't see any difference, fileno(stdin) returns 0 whether stdin is open or closed.

The forum discussion also suggested using fstat (or octave_fstat_wrapper in our case) to test whether the file descriptor is valid. That does do something useful for me:


$ cat checkstdin.c
#include <stdio.h>
#include <sys/stat.h>

int
main (int argc, char *argv[])
{
  struct stat buf;
  printf ("fileno (stdin) == %d\n", fileno (stdin));
  printf ("fstat (fileno (stdin)) == %d\n", fstat (fileno (stdin), &buf));
  return 0;
}
$ ./checkstdin
fileno (stdin) == 0
fstat (fileno (stdin)) == 0
$ ./checkstdin </dev/null
fileno (stdin) == 0
fstat (fileno (stdin)) == 0
$ ./checkstdin <&-
fileno (stdin) == 0
fstat (fileno (stdin)) == -1


Mike Miller <mtmiller>
Group Member
Thu 01 Feb 2018 06:33:11 PM UTC, comment #4: 

I use tcsh, but I knew that syntax would be a problem so I dropped in to bash before running the command.  But, now that I think about it, the shell variable SHELL may still be set to "/bin/tcsh" which could cause problems.  I'll check whether modifying that makes this work.

Rik <rik5>
Group administrator
Thu 01 Feb 2018 06:12:15 PM UTC, comment #3: 

Rik, remind me do you use csh instead of bash? That redirect syntax may be no good for you.

Mike Miller <mtmiller>
Group Member
Thu 01 Feb 2018 05:54:52 PM UTC, comment #2: 

Attached is an updated patch with a commit message.  I built and installed a development version of Octave in order to run the three tests in comment #1.  Tests #1 and #3 pass just fine.  Using a pipe also works correctly as in


echo 'disp (pi)' | octave
 3.1416


Test #2 gives strange results


octave <&-
parse error:

  invalid character '' (ASCII 17)

>>> ����2*a�!����>N�{8��ψL�$�M�)!��{�Ug���,���
    ^


Is there another way to verify that this works when stdin is closed?


(file #43153)

Rik <rik5>
Group administrator
Thu 01 Feb 2018 05:30:22 PM UTC, comment #1: 

I also glanced at the forum discussion last week. I have no problem with the proposed change, but I doubt it will fix anything for a great number of users.

I assume you are referring to the GLib g_spawn family of functions? In particular, those functions set stdin to /dev/null by default, unless the users chooses for the child to inherit stdin or to receive a fifo as stdin. I assume most window managers and desktop environments execute launchers with stdin set to /dev/null, which will still be considered by Octave to be an empty file input, not a closed file descriptor. But I have not tested every windowing environment to verify this.

The surest solution is to continue to use --force-gui when intending to run Octave's GUI.

But like I said at the start, I have no problem with the proposed change. The only way I know how to test whether this change will work is to run


$ octave             ## start GUI, stdin is a tty
$ octave <&-         ## start GUI, stdin is closed
$ octave </dev/null  ## exit with success, stdin is empty file


Mike Miller <mtmiller>
Group Member
Thu 01 Feb 2018 01:37:05 PM UTC, original submission:  

The Octave gui checks to see whether stdin is a tty prior to launching to gui itself.  It seems the goal is to check whether octave is being sent a script it should run in which case the gui need not launch.

The current implemented logic is, if stdin is a tty, run the gui, otherwise assume stdin is a file and do not run the gui.  This is rather absurd.  Check if stdin is a file, and if it is, not launch the gui, otherwise do so.

This faulty logic fails on all cases where stdin is neither a file nor a tty (e.g., when stdin has been closed by a launcher such as one of the gnu gspawn family).

This appeared on our distros forums here:
https://bbs.archlinux.org/viewtopic.php?id=233883

A gnu developer even chimed in, but seemed to take no note of the actual solution.  I see updates have been made in the intervening time, but this problem has not been address.

The following minimal patch with avoid the problem when stdin is closed.  For a longer term solution I'd encourage the above: if you want to know if stdin is a file, check if it is a file with `stat`; don't just check if it's a tty and assume it's a file otherwise.


--- a/octave-gui.cc
+++ b/octave-gui.cc
@@ -115,7 +115,7 @@
     // with no terminal) and you want to start the GUI, you may use the
     // --force-gui option to start the GUI.

-    if (! octave_isatty_wrapper (fileno (stdin)))
+    if (fileno(stdin) != -1 && !octave_isatty_wrapper (fileno (stdin)))
       return false;

     // If we have code to eval or execute from a file, and we are going
+verbatim+

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #43179:  octave-gui-stdin-logic.patch added by mtmiller (3KiB - application/octet-stream)
file #43153:  53034.cset added by rik5 (1KiB - application/octet-stream)
file #43149:  check_stdin.patch added by None (400B - text/x-patch - Patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by aphirst
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by None (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-03-20 mtmiller StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2018-02-05 mtmiller Attached File- Added octave-gui-stdin-logic.patch, #43179
    2018-02-03 aphirst Carbon-Copy- Added -email is unavailable-
    2018-02-01 rik5 Attached File- Added 53034.cset, #43153
        StatusPatch Submitted Patch Reviewed
    2018-02-01 mtmiller StatusNone Patch Submitted
    2018-02-01 None Attached File- Added check_stdin.patch, #43149

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code