patchGNU Octave - Patches: patch #9464, texinfo errors/warnings will set...

 
 

patch #9464: texinfo errors/warnings will set status flag

Submitter:  Colin Macdonald <cbm>
Submitted:  Sat 14 Oct 2017 04:58:33 PM UTC
   
 
Category:  Core : other Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  mtmiller Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 02 Jan 2020 10:42:15 PM UTC, comment #12: 

Of course the patch had fallen behind the code base since March so I had to update it to apply cleanly.  I checked it in here https://hg.savannah.gnu.org/hgweb/octave/rev/fde18e0a2984.

This will be a part of the 6.1 release of Octave.  Marking as "Done" and closing report.

Rik <rik5>
Group administrator
Tue 26 Mar 2019 05:16:45 AM UTC, comment #11: 

@apjanke, that isn't very meaningful.

After this patch, if you have a .m file with malformed texinfo in it, then doctest should register a failure.  Try "doctest pkj" on your project, before the @ -> @@ bug I showed you earlier...

Or misspell @defunxxXXXxzz, etc

Colin Macdonald <cbm>
Tue 26 Mar 2019 01:53:55 AM UTC, comment #10: 

That patch applies cleanly to head of default for me. This is what I get:


octave:8> version
ans = 6.0.0
octave:9> system('sw_vers');
ProductName:        Mac OS X
ProductVersion:        10.14.3
BuildVersion:        18D109
octave:10> pkg list
Package Name  | Version | Installation directory
--------------+---------+-----------------------
     doctest *|   0.6.1 | /Users/janke/octave/doctest-0.6.1
octave:11> doctest vpa
Doctest v0.6.1: this is Free Software without warranty, see source.

vpa .................................................... EXTRACTION ERROR

    Function or class not found.


Summary:

   PASS    0/0

0/1 targets passed, 0 without tests, 1 with extraction errors.

octave:12>


I don't know if that counts as success or failure.

Andrew Janke <apjanke>
Tue 26 Mar 2019 01:44:18 AM UTC, comment #9: 


> See bug #44712. I would recommend we fix that so that leaking stderr is not an issue here.


Cool.

If you do fix it, it might be nice to keep around a variant of `system()` which keeps stdout and stderr separate. Sometimes it's nice to get just a list of error output in the midst of a big operation, or it can be difficult to deal with output and error diagnostics that are interleaved in one stream. And because system() is a low-level call, it would be hard for users to implement that on their own.

Andrew Janke <apjanke>
Tue 26 Mar 2019 01:24:28 AM UTC, comment #8: 


> Is it portable to Windows?


It seems to be, in the basic testing I've done.


>> ver
----------------------------------------------------------------------
GNU Octave Version: 5.1.0 (hg id: d05d6eebde10)
GNU Octave License: GNU General Public License
Operating System: MINGW32_NT-6.2 Windows 6.2  x86_64
----------------------------------------------------------------------
[...]
܀>> [std_out, std_err, status] = packajoozle.internal.Util.system('ls no such file')
std_out =
std_err = ls: cannot access 'no': No such file or directory
ls: cannot access 'such': No such file or directory
ls: cannot access 'file': No such file or directory

status =  2
>>


Andrew Janke <apjanke>
Tue 26 Mar 2019 12:37:18 AM UTC, comment #7: 

Thanks for the workaround link.  Is it portable to Windows?

Colin Macdonald <cbm>
Tue 26 Mar 2019 12:33:03 AM UTC, comment #6: 

Oops, you're right I didn't seem to attach it, trying again...

