bugGNU Octave - Bugs: bug #55023, gnuplot_binary test fails if...

 
 

bug #55023: gnuplot_binary test fails if there's no gnuplot and leaves incorrect configuration

Submitter:  Carnë Draug <carandraug>
Submitted:  Wed 14 Nov 2018 01:15:17 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  rik5
Originator Name:  Open/Closed:  * Closed
Release:  * 4.4.1 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 15 Nov 2018 11:59:09 AM UTC, comment #10: 

The change fixes the issue. I did make a further change that does use unwind_protect. Basically the code is the same, tests failures would still be reported but the cleanup code needs to only be written once (in the cleanup) instead of both in the try and and catch block.

Carnë Draug <carandraug>
Group Member
Wed 14 Nov 2018 06:58:52 PM UTC, comment #9: 

I checked in a cset on the stable branch here: https://hg.savannah.gnu.org/hgweb/octave/rev/658153e1976d.  I also merge it to the development branch so you can use that code base as well.

Marking as Ready for Test.

Rik <rik5>
Group administrator
Wed 14 Nov 2018 06:44:56 PM UTC, comment #8: 

I don't follow. One always wants to restore the original values, whether a test fails or doesn't. So unwind_protect would be the thing to use, not a try/catch. This is probably just a misunderstanding so I'll wait until you make the change to comment.

Carnë Draug <carandraug>
Group Member
Wed 14 Nov 2018 06:13:19 PM UTC, comment #7: 

The point I wanted to make is that an unwind_protect/unwind_protect_cleanup is not the optimal strategy.  The current set of tests already restores the original value of gnuplot_binary, so there is no need to run cleanup code every time which repeats that.  On the other hand, if the tests fail for any reason then you do want to restore the original values.  This calls out for a try/catch strategy.

Rik <rik5>
Group administrator
Wed 14 Nov 2018 05:22:42 PM UTC, comment #6: 

Rik wrote:

> [...] Thus, the test would pass in the unwind_protect block [...]


It would not. In this specific case, the assertion would fail in the protect block which still throw an error and so the test would fail.

It is true that in this case the gnuplot binary name would continue to point incorrectly to _foobar_ but if the test fails in the future for some other reason it will at least attempt to restore the right name.

Actually, because the assertions in the test unit are done before restoring the gnuplot binary name, if this test ever fails, the original name is never restored.

Carnë Draug <carandraug>
Group Member
Wed 14 Nov 2018 05:03:04 PM UTC, comment #5: 

I'll work on this.  I think it is also related to bug #55013.  Using unwind_protect wouldn't work well here because the cleanup block would attempt to restore the gnuplot binary to the original value, which is "", and that command would fail.  Thus, the test would pass in the unwind_protect block (and no error reporting like 1 test failed in fntests.log), but the value of the binary would be left at _foobar_.

Rik <rik5>
Group administrator
Wed 14 Nov 2018 04:39:47 PM UTC, comment #4: 

Your patch fixes the issue for me.

Still, I think the change on the test should be done in a unwind_protect block. While it wouldn't protect this specific issue, it may protect for future issues.

In addition, the test is ran from the .in.m template in the source tree instead of the actual file that was "built". In this case there is no change to the test code but if there was, then the test ran could be incorrect.

Carnë Draug <carandraug>
Group Member
Wed 14 Nov 2018 04:33:59 PM UTC, comment #3: 

Configured with :


configure --disable-docs --disable-java --without-qt --without-qscintilla --prefix=/home/carandraug/.local/ CXXFLAGS= -I/home/carandraug/.local/include --no-create --no-recursion


Carnë Draug <carandraug>
Group Member
Wed 14 Nov 2018 04:32:30 PM UTC, comment #2: 

Seems to be trouble with an m4 macro that we provide in m4/acinclude.m4.

Try the attached diff.  It seems to do the right thing.

If it works for you I can make it a real changeset on stable.



(file #45424)

Rik <rik5>
Group administrator
Wed 14 Nov 2018 04:21:35 PM UTC, comment #1: 

Something is odd here.  The gnuplot_binary command names the command that is executed when a user tries to switch toolkits using the command


graphics_toolkit gnuplot


As such, it is fine for it to have a value, such as 'gnuplot', even when that executable does not exist on the system.

The gnuplot_binary program is very simple and only allows get/set access to the persistent variable gp_binary.

The reason why the test seems to fail, is because your original value was "", and it is not valid to set an empty string.  As I mentioned at the beginning, it is okay not to have gnuplot on your system, but the string itself should not be empty.

The problem seems to be that gnuplot_binary.m is generated by configure from gnuplot_binary.in.m where there is this line:


  persistent gp_binary = %OCTAVE_CONF_GNUPLOT%;


To make sure I'm on the right track, how did you configure this version of Octave?

Rik <rik5>
Group administrator
Wed 14 Nov 2018 01:15:17 PM UTC, original submission:  

The test for gnuplot_binary changes the internal value of gnuplot_binary to "__foobar__". If the test fails, then the value is never set back to what it should.


octave:1> gnuplot_binary
ans =
octave:2> test gnuplot_binary
[...]
!!!!! test failed
gnuplot_binary: NEW_PROG must be a non-empty string
octave:3> gnuplot_binary
ans = __foobar__


Related to this issue, is the case that an empty string is a valid gnuplot_binary value (I guess when there's no gnuplot binary on the system), but setting it as such will fail. I think the check for an empty string should be removed.

Carnë Draug <carandraug>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45424:  gnuplot.diff added by rik5 (455B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by carandraug (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-11-15 carandraug StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-11-14 rik5 StatusIn Progress Ready For Test
    2018-11-14 rik5 Assigned toNone rik5
    2018-11-14 rik5 Attached File- Added gnuplot.diff, #45424
        StatusNone In Progress
        Releasedev 4.4.1

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code