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

 
 

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

Submitted by:  None
Submitted on:  Thu 01 Feb 2018 01:37:05 PM UTC  
 
Category: GUISeverity: 3 - Normal
Priority: 5 - NormalItem Group: None
Status: Patch ReviewedAssigned to: None
Originator Name: jmcclureOriginator Email: -unavailable-
Open/Closed: OpenRelease: dev
Operating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

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>
Project Administrator
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>
Project Administrator
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:

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>
Project 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:

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

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:

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

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

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

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>
Project Administrator
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>
Project Administrator
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>
Project Administrator
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

I made a test script launch.sh with

but still no luck.

Rik <rik5>
Project 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>
Project Administrator
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:

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:

Mike Miller <mtmiller>
Project Administrator
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>
Project 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>
Project Administrator
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

Test #2 gives strange results

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

(file #43153)

Rik <rik5>
Project 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

Mike Miller <mtmiller>
Project Administrator
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.

Anonymous

 

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

Attach File(s):
   
   
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
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by aphirst
  • -unavailable- added by rik5 (Updated the item)
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by None (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 05 Feb 2018 06:42:33 PM UTCmtmillerAttached File-=>Added octave-gui-stdin-logic.patch, #43179
    Sat 03 Feb 2018 12:30:34 PM UTCaphirstCarbon-Copy-=>Added -unavailable-
    Thu 01 Feb 2018 05:54:52 PM UTCrik5Attached File-=>Added 53034.cset, #43153
      StatusPatch Submitted=>Patch Reviewed
    Thu 01 Feb 2018 05:30:22 PM UTCmtmillerStatusNone=>Patch Submitted
    Thu 01 Feb 2018 01:38:24 PM UTCNoneAttached File-=>Added check_stdin.patch, #43149

    Back to the top


    Powered by Savane 3.1-cleanup1