bugGNU Octave - Bugs: bug #60886, Command line option '--gui'...


bug #60886: Command line option '--gui' ignored when used together with '--path'

Submitter:  None
Submitted:  Tue 06 Jul 2021 01:56:43 PM UTC
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Lando Nachtmann Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 6.2.0
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment (Rich Markup)

Comment Type & Canned Response:


( Jump to the original submission )

Thu 22 Jul 2021 05:56:38 PM UTC, comment #13: 

I pushed one more change on stable to avoid duplicate error messages about unrecognized options


I also merged with default.  Otherwise, I think it is working correctly now.  Thanks.

John W. Eaton <jwe>
Project Administrator
Thu 22 Jul 2021 05:16:12 PM UTC, comment #12: 

I pushed a patch that removes the impossible(?) case statement here:

At the moment, `main.cc` still does what you describe.
IIUC, it passes all arguments (minus the ones that were already removed before the changes here) on to the "actual" program. I was assuming the the `default:` case would include unrecognized option characters or anything alike. Is this not the case?

Since `octave-{gui,cli}` will always need to check for errors in their arguments anyway, there is probably not much point in duplicating those checks in the wrapper imho. But I might be missing something.

What is still missing to fix the wrapper program to ignore getopt errors?

Markus Mützel <mmuetzel>
Project Member
Wed 21 Jul 2021 06:22:37 PM UTC, comment #11: 

OK, yes, I don't think we need to match 0.  The docs say it returns -1 if no more options are available (there may still be non-option arguments) and '?' if an unrecognized option character or a missing option argument is found.

As I remember, the original intent of the option handling in main.cc was to do minimal processing, detect errors that would make it impossible for the wrapper program to decide what to do, but otherwise pass the arguments on to the real program (octave-cli or octave-gui) even if there were some arguments that didn't make sense.  But now maybe getopt in main.cc could be detecting other errors?  If so, I guess we need to decide whether to handle the errors in the wrapper program and octave-{gui,cli} or try to fix the wrapper program to ignore getopt errors and just pass the options on to octave-{gui,cli}.

John W. Eaton <jwe>
Project Administrator
Wed 21 Jul 2021 06:02:58 PM UTC, comment #10: 

Hmmm. I thought I've seen getopt return 0 if an option was passed that didn't match any of the parameters. My thinking was that this should be treated as a command.

But I can't reproduce that now.

When I first did that, I added a bunch of `std::cout` to see what is happening. I must have mixed some of the output up...
We can probably remove the `case 0:`.

Markus Mützel <mmuetzel>
Project Member
Wed 21 Jul 2021 05:51:45 PM UTC, comment #9: 

The merge looks right to me.

What does it mean if getopt returns 0?  It appears to be handled the same as the EVAL_OPTION case but I don't understand why.

John W. Eaton <jwe>
Project Administrator
Wed 21 Jul 2021 12:48:35 PM UTC, comment #8: 

I grafted the changes to stable here:

There were quite a few conflicts when grafting and then again when merging to default. I hope I did that correctly.

Marking as ready for test.

Markus Mützel <mmuetzel>
Project Member
Mon 19 Jul 2021 07:17:43 PM UTC, comment #7: 

It's fine with me to make this change on stable as well.  I avoided using getopt originally because I didn't anticipate doing much option parsing.  I thought it would be simple enough to detect a few key options and then pass everything on to the real programs.

John W. Eaton <jwe>
Project Administrator
Sun 18 Jul 2021 02:03:31 PM UTC, comment #6: 

I split the header "options-usage.h" into two headers "options.h" and "usage.h" to avoid the unused function warnings.

Since this is a pretty important change to how Octave starts, I felt like it would be best to make that change on the default branch here:

For the stable branch, should we just revert this changeset to fix the regression?

But that would mean that we would re-introduce the possible race condition that lead to bug #59628.

Alternatively, we could try and graft this change to the stable branch if we feel confident that it'll be ok.
Or leave it as it is on stable...

What do you think?

Markus Mützel <mmuetzel>
Project Member
Sun 11 Jul 2021 11:13:50 AM UTC, comment #5: 

The attached patch changes the "main.cc" wrapper program to use "getopt" instead of our own command line arguments parsing. (I only did very limited testing so far. But it fixes the original use case described in comment #0 for me.)

Since this is very similar to what is done in octave.cc, I thought we could re-use the same functions.

Including "options-usage.h" in "main.in.cc" however means that the following warnings are emitted when building:

  GEN      src/main.cc
  CXX      src/octave-main.o
config.status: creating build-aux/subst-script-vals.sh-tmp
config.status: executing build-aux/subst-script-vals.sh commands
In file included from src/main.cc:83:
../libinterp/options-usage.h:199:1: warning: ‘void octave_print_version_and_exit()’ defined but not used [-Wunused-function]
  199 | octave_print_version_and_exit (void)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../libinterp/options-usage.h:191:1: warning: ‘void octave_print_terse_usage_and_exit()’ defined but not used [-Wunused-function]
  191 | octave_print_terse_usage_and_exit (void)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../libinterp/options-usage.h:132:1: warning: ‘void octave_print_verbose_usage_and_exit()’ defined but not used [-Wunused-function]
  132 | octave_print_verbose_usage_and_exit (void)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  CXXLD    src/octave

Should we try and silence those warnings? What would be the best strategy for that?

Since this is a change that might have a larger impact, I was targeting the default branch.
However, this is also a regression from Octave 5. Should it land on stable instead?

(file #51661)

Markus Mützel <mmuetzel>
Project Member
Tue 06 Jul 2021 04:18:52 PM UTC, comment #4: 

It's a bug in the main.cc wrapper program.  I guess we could use getopt there instead of rolling our own argument processing.  Then we could easily handle the --foo OPT and --foo=OPT syntax for all arguments that accept option arguments.  I didn't expect the argument processing in the wrapper program to become this complex.

John W. Eaton <jwe>
Project Administrator
Tue 06 Jul 2021 04:10:59 PM UTC, comment #3: 
Markus Mützel <mmuetzel>
Project Member
Tue 06 Jul 2021 04:03:47 PM UTC, comment #2: 

That is most probably due to this pattern in the `main` function in main.in.cc:

      else if (! strcmp (argv[i], "--eval") ||
               (strlen (argv[i]) > 0 && argv[i][0] != '-'))
          eval_code = true;
          new_argv[k++] = argv[i];


  if (start_gui && eval_code && ! persist_octave)
    start_gui = false;

So, if any of the command line parameters does not start with "-" it is treated as code to be evaluated. `eval_code` is set to `true` in that case.
The next condition overrides the GUI setting if `eval_code` is `true`.

I'm not sure if this is an error in that function or in the documentation. (The syntax with `=` is working.)

Markus Mützel <mmuetzel>
Project Member
Tue 06 Jul 2021 03:26:47 PM UTC, comment #1: 

Confirmed here.

It works for me with the following syntax:

$ octave --gui --path="/my/octave/functions/path"

Not technically a bug in the interpreter. But setting to that category as the next best match...

Markus Mützel <mmuetzel>
Project Member
Tue 06 Jul 2021 01:56:43 PM UTC, original submission:  


$ octave --gui

starts Octave in GUI mode as expected,

$ octave --gui --path /my/octave/functions/path

does not.



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

Attach Files:

Attached Files
file #51661:  bug60886_getopts.patch added by mmuetzel (10KiB - application/octet-stream)


Depends on the following items: None found

Items that depend on this one: None found


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

    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 project members can vote.





    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-07-29 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2021-07-21 mmuetzel StatusIn Progress Ready For Test
    2021-07-11 mmuetzel Attached File- Added bug60886_getopts.patch, #51661
        StatusConfirmed In Progress
        Operating SystemGNU/Linux Any
    2021-07-06 mmuetzel CategoryNone Interpreter
        Item GroupNone Incorrect Result
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.9