bugGNU Octave - Bugs: bug #62865, 'path' should return drive letter,...

 
 

bug #62865: 'path' should return drive letter, not UNC path when 'addpath' was called with drive letter

Submitter:  Philip Nienhuis <philipnienhuis>
Submitted:  Fri 05 Aug 2022 06:50:39 PM UTC
   
 
Category:  Octave Function Severity:  2 - Minor
Priority:  3 - Low Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Philip Nienhuis Open/Closed:  * Closed
Release:  * dev Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 18 Oct 2022 08:36:35 PM UTC, comment #23: 

Just checked with 8.0.0 (dev) from Oct. 7th where the fix is included.
No more error messages in the scenario I described in comment #6.

So, closing this report as fixed :-)

Philip Nienhuis <philipnienhuis>
Group Member
Mon 17 Oct 2022 07:10:08 PM UTC, comment #22: 

I forgot about it too. Let me check tomorrow at work where this issue popped up.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 17 Oct 2022 06:40:11 PM UTC, comment #21: 

forgot about this one when bug #62847 was closed. 

Using a recent stable build (7.2.1 (hg id: e649aa69d5bc)) which includes the bug #62847 fix to canonicalize_file_name for network paths on windows, i can no longer recreate the comment #6 issue. loading the file as described, attempting to set a breakpoint to trigger the "Change directory" / "Add directory to load path" / "Cancel" dialog, and selecting Add directory to load path no longer produces an error message. The breakpoint is created and execution can proceed as expected.  Checking the Path afterward shows the network file correctly added to the top of the path list.

Can others verify? I think we may be able to close this as fixed.



Nicholas Jankowski <nrjank>
Group Member
Tue 23 Aug 2022 02:48:23 PM UTC, comment #20: 

just bumping this since last comment was during a savannah no-email period.

Nicholas Jankowski <nrjank>
Group Member
Mon 15 Aug 2022 07:20:29 PM UTC, comment #19: 

updating status with the patch from friday mentioned in bug #62847.  as mentioned over there, the patch on the latest stable build (hg id: 820d2c802247) still does not solve the path issue for my system. Regarding setting the breakpoint, I still get the 'unable to find function' error, and path shows nothing was added. if the file is loaded with the UNC path the breakpoint sets and the UNC path appears in path without issue.

Nicholas Jankowski <nrjank>
Group Member
Fri 12 Aug 2022 08:35:01 PM UTC, comment #18: 

In reply to comment #6: I think, the function that is analyzing the file path an extracting the function when adding a breakpoint in the GUI editor should be the bp_file_info constructor in bp-table.cc:


    bp_file_info (tree_evaluator& tw, const std::string& file)
      : m_ok (false), m_file (file), m_dir (), m_fcn (), m_class_name ()
    {
      std::string abs_file = sys::env::make_absolute (file);

      std::string dir = sys::file_ops::dirname (abs_file);
      std::string fcn = sys::file_ops::tail (abs_file);
      std::size_t len = fcn.length ();
      if (len >= 2 && fcn[len-2] == '.' && fcn[len-1] == 'm')
        fcn = fcn.substr (0, len-2);

      std::size_t pos = dir.rfind (sys::file_ops::dir_sep_chars ());

      if (pos != std::string::npos && pos < dir.length () - 1)
        {
          if (dir[pos+1] == '@')
            {
              m_class_name = dir.substr (pos+1);

              fcn = sys::file_ops::concat (m_class_name, fcn);

              dir = dir.substr (0, pos);
            }
        }

      m_dir = dir;
      m_fcn = fcn;

      interpreter& interp = tw.get_interpreter ();

      load_path& lp = interp.get_load_path ();

      if (lp.contains_file_in_dir (m_file, m_dir))
        m_ok = true;
    }


Torsten Lilge <ttl>
Group Member
Fri 12 Aug 2022 03:04:14 PM UTC, comment #17: 

I don't know if this is the only use case. But one that comes to mind is the following:
The load path is where the information is stored which character encoding is used to parse a file. If a user executes a script or function using a path that differs from the one originally fed to the load path (e.g., in character case or different symlink path), they probably still expect that the file is executed taking that information into account.
That is what I meant by the second requirement. What you are describing would be the covered by the first requirement.

Markus Mützel <mmuetzel>
Group administrator
Fri 12 Aug 2022 02:48:29 PM UTC, comment #16: 

