bugGNU Octave - Bugs: bug #54636, path is not be reinitialized if a...

 
 

bug #54636: path is not be reinitialized if a class directory is modified

Submitter:  Adam Dodd <adamd1008>
Submitted:  Sun 09 Sep 2018 04:30:55 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Adam Dodd Open/Closed:  * Closed
Release:  * 7.0.90 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 28 Jan 2022 08:59:12 AM UTC, comment #9: 

Thank you again for your patch @adamd1008 and sorry that the review process stalled.

I made a few modifications (see comments above the function) and pushed it here: https://hg.savannah.gnu.org/hgweb/octave/rev/7e5e77ef09d7

Kai Torben Ohlhus <siko1056>
Group Member
Wed 26 Jan 2022 02:33:52 AM UTC, comment #8: 

Reviewed patch for Octave 7 (file #52735).

Kai Torben Ohlhus <siko1056>
Group Member
Thu 16 Jul 2020 01:43:13 PM UTC, comment #7: 

Mike, I've noticed recently that this issue is still present in v5.2.0.

Is there something wrong with the patch? Does it require more work? Can I help?

Adam Dodd <adamd1008>
Thu 20 Sep 2018 09:06:00 PM UTC, comment #6: 

Thanks Mike. Updated patch as advised. Apologies for the rusty start!

Re-ran the tests with the same result:


  PASS                            14733
  FAIL                                0
  XFAIL (reported bug)               34
  SKIP (missing feature)            520
  SKIP (run-time condition)          12


Manual tests (as described below) for bug #46282 and bug #54636 also pass.

I was rather phased by the test log not uploading. It's 5.3 MB and there was no error message when submitting it! I've compressed it this time (1.1 MB), hopefully that works.

Let me know if there's anything else.

(file #45068, file #45069)

Adam Dodd <adamd1008>
Thu 20 Sep 2018 06:37:23 PM UTC, comment #5: 

Yes, probably a file size issue. The "upload size limit" mentioned is not reliable.

Thanks for working on improving this. The new patch looks good, better that it doesn't need to add a new method to the class.

The indentation on the nested 'if' statements in 'subdirs_modified' is not quite right. And you might as well combine all of those nested 'if's into one complex condition, since there is no 'else'. That might look something like


            if (fs && fs.is_dir () && (fname[0] == '@' || fname[0] == '+'
                                       || fname == "private")
                && (fs.mtime () + fs.time_resolution () > last_checked)
              {
                ret = true;
                break;
              }


The first summary line of the commit message should mention this and any other bug reports fixed by this change with a "(bug #1, #2, #3)" tag at the end of the line.

Thanks for doing a full test suite run as well as showing the test cases that this bug resolves.

Mike Miller <mtmiller>
Group Member
Thu 20 Sep 2018 06:03:01 PM UTC, comment #4: 

Trying to attach test log again (didn't work; size issue?)

Adam Dodd <adamd1008>
Thu 20 Sep 2018 05:50:29 PM UTC, comment #3: 

Attached new patch:

  • subdirs_modified is now a static function (in namespace octave due to use of sys:: classes)


I ran `make check` for this patch, summary:


  PASS                            14733
  FAIL                                0
  XFAIL (reported bug)               34
  SKIP (missing feature)            520
  SKIP (run-time condition)          12


Full log attached. Also did some quick manual testing as outlined below, all good.

Please let me know if anything else needs changing.

(file #45064)

Adam Dodd <adamd1008>
Mon 17 Sep 2018 09:35:34 PM UTC, comment #2: 

I have not yet tested this patch, but at a glance it looks like it is doing something sensible and the coding style looks mostly correct. I'm not sure that the new function 'subdirs_modified' needs to be a member function of the 'octave::load_path::dir_info' class, I think it could be a bare file scoped function.

Mike Miller <mtmiller>
Group Member
Sun 09 Sep 2018 04:34:14 PM UTC, comment #1: 
Adam Dodd <adamd1008>
Sun 09 Sep 2018 04:30:55 PM UTC, original submission:  

Octave has a bug where removal of a class function script file will cause a "no such file" error to be thrown.

Please note that the attached patch fixes this bug as well as bug #46282. It incorporates a fix to bug #46281 and #53856 committed in changeset 25374:c8f49ee7a687.

To reproduce, I used the current head of the Mercurial repo:


[adam@doddy-pc octave-hg2]$ hg summary
parent: 25869:462066384af3 tip
 fix minimum size of console widget in gui (bug54620)
<snip>


Do the following:


[adam@doddy-pc ~]$ cd `mktemp -d`
[adam@doddy-pc tmp.SxVxOnqAot]$ mkdir @myobj
[adam@doddy-pc tmp.SxVxOnqAot]$ mkdir @myobj
[adam@doddy-pc tmp.SxVxOnqAot]$ cat > @myobj/myobj.m
function c = myobj(arg = "hi")
    s.arg = arg;
    c = class(s, "myobj");
end
[adam@doddy-pc tmp.SxVxOnqAot]$ cat > @myobj/display.m
function display(obj)
    printf("%s = <arg = \"%s\">\n", inputname(1), obj.arg);
end
[adam@doddy-pc tmp.SxVxOnqAot]$ /opt/dbg-octave2/bin/octave-cli
GNU Octave, version 5.0.0
<snip>
octave:1> o = myobj()
o = <arg = "hi">


Now, using another terminal:


[adam@doddy-pc tmp.SxVxOnqAot]$ mv @myobj/display.m{,.bck}


Returning to Octave terminal:


octave:2> o = myobj()
error: no such file, '/tmp/tmp.SxVxOnqAot/@myobj/display.m'


A similar approach can be taken to reproduce bug #46282. Foo/bar example taken from that bug.


octave:1> Foo.bar
Hello from Foo.bar
octave:2> Foo.bar
error: no such file, '/tmp/tmp.SxVxOnqAot/+Foo/bar.m'


The issue is that load_path::dir_info::update checks the top directory's timestamp (in this case /tmp/tmp.SxVxOnqAot), when it's the @myobj or +Foo folder which has its timestamp updated.

My patch fixes these:


octave:1> o = myobj()
o = <arg = "hi">
octave:2> o = myobj()
o =

  <class myobj>

octave:3> o = myobj()
o = <arg = "hi">
octave:4> Foo.bar
Hello from Foo.bar
octave:5> Foo.bar
error: member `bar' in package `Foo' does not exist
octave:5> Foo.bar
Hello from Foo.bar


In each case, I renamed the file for the second call and reinstated it for the third.

Adam Dodd <adamd1008>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #52735:  54636_r4.patch added by siko1056 (3KiB - text/x-patch)
file #45068:  54636_r3.patch added by adamd1008 (3KiB - text/x-patch - r3: patch, test log)
file #45069:  fntests_r3.log.bz2 added by adamd1008 (1MiB - application/x-bzip - r3: patch, test log)
file #45064:  54636_r2.patch added by adamd1008 (3KiB - text/x-patch - New patch, test log)
file #44960:  mypatch.patch added by adamd1008 (4KiB - text/x-patch - Patch file)

 

Carbon-Copy List
  • -email is unavailable- added by siko1056 (Knows it all ;-))
  • -email is unavailable- added by siko1056 (Updated the item)
  • -email is unavailable- added by adamd1008 (Submitted the item)
  • -email is unavailable- added by adamd1008
  •  

    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 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-01-28 siko1056 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2022-01-26 siko1056 Dependencies- bugs #46282 is dependent
    2022-01-26 siko1056 Release4.4.1 7.0.90
    2022-01-26 siko1056 StatusPatch Submitted Patch Reviewed
        Carbon-Copy- Added jwe
    2022-01-26 siko1056 Attached File- Added 54636_r4.patch, #52735
    2018-09-20 adamd1008 Attached File- Added 54636_r3.patch, #45068
        Attached File- Added fntests_r3.log.bz2, #45069
    2018-09-20 adamd1008 Attached File- Added 54636_r2.patch, #45064
    2018-09-19 mtmiller Carbon-CopyRemoved 80942 -
    2018-09-10 mtmiller Carbon-CopyRemoved 80942 -
    2018-09-10 mtmiller Item GroupOther Unexpected Error or Warning
        StatusNone Patch Submitted
    2018-09-09 adamd1008 Attached File- Added mypatch.patch, #44960
        Carbon-Copy- Added tpapastylianou

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code