bugGNU Octave - Bugs: bug #64401, load-path promise 'removing from...

 
 

bug #64401: load-path promise 'removing from path' not kept

Submitter:  Muhali <muhali>
Submitted:  Mon 10 Jul 2023 11:40:00 AM UTC
   
 
Category:  Interpreter Severity:  2 - Minor
Priority:  3 - Low Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 8.1.0 Operating System:  * Any
Fixed Release:  9.1.0 Planned Release:  9.1.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 13 Oct 2023 07:15:45 PM UTC, comment #9: 

No problems reported in 3 months. Closing as fixed.

Arun Giridhar <arungiridhar>
Group Member
Wed 12 Jul 2023 06:24:34 PM UTC, comment #8: 

I checked in Markus's original patch under his name here: http://hg.savannah.gnu.org/hgweb/octave/rev/65781d62fee7.  I added in the additional changes to call any removal hook function and cleanly remove a directory in a second changeset http://hg.savannah.gnu.org/hgweb/octave/rev/e82a6eec8880.

I think this is now correctly taken care of.  Marking as Ready for Test.

Rik <rik5>
Group administrator
Wed 12 Jul 2023 05:50:49 PM UTC, comment #7: 

Thanks for looking into the patch and the additional clarifications.

I think I got your point. Please, see the updated patch that should be removing the folder also from any +folder "packages" if that should be necessary.

(Just a tangent: Are those "+folder" things called packages or namespaces? I was calling them namespaces most of the time because they behave kind of like namespaces in other languages. It's a bit confusing that we also have Octave packages using the `pkg` function which are something completely different.)


(file #54922)

Markus Mützel <mmuetzel>
Group administrator
Wed 12 Jul 2023 05:23:44 PM UTC, comment #6: 

The m_dir_list variable is a member of the nested package_info class.  It is used to track the directories that contain a +package directory.  Multiple directories can contain +package directories with the same name.  They are all considered to be parts of the same package instead of having one shadow the rest.

The m_dir_info_list variable is the list of the dir_info objects known to the load path.

John W. Eaton <jwe>
Group administrator
Wed 12 Jul 2023 04:58:14 PM UTC, comment #5: 

I verified that Markus's patch works for the issue at hand.

Unfortunately, the code is not clear to me.  The existing function is


void
load_path::update ()
{
  // I don't see a better way to do this because we need to
  // preserve the correct directory ordering for new files that
  // have appeared.

  signal_clear_fcn_cache ();

  m_top_level_package.clear ();

  m_package_map.clear ();

  for (auto& di : m_dir_info_list)
    {
      bool ok = di.update ();

      if (! ok)
        warning_with_id
        ("Octave:load-path:update-failed",
         "load-path: update failed for '%s', removing from path",
         di.dir_name.c_str ());
      else
        add (di, true, "", true);
    }
}


This appears as if it would automatically exclude a path that doesn't exist from future consideration.  But, the load-path class contains two member variables "m_dir_info_list" and "m_dir_list".  I can see that they are of different types "std::list<dir_info>" and "std::list<std::string>", but otherwise I'm not sure how they are used.  I've added jwe to the CC list for a comment.

Otherwise, I think the patch can be committed.

Rik <rik5>
Group administrator
Tue 11 Jul 2023 04:32:37 PM UTC, comment #4: 

I just checked in Matlab and the 'rmdir' command issues a warning and automatically removes the no longer present directory from the path.  That is probably the nicest thing to do.

We could remove the directory from the path whenever it is unavailable.  The downside for that might be a network-connected drive which has intermittent connectivity.  In that case, just because the path was not available when Octave returned to the command prompt one time doesn't mean it might not be available again at the next command prompt.

I'm not greatly concerned about which strategy is used to solve this.

Rik <rik5>
Group administrator
Mon 10 Jul 2023 06:28:29 PM UTC, comment #3: 

I seem to remember that that worked at some time in the past. But I didn't actually check.

See the attached patch for a possible solution.
It removes the path for which the update of the load path failed from the list of folders in the load path. So, the warning from comment #0 will only be emitted once (for each non-existing path).


(file #54920)

Markus Mützel <mmuetzel>
Group administrator
Mon 10 Jul 2023 05:04:51 PM UTC, comment #2: 

I know, but this is about the 'removing from path'-clause.

Muhali <muhali>
Mon 10 Jul 2023 05:00:04 PM UTC, comment #1: 

rmpath works

Anonymous
Mon 10 Jul 2023 11:40:00 AM UTC, original submission:  

If I do

mkdir foo
addpath foo
rmdir foo
pi


the (annoying) warnings

warning: load_path: foo: No such file or directory
warning: load-path: update failed for 'foo', removing from path

are never quieted, as 'foo' remains in the path despite the promise 'removing from path'.

Muhali <muhali>

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by muhali (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 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-10-13 arungiridhar StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2023-07-12 rik5 StatusPatch Reviewed Ready For Test
        Fixed ReleaseNone 9.1.0
    2023-07-12 mmuetzel Attached File- Added bug64401-load-path-remove-non-existing-v2.patch, #54922
    2023-07-12 rik5 StatusPatch Submitted Patch Reviewed
        Carbon-Copy- Added jwe
    2023-07-11 rik5 Severity3 - Normal 2 - Minor
        Priority5 - Normal 3 - Low
    2023-07-10 mmuetzel Attached File- Added bug64401-load-path-remove-non-existing.patch, #54920
        CategoryNone Interpreter
        Item GroupNone Unexpected Error or Warning
        StatusNone Patch Submitted
        Operating SystemGNU/Linux Any
        Planned ReleaseNone 9.1.0

    Back to the top

    Powered by Savane 3.13-dea8.
    Corresponding source code