is 2 really that much harder?  it has always been possible to have a location in a system PATH twice (at least, most systems I've looked at make no attempt to trim any redundancies). an 'is_in_path' check just navigates through them in order, and stops when it finds one. if something exists in multiple locations, typically, unless you've asked it to find all occurrences, the first one wins for execution, and that successful path entry is returned as the location, and it looks no farther.  Now in matlab, which can have a -ALL option which will return every instance of a function it sees in the path. but even still, if something exists twice, (or is pointed to twice) it would just get listed twice.

is there a use case where you would need to know something's location(s) and need to make use of a non-first one?

Nicholas Jankowski <nrjank>
Group Member
Fri 12 Aug 2022 07:21:57 AM UTC, comment #15: 


> Does your CMD.exe support UNC paths?


It doesn't. (But I'm not sure why `cmd.exe` should be involved when adding a directory to the load path...)

> the part about just one unique name for a path entry. But he didn't motivate why that should be the actual name of the directory rather than symlinks.


IIUC, the load path needs to fulfill two conditions (currently):
1. Check if a certain file is in some folder in the load path.
2. Determine in which directory in the load path a given file resides.

The first condition is pretty easy to realize. (Famous last words. 😉)

The second one is much harder:
It is rather simple to get the "actual" path (One True Directory, i.e., "canonicalized path") for a path of a pretty much arbitrary form. But it is virtually impossible to calculate the whole set of possible paths that could point to a given file. (Those could be infinitely many.) And then compare all of those to the path stored in the load path to check where it matches.
This is further complicated by the fact that file paths are case-insensitive most of the time on Windows (but sometimes they aren't).
The only reasonable way to know if two paths are actually the same (without accessing files repeatedly, and that should be working independent of the used file system) is to canonicalize them and compare those canonicalized paths.
That is what is currently done in the load path afaict.

Markus Mützel <mmuetzel>
Group administrator
Thu 11 Aug 2022 07:38:16 PM UTC, comment #14: 

you mean, from command line?

Philip Nienhuis <philipnienhuis>
Group Member
Thu 11 Aug 2022 06:48:54 PM UTC, comment #13: 

not sure if it's significant, but the "change directory" option works without issue.

Nicholas Jankowski <nrjank>
Group Member
Thu 11 Aug 2022 06:25:00 PM UTC, comment #12: 

adding - as noted on the other bug report, the patch did not fix my issue of incorrect canonicalized paths over there, so it could still be related.

Nicholas Jankowski <nrjank>
Group Member
Thu 11 Aug 2022 06:13:37 PM UTC, comment #11: 

i get the same "error: add_breakpoints_in_function:..." error using the latest stable build ( 7.2.1 (hg id: 51311efef5dd))


Nicholas Jankowski <nrjank>
Group Member
Thu 11 Aug 2022 06:12:56 PM UTC, comment #10: 

I can very reliably reproduce the issue in comment #6.

Does your CMD.exe support UNC paths? seems to be some (registry?) setting somewhere.
At my work that setting is completely shielded from us. AFAIK we have quite a bit of legacy SW like SCADA systems and PLC stuff, maybe that is a reason.

I have a Windows build from Aug. 7,
7f4ad92265d2
maint: Merge stable to default.
author Markus Mützel <markus.muetzel@gmx.de>


As to JWE's remark in comment #:
I can follow his (and apparently your) reasoning, esp. the part about just one unique name for a path entry. But he didn't motivate why that should be the actual name of the directory rather than symlinks. Yet instinctively I even agree on that.
However, the thing is that on Windows, drive letter paths seem to have some extra associated "magic" compared to just symlinks. That "magic" might be little more than translating UNC paths into drive letter paths for applications not being able to use UNC, possibly through legacy file system calls. Yet the consequences pop up all the time, usually harmless, but sometimes adversely.

My point still stands that if users enter & use drive letter paths, they'll expect, and IMO are entitled to expect, to see drive letter paths returned to them.
If Octave morhps them into UNC paths behind the scenes and only uses those, well that's fine and I would fully support that.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 11 Aug 2022 06:03:43 PM UTC, comment #9: 

Octave 7.2.0 was released before the fix for bug #62847. Can you still reproduce with a recent nightly?
https://buildbot.octave.space/#/download

Markus Mützel <mmuetzel>
Group administrator
Thu 11 Aug 2022 05:56:52 PM UTC, comment #8: 

I can reproduce that comment #6 behavior with Octave 7.2.0, Windows 10.

Markus, I believe you said you're running Win 11? It's possible that the UNC path limitation is no longer a concern?

it's possible this is related to the windows UNC in CMD limits, but I think more likely that it's running into the other canonicalizing  issue from bug #62847 that creates a completely erroneous path.

i didn't realize that fix was pushed to stable.  I'll grab a recent build and see if the issue is repeatable there.

(also added ttl who may be able to comment on what functions are called by the gui in that case)

Nicholas Jankowski <nrjank>
Group Member
Thu 11 Aug 2022 05:07:17 PM UTC, comment #7: 

Not sure if this related to the original issue here (i.e., is it about how paths are shown in the output of the `path` function?).

jwe's comment (copied to comment #3 here) basically covers what would probably need to be changed for this report.

Anyway, I'm not able to reproduce here. Neither with an .m file in a "pure" mapped network drive, nor with an .m file that is on a mapped network drive but "behind" a symbolic link to a different UNC network share (like the issue in bug #62847).

Which version of Octave did you use? I was testing with a build from the default branch (hg id 7f4ad92265d2).

Markus Mützel <mmuetzel>
Group administrator
Thu 11 Aug 2022 04:50:40 PM UTC, comment #6: 

Next issue:

At work, the GUI editor cannot add a network path to the load path through the debug-popup.
Not sure if this is a similar but separate bug from bug #82847, or something else.
While investigating I found a few other UNC path issues with a.o., the file browser pane, I'll enter a separate report for that.

Let's see:

When I want to debug <some_function.m> that:

  • is not in the path
  • that was loaded in the editor through a drive letter path in the "Recent Editor Files" drop-down list
  • by clicking in the editor's left margin,

the editor yields a popup with the
"Change directory" / "Add directory to load path" / "Cancel" buttons.

Clicking the "Add directory to load path" button gets me an error message in the command pane:

error: add_breakpoints_in_function: unable to find function <some_function>


Interestingly, the path shown in the popup is a "drive letter path", not a UNC path.

This does work (adding the subdir to the path through the popup) if I make use of UNC paths (i.e., load the function through a UNC path entry in the Editor's "Recent Editor Files" list.  In the popup the UNC path is shown in that case.

Note: CMD.EXE doesn't support UNC paths.


Philip Nienhuis <philipnienhuis>
Group Member
Tue 09 Aug 2022 07:18:57 PM UTC, comment #5: 

not sure when that would happen.  on my server situation, if I move an m file to the root of a network drive, open octave cli, cd there, run the following I get:


>> which logistic_regression
'logistic_regression' is a function from the file \\full.server.name\path\to\mapped\serer\logistic_regression.m

>> help logistic_regression
'logistic_regression' is a function from the file \\alcnxfs.nae.ds.army.mil\ALC-HomeShare\nicholas.jankowski\logistic_regression.m

 -- [THETA, BETA, DEV, DL, D2L, P] = logistic_regression (Y, X, PRINT,
          THETA, BETA)
     Perform ordinal logistic regression.
...


I think whatever it is has to call system() for it to hit the CMD issue with UNC paths. First time i ran into the UNC issue I was using an old script calling an external tool with system.

does pkg do system calls? if so that might be the most common opportunity for this issue. maybe instrument control? but i don't think that really runs fully on windows (haven't checked in a while).

Nicholas Jankowski <nrjank>
Group Member
Tue 09 Aug 2022 06:37:16 PM UTC, comment #4: 

What about this?


debug> help ismember

'\\<server>.cmpany.local\FULL$\path\to\some\subdir'

CMD.EXE was started with the above path as the current directory.

UNC paths are not supported.  Defaulting to Windows directory.

'ismember' is a function from the file C:\Programs\Octave\Octave-8.0.0_20220807H\mingw64\share\octave\8.0.0\m\set\ismember.m


No idea if this can be harmful (not in this example, I got the help text). But it makes me wonder what happens if one wants to invoke some function living in a network drive in the path where the CMD processor is invoked behind the scenes - unsure is that is a real-life viable scenario.

Whether the UNC path is the Real Thing or not, on Windows equivalent drive letter paths seem to be more reliable in some cases.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 07 Aug 2022 08:39:32 AM UTC, comment #3: 

See also jwe's comment in bug #62847:
https://savannah.gnu.org/bugs/index.php?62847#comment43

> My point wasn't specifically about Unix-style symbolic links, but that if there can be more than one name for a directory (symbolic link, second mount point, whatever) it made sense to me to only store the One True Directory Name as an index for load path searching.  Maybe we should also store the name initially used (the one passed to addpath or other functions) and use that as the one to display to the user?

Markus Mützel <mmuetzel>
Group administrator
Sat 06 Aug 2022 01:47:59 PM UTC, comment #2: 

possibly unrelated - thinking about reasoning for using canonicalize in addpath - does the canonicalize behavior turn windows/dos 8.3 filenames/paths into full-name paths? I recall there being a push a few versions ago to get away from parts of the code that resulted in 8.3 naming conventions, and wasn't sure if things like using canonicalize in addpath was part of fixing that

Nicholas Jankowski <nrjank>
Group Member
Sat 06 Aug 2022 07:22:58 AM UTC, comment #1: 


> Since (IIRC) 7.x.x, network paths returned by the 'path' command are shown as UNC paths, regardless of what kind of path was fed to the 'addpath' command.


To clarify: That is not correct in general. `canonicalize_file_name` still returns paths starting with the mapped network drive letter unless that path contains a symbolic link that points to a UNC network share that differs from the network share corresponding to the mapped network drive letter. Fwiw, it would also return that UNC path if the symbolic link pointing to a UNC network share would reside on a local disc.

Imho, that behavior of `canonicalize_file_name` is correct and matches how it is documented for POSIX.

If servers are swapped and mapped drive letters or symbolic links would be changed to point to different servers, `canonicalize_file_name` would automatically resolve to those new servers. So, I don't think the second point you raise is something we'd need to worry about.

Which sort of path (drive root or UNC) is easier to understand probably depends on which a user is more exposed to. E.g., where I'm working, UNC paths are used pretty commonly...

Imho in this particular case, we don't need to be 100% Matlab compatible. It's not like an evaluation result would differ because of this difference in behavior.

However, I tend to agree that it might be less surprising if addpath wouldn't change paths that it was fed. However, there is probably a reason why the paths are canonicalized currently. Changing anything revolving around the load path implementation tended to be critical in the past. So, if there is just a "cosmetic" reason for changing this, the "costs" of changing this need to be considered.
Additionally, this only affects very specific use cases (symbolic links that point to different network shares).

Lowering the priority because of this.

Markus Mützel <mmuetzel>
Group administrator
Fri 05 Aug 2022 06:50:39 PM UTC, original submission:  

(split off from bug #62847)

Since (IIRC) 7.x.x, network paths returned by the 'path' command are shown as UNC paths, regardless of what kind of path was fed to the 'addpath' command. That is caused by 'addpath's helper function 'canonicalize_file_name', which cf. posix semantics should always return a UNC path.

OTOH Matlab at my work (we're still on r2014a due to the old graphics stuff, and occasionally use r2018b) returns drive letter search paths for network drives.

This is a call for restoring the old behavior of Octave and make it more Matlab-compatible, for the following reasons:

  • Drive letters are much more parsimonious, they're easier to understand than UNC paths, esp. for the Matlab newbies
  • Drive letters are more permanent than UNC paths. Servers can be swapped, file trees transferred, but drive letters usually persist
  • Matlab's path command returns drive letter paths if we feed it drive letter paths tru 'addpath'
  • (A variant of the previous) it is addpath that adds paths to the path, canonicalize_file_name is just a helper routine. I'd like the result of addpath to resemble as much as possible what I have fed it. For me that makes it a lot easier to e.g., figure out which variant of our Matlab script libraries is actually invoked.


I'd guess (just a hunch) that if canonicalize_file_name, called from addpath, successfully finds a network drive when given a drive letter path, addpath can conclude that the drive letter path was valid and can enter that to the search path rather than the UNC path.
Of course what happens next is quite relevant. If the interpreter must re-morph drive letter paths into UNC paths when searching each separate function, quite a bit of overhead is introduced. As Markus mentioned over in bug #62487, comment #35:

> (A first step would be to check why we are canonicalizing paths in the load path currently. Maybe that is not necessary?)

Again, Matlab doesn't seem to have any issues with it.

I've entered 'Matlab Compatibility' for 'Item Group" but it can be perceived as feature request just as well.

Philip Nienhuis <philipnienhuis>
Group Member

 

(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 ttl (Posted a comment)
  • -email is unavailable- added by nrjank
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by philipnienhuis (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-10-18 philipnienhuis StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2022-10-17 nrjank StatusConfirmed Ready For Test
    2022-08-11 nrjank StatusNone Confirmed
    2022-08-11 nrjank Carbon-Copy- Added ttl
    2022-08-06 mmuetzel Priority5 - Normal 3 - Low

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code