bugGNU Octave - Bugs: bug #51708, multiple --eval options results in...

 
 

bug #51708: multiple --eval options results in corrupted parsing of command line

Submitted by:  Mike Miller <mtmiller>
Submitted on:  Wed 09 Aug 2017 07:41:25 PM UTC  
 
Category: InterpreterSeverity: 4 - Important
Priority: 5 - NormalItem Group: Regression
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating 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)

Thu 10 Aug 2017 01:07:03 AM UTC, comment #10:

I tediously reviewed the cset and I believe that the issue identified here of mixing char with char * occurred only once. I fixed it here http://hg.savannah.gnu.org/hgweb/octave/rev/4d4ba038d103.

For the testing, the advantage of staying within Octave is that the tests will be counted in the summary tallies at the end of fntests.log. A disadvantage may be trying to determine whether run-octave or regular octave should be used in the system command.

Rik <rik5>
Project Administrator
Wed 09 Aug 2017 11:28:16 PM UTC, comment #9:

Here's an example of a simple test that just runs "run-octave --version" and expects a success exit status.

If any test script fails, make check fails. The only weird bit is that it runs concurrently with the existing fntests.m test suite, but I'm sure we can work out a way to serialize them.

(file #41467)

Mike Miller <mtmiller>
Project Administrator
Wed 09 Aug 2017 11:16:58 PM UTC, comment #8:

Automake includes basic support for running series of test scripts or programs and collecting the exit statuses, that's the test framework I've used mostly with autotools in the past.

Autoconf includes Autotest, but that's just another m4-based macro language like Autoconf for building portable test scripts, seems like overkill for what we need.

Dejagnu seems to still be around, I've never used it.

I think most of the more fully-featured testing frameworks are going to be huge libraries built on perl, python, ruby, node, etc, and we probably don't want to add that bloat just to run some command-line permutations of the octave executable. I would lean towards just using Automake's built-in mechanism for running scripts.

Mike Miller <mtmiller>
Project Administrator
Wed 09 Aug 2017 10:59:46 PM UTC, comment #7:

Kind of clunky, but if we are only doing a few tests then we could use system() in an m-file. I just tried it and works.

Rik <rik5>
Project Administrator
Wed 09 Aug 2017 10:54:20 PM UTC, comment #6:

Tests for invoking Octave would be useful, but that will require a completely different testing mechanism. What modern testing framework could we use? Octave originally used dejagnu. I'd rather not go back to that if we can avoid it.

John W. Eaton <jwe>
Project Administrator
Wed 09 Aug 2017 09:48:43 PM UTC, comment #5:

I bisected independently and came to 336f89b6208b as the culprit as well.

Mike Miller <mtmiller>
Project Administrator
Wed 09 Aug 2017 09:26:41 PM UTC, comment #4:

I checked 23829 completely and it is fine (Eliminate unnecessary std::string ("...") constructor calls when "..." suffices.). The problem cset is very large and will take some time.

Rik <rik5>
Project Administrator
Wed 09 Aug 2017 09:19:16 PM UTC, comment #3:

Yeah I was thinking we could add some facility to the test directory to run octave in various ways to test command line options, etc.

Mike Miller <mtmiller>
Project Administrator
Wed 09 Aug 2017 09:07:26 PM UTC, comment #2:

Grrr. I made the change from " " to ' ' to be more specific and because there were certain cases, such as appending with +=, where it was faster. And then I made a second set of changes for readability where I removed the std::string () qualifying the bare string literal because it was unnecessary.

Again, this points out that we don't have a full set of regression tests because I ran 'make check' and it passed.

Can we add some invocation tests to the tests/ directory or is it really only running m-files and .tst files? For this case,

should return 2.

Rik <rik5>
Project Administrator
Wed 09 Aug 2017 08:43:27 PM UTC, comment #1:

It was this changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/336f89b6208b

octave_optarg_wrapper returns char*, not std::string, so adding that to ' ' is trouble.

I haven't looked to see whether there are other places where similar problems may have been introduced.

Adding Rik to the notification list.

John W. Eaton <jwe>
Project Administrator
Wed 09 Aug 2017 07:41:25 PM UTC, original submission:

Multiple --eval options should be combined into a single command string to be evaluated in sequence. This worked in all versions up to the current stable branch, but looks like it's regressed on the default branch.

Stable:

Default:

Depending on the contents of the --eval options, a different string is passed to the parser. It looks like it's coming from the environment. I think the environ array normally follows the argv array in memory.

Mike Miller <mtmiller>
Project Administrator

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #41467:  basic-test.diff added by mtmiller (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by jwe
  • -unavailable- added by mtmiller (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
    Thu 10 Aug 2017 03:44:55 PM UTCrik5StatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Thu 10 Aug 2017 01:07:03 AM UTCrik5StatusConfirmed=>Ready For Test
    Wed 09 Aug 2017 11:28:16 PM UTCmtmillerAttached File-=>Added basic-test.diff, #41467
    Wed 09 Aug 2017 09:07:26 PM UTCrik5StatusNone=>Confirmed
    Wed 09 Aug 2017 08:43:27 PM UTCjweCarbon-Copy-=>Added rik

    Back to the top


    Powered by Savane 3.1-cleanup1