(file #46638)

Colin Macdonald <cbm>
Tue 26 Mar 2019 12:27:09 AM UTC, comment #5: 

There is a long open bug about being compatible with Matlab's 'system' function, which does always redirect stderr. See bug #44712. I would recommend we fix that so that leaking stderr is not an issue here.

Mike Miller <mtmiller>
Group Member
Mon 25 Mar 2019 11:25:00 PM UTC, comment #4: 

Patch still doesn't apply to the head of `default`. Did you post a new patch file?


$ hg import ~/Downloads/makeinfo_set_status_on_error.patch
applying /Users/janke/Downloads/makeinfo_set_status_on_error.patch
patching file scripts/help/__makeinfo__.m
Hunk #1 FAILED at 126
1 out of 2 hunks FAILED -- saving rejects to file scripts/help/__makeinfo__.m.rej
abort: patch failed to apply



For item 4, you could borrow my hack: https://github.com/apjanke/octave-packajoozle/blob/c0b5c6712fd4042162804dd8cbb13404868156b3/inst/%2Bpackajoozle/%2Binternal/Util.m#L243-L266

Andrew Janke <apjanke>
Thu 07 Mar 2019 06:33:57 PM UTC, comment #3: 

Thanks, I've revised.

1. I passed 'once' to regexprep in the unlikely event the temp file name contains "--output=".

2. I revised the message a bit to clarify this is only about errors.  Without '--no-warn' we get a lot of warnings like "/tmp/octave-help-2ri7iS:117: warning: entry for index `fn' outside of any node".  Outside the scope of this fix...  Anyway, AFAICT warnings don't set non-zero return value.

3. What should we do about BIST?  If we put a deliberately broken texinfo string in, the test suite is going to output messages like "/tmp/octave-help-JpLtLe:118: unknown command `defunxx'".

4. If we did want to silence that output, IIRC evalc won't capture stderr output of system.  Not for the first time, I'm reminded it would be useful to have something like "system" that returns the stderr and stdout as separate strings...

Colin Macdonald <cbm>
Thu 07 Mar 2019 01:23:55 AM UTC, comment #2: 

Or


cmd = regexprep (cmd, "(--output=)", "--force $1");


Mike Miller <mtmiller>
Group Member
Thu 07 Mar 2019 01:19:49 AM UTC, comment #1: 

This doesn't apply due to some minor quoting differences now, but I think we can do this for Octave 6. @cbm Do you want to refresh the patch? If not I can easily enough.

Aesthetically it would be nice if the "--force" option were added along with the other options, even though yeah, yeah, GNU allows options after arguments...

What about something like


cmd = regexprep (cmd, "--output=", "--force --output=");


?

Other than that, I just need to test it against the current default branch, make sure help, docs, tests all work as before, test a few packages, etc. Should be safe, and shouldn't make things any slower as long as texinfo is written properly. Just cautious because spawning makeinfo for each doc string is a very slow part of Octave these days.

Mike Miller <mtmiller>
Group Member
Sat 14 Oct 2017 04:58:33 PM UTC, original submission:  

Previously, we called "makeinfo --force" so we returned at least partial
output even from misformed texinfo.  However, it wasn't possible to
determine that an error had occured because "status" was not set.
Instead we call makeinfo normally, and if there was an error, we process
a second time with --force.

  • _makeinfo_.m: set status non-zero if makeinfo gave errors/warnings


- - - - - - -

This came out of a discussion at:

https://github.com/catch22/octave-doctest/issues/166

Colin Macdonald <cbm>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #46638:  texinfo_errors_will_set_status_flag.patch added by cbm (2KiB - text/x-patch - updated 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 apjanke (Posted a comment)
  • -email is unavailable- added by cbm (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 logged-in users can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-01-02 rik5 StatusNone Done
        Open/ClosedOpen Closed
    2019-06-09 mtmiller Assigned toNone mtmiller
    2019-04-07 mtmiller Carbon-CopyRemoved mtmiller -
    2019-03-26 cbm Attached File- Added texinfo_errors_will_set_status_flag.patch, #46638
    2017-10-14 cbm Attached File- Added makeinfo_set_status_on_error.patch, #42146
        Carbon-Copy- Added mtmiller

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code