bugGNU Octave - Bugs: bug #62780, oruntests looks in file without...

 
 

bug #62780: oruntests looks in file without respecting .oct-config

Submitter:  Colin Macdonald <cbm>
Submitted:  Mon 18 Jul 2022 05:34:42 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  9.1.0 Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 26 Jan 2023 08:17:20 AM UTC, comment #14: 

No complaints in more than two months.
Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Sat 19 Nov 2022 01:51:09 PM UTC, comment #13: 

I pushed a change here that changes to the directory containing the tests and adds a note about this change to the NEWS file:
https://hg.savannah.gnu.org/hgweb/octave/rev/601b08ce0c00

Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Fri 21 Oct 2022 06:40:44 AM UTC, comment #12: 

Setting status to confirmed because we still might want that `oruntests` should `cd` to the directory with the tests. That would add the file encoding for that directory to the look-up table in the load path.

Markus Mützel <mmuetzel>
Group administrator
Mon 25 Jul 2022 07:01:10 AM UTC, comment #11: 


> While I personally don't see how this particular behaviour is desirable,


The implementation of `dir_encoding` should be as fast as possible. So, instead of reading files from the disc (which is slow in general), it uses a look-up table that it generated before (on updates of the load path which is slow anyway).

Markus Mützel <mmuetzel>
Group administrator
Sun 24 Jul 2022 10:52:53 PM UTC, comment #10: 

Ok I see.  While I personally don't see how this particular behaviour is desirable, I can confirm that this patch works, and it is well-tested by the included BISTs.

Colin Macdonald <cbm>
Sun 24 Jul 2022 12:15:51 PM UTC, comment #9: 

The behavior that you show in comment #6 and comment #7 is expected if that directory was never in Octave's load path and no encoding has manually been specified with `dir_encoding`. See also the tests here:
https://hg.savannah.gnu.org/hgweb/octave/file/7d4cf04665e6/test/file-encoding/file-encoding.tst

If the documentation of `dir_encoding` is unclear in that respect, updates to it are welcome. It currently contains the following sentence:

> The directory encoding is automatically read from the file .oct-config when a new path is added to the load path (for example with addpath).


Negate that sentence, and it says: "The encoding is not read from those files unless their path is added to the load path." (The current directory is always in the load path.)

IIUC, `make check` or `__run_test_suite__` change to each directory for which they are running tests. The possible change that you describe in comment #8 would bring `oruntests` more in line with those commands. However, it might also break some use cases that rely on the current directory not changing during the tests...
If we decide to make that change, it would definitely be worth to at least add a comment about it in the NEWS file (and maybe also clarify that in the [currently very sparse] documentation of `oruntests`).

Markus Mützel <mmuetzel>
Group administrator
Sat 23 Jul 2022 09:09:17 PM UTC, comment #8: 

I guess the quick fix here is to patch oruntests to visit directories it is exploring: that might be a good idea anyway.  Esp. if the current behaviour of `dir_encoding` is correct/desirable.

Colin Macdonald <cbm>
Sat 23 Jul 2022 09:04:39 PM UTC, comment #7: 

Here's MWE:

>> d = "/home/cbm/src/octave-doctest.git/test_extra/test_encoding"
>> s = dir_encoding (d)
s = utf-8
>> cd(d)
>> cd()
>> s = dir_encoding (d)
s = windows-1252


Is this how `dir_encoding` is supposed to work?  The docs say "Query ... the ENCODING that is used for reading m-files in DIR."

I'm just learning all this but, it seems to me either:

1.  `dir_encoding` should "visit" the directory

or

2.  `help dir_encoding` should explain that it is something theoretical like "what encoding we could try unless there is a .oct-config" and only becomes concrete if you've visited (via cd or addpath or whatever).

Personally, I think 1 sounds like principle of least surprise, but maybe I'm not seeing the whole picture here?

Colin Macdonald <cbm>
Sat 23 Jul 2022 08:14:39 PM UTC, comment #6: 

I'm trying this but it does not seem to be helping...  I'm testing by doing `oruntests("test_encoding")` from inside the doctest "test_extra" directory.

It fails like:

error: regexp: the input string is invalid UTF-8
error: called from
    oruntests>has_tests at line 155 column 10
    oruntests>run_all_tests at line 82 column 11
    oruntests at line 62 column 5

(line numbers are a little off b/c I'm debugging)

I've echoed some debugging stuff here:

  str = fread (fid, "*char").';
  fclose (fid);
  disp(fileparts(f))
  enc = dir_encoding (fileparts (f))
  if (! strcmp (enc, "utf-8"))
    disp("convertx from input encoding to UTF-8")
    str = native2unicode (uint8 (str), enc);
  endif
  disp(str)
  disp("about to regexp")
  retval = ! isempty (regexp (str,
                              '^%!(assert|error|fail|test|xtest|warning)',
                              'lineanchors', 'once'));


From here, I learn that `enc` is set to "utf-8" despite the .oct-config file containing `encoding=windows-1252`.

AFAICT, the issue is that `dir_encoding(foo)` only tells you how Octave will consider the encoding...  But if the path hasn't been yet loaded (e.g., with addpath) then it just (incorrently) says "utf-8".


- - - - - - - - -

Here's a fresh session:


>> s = dir_encoding("/home/cbm/src/octave-doctest.git/test_extra/test_encoding")
s = utf-8
>> addpath("/home/cbm/src/octave-doctest.git/test_extra/test_encoding")
>> s = dir_encoding("/home/cbm/src/octave-doctest.git/test_extra/test_encoding")
s = windows-1252


So here oruntest has not yet accessed the path when we query the `dir_encoding`...?

Colin Macdonald <cbm>
Wed 20 Jul 2022 02:55:37 PM UTC, comment #5: 

Specifying the encoding with `fopen` doesn't make a difference because `fread` reads binary data from the file (i.e., it doesn't convert anything).
Instead, I pushed a change that converts the read byte stream from the input encoding to UTF-8 here:
https://hg.savannah.gnu.org/hgweb/octave/rev/7d4cf04665e6

This change also adds a few tests for different scenarios.

Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Tue 19 Jul 2022 07:26:55 AM UTC, comment #4: 

There are a couple of places where `fopen` is called with a bare file argument in `oruntests.m`.
These bare calls can probably be modified to look somewhat like this:

  fid = fopen (f, "rt", "n", dir_encoding (filedir));


And some logic to gather the `filedir` for the file `f`. Maybe, that is just the following if it is clear that `f` will always be the full path to the file:

  fid = fopen (f, "rt", "n", dir_encoding (fileparts (f)));


Markus Mützel <mmuetzel>
Group administrator
Mon 18 Jul 2022 08:06:27 PM UTC, comment #3: 

I looked at the source:


function retval = has_tests (f)

  fid = fopen (f);
  if (fid < 0)
    error ("oruntests: fopen failed: %s", f);
  endif

  str = fread (fid, "*char").';
  fclose (fid);
  retval = ! isempty (regexp (str,
                              '^%!(assert|error|fail|test|xtest|warning)',
                              'lineanchors', 'once'));


I haven't done this yet, but I guess next step is to compare that to some code that does respect .oct-config.

Colin Macdonald <cbm>
Mon 18 Jul 2022 08:02:05 PM UTC, comment #2: 

It seems it does not help to add the directory to the load path:

>> addpath(canonicalize_file_name ("test_extra/test_encoding/"))
>> oruntests test_extra/test_encoding/
Processing files in /home/cbm/src/octave-doctest.git/test_extra/test_encoding:

error: regexp: the input string is invalid UTF-8
error: called from
    oruntests>has_tests at line 141 column 10
    oruntests>run_all_tests at line 82 column 11
    oruntests at line 62 column 5



`dir_encoding` also did not seem to help:

>> p = canonicalize_file_name ("test_extra/test_encoding/")
p = /home/cbm/src/octave-doctest.git/test_extra/test_encoding
>> dir_encoding (p, "CP1252")
>> tmp = dir_encoding (p)
tmp = cp1252
>> oruntests (p)
Processing files in /home/cbm/src/octave-doctest.git/test_extra/test_encoding:

error: regexp: the input string is invalid UTF-8
error: called from
    oruntests>has_tests at line 141 column 10
    oruntests>run_all_tests at line 82 column 11
    oruntests at line 62 column 5


Colin Macdonald <cbm>
Mon 18 Jul 2022 05:14:02 PM UTC, comment #1: 

The `.oct-config` logic is linked to the load path for caching purposes.
Is the directory with the test in Octave's load path when you run the test? Does it work if it is?

If you don't want to add that path to Octave's load path, maybe set the encoding manually with `dir_encoding`.

Markus Mützel <mmuetzel>
Group administrator
Mon 18 Jul 2022 05:34:42 AM UTC, original submission:  

@mmuetzel, I found another place where .oct-config is not respected:


>> oruntests test_extra/test_encoding/
Processing files in /home/cbm/src/octave-doctest.git/test_extra/test_encoding:

error: regexp: the input string is invalid UTF-8
error: called from
    oruntests>has_tests at line 141 column 10
    oruntests>run_all_tests at line 82 column 11
    oruntests at line 62 column 5


This is using the `test_CP1252.m` file you added in https://github.com/catch22/octave-doctest/pull/256

I tried that on Octave HG tip, including your patch:

changeset:   31144:f125ddb46055
user:        Markus Mützel <markus.muetzel@gmx.de>
date:        Wed Jul 13 13:20:48 2022 +0200
summary:     dir_encoding: Use encoding from .oct-config file in current directory (bug #6276>


- - - -

I guess oruntests does some raw file reading stuff that bypasses the oct-config stuff.

Colin Macdonald <cbm>

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (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 group members can vote.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-01-26 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 9.1.0
    2022-11-19 mmuetzel StatusConfirmed Ready For Test
    2022-10-21 mmuetzel StatusReady For Test Confirmed
    2022-07-20 mmuetzel StatusNeed Info Ready For Test
    2022-07-20 mmuetzel CategoryTest Suite Octave Function
    2022-07-18 mmuetzel StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code