bugGNU Octave - Bugs: bug #56267, addpath() on Windows fails to...

 
 

bug #56267: addpath() on Windows fails to recognize same subdirs with different fileseps and case

Submitted by:  Philip Nienhuis <philipnienhuis>
Submitted on:  Sun 05 May 2019 08:41:43 AM UTC  
 
Category:  Octave Function Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Philip Nienhuis Open/Closed:  Closed
Release:  dev Operating System:  Microsoft Windows

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Tue 12 Nov 2019 09:40:34 PM UTC, comment #25: 
Philip Nienhuis <philipnienhuis>
Project Member
Tue 12 Nov 2019 09:04:55 PM UTC, comment #24: 

"edit (canonicalize_file_name ('curr_prefix.m'))" (w/o double quote) does work,
"edit curr_prefix" doesn't.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 12 Nov 2019 09:02:19 PM UTC, comment #23: 

This isn't really related to the change for this bug report. Maybe it would be better to have a dedicated bug number...

Markus Mützel <mmuetzel>
Project Member
Tue 12 Nov 2019 09:00:08 PM UTC, comment #22: 

New bug report, or just prolong this one?

AFAICS the only difference in the picture I attached was forward vs. backward slashes.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 12 Nov 2019 08:59:56 PM UTC, comment #21: 

I used the prefix "\\?\" to remove the MAX_PATH limitation [1].
However, this seems to work with absolute paths only.

The following change should fix it:

diff -r e8e7ec705418 liboctave/util/kpse.cc
--- a/liboctave/util/kpse.cc        Wed Nov 06 20:07:55 2019 +0100
+++ b/liboctave/util/kpse.cc        Tue Nov 12 21:58:14 2019 +0100
@@ -169,7 +169,7 @@
{
#if defined (OCTAVE_USE_WINDOWS_API)

-  std::wstring w_fn = L"\\\\?\\" + octave::sys::u8_to_wstring (fn);
+  std::wstring w_fn = octave::sys::u8_to_wstring (fn);

   DWORD f_attr = GetFileAttributesW (w_fn.c_str ());

I'll try to cross-build before I commit.

[1]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfileattributesa

Markus Mützel <mmuetzel>
Project Member
Tue 12 Nov 2019 08:12:58 PM UTC, comment #20: 

"file_in_path" seems to be broken currently on Windows if the first argument is '.'.
I'm trying to find out what has caused the issue. Maybe this change:
http://hg.savannah.gnu.org/hgweb/octave/rev/12f28f9368ec

Markus Mützel <mmuetzel>
Project Member
Tue 12 Nov 2019 07:07:55 PM UTC, comment #19: 

Did I just hit another manifestation of this bug? (it doesn't seem to canonicalize "."). If not I'll open a new bug report.

See attached pic, where "which" can find curr_prefix.m but the editor can't.


Philip Nienhuis <philipnienhuis>
Project Member
Sat 02 Nov 2019 03:51:34 PM UTC, comment #18: 

Hmm. Maybe we could use the shortened paths only if "path" is called without input and output arguments. But I'm still undecided whether that is a good idea.
Please, open a new report for that feature request.

Closing this report as fixed.

Markus Mützel <mmuetzel>
Project Member
Sat 02 Nov 2019 02:05:34 PM UTC, comment #17: 

OK, all paths in the output of "path" are now LFN, good & thanks.

That brings up a second question:
I now some some very long paths listed:

:
:
L:\philip\devel\octdev\mxe\mxe_64w_20190901\dist\octave\mingw64\share\octave\6.0.0\m\testfun
L:\philip\devel\octdev\mxe\mxe_64w_20190901\dist\octave\mingw64\share\octave\6.0.0\m\time
L:\philip\devel\octdev\mxe\mxe_64w_20190901\dist\octave\mingw64\share\octave\6.0.0\m\web
L:\philip\devel\octdev\mxe\mxe_64w_20190901\dist\octave\mingw64\share\octave\6.0.0\data

>> OCTAVE_HOME
ans = L:\philip\devel\octdev\mxe\mxe_64w_20190901\dist\octave\mingw64
>>

(in case you wonder, L: is my Linux /home partition mounted in windows using ext2fsd)

I'm wondering if there's any support for shortening those by using e.g., <OCTAVE_HOME> as first entry, so the listing could be

:
:
<OCTAVE_HOME>\share\octave\6.0.0\m\testfun
<OCTAVE_HOME>\share\octave\6.0.0\m\time
<OCTAVE_HOME>\share\octave\6.0.0\m\web
<OCTAVE_HOME>\share\octave\6.0.0\data
>>

m-file example code to get this together is in my pending patch for bug #51632, second hunk for private/installed_packages.m.
For load-path.cc it obviously needs translation to C++.

Could be a new bug report / feature request. Shall I open one?

Philip Nienhuis <philipnienhuis>
Project Member
Fri 01 Nov 2019 08:59:44 PM UTC, comment #16: 

Philip reported in bug #51632 that the paths set with the function "path" are not canonicalized. Also "rmpath" doesn't remove the canonicalized version of the path.

The following patch does a better job of keeping only canonicalized paths in the list of load paths (if possible). It also converts paths to canonical paths before comparing them to list members:
http://hg.savannah.gnu.org/hgweb/octave/rev/51d26dd80828

Markus Mützel <mmuetzel>
Project Member
Sat 26 Oct 2019 11:42:33 AM UTC, comment #15: 

@Markus:
Your fix works fine, thanks.
I tried the examples in my original report in various combinations of case and fileseps but Octave won't get fooled anymore, I only get one instance of the subdir in question in the path.
Fine, so this bug can be closed I think.

Just as a side note, I've hit issues with it that seemed unrelated at first.

On Windows, 8.3 style path names are used to make Octave cope with spaces in path names. Your patch morphs these 8.3 style path names into LFN (long file name) path names and it now turns out that pkg.m is subsequently affected by a mixup of 8.3 and LFN path names.
Further discussion of those effects are in bug #57083, as I think pkg.m needs adaptation and your big fix here is what needed to be done.

Philip Nienhuis <philipnienhuis>
Project Member
Wed 16 Oct 2019 05:48:48 PM UTC, comment #14: 

Thanks Markus, I'll try the fixes asap.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 15 Oct 2019 08:10:33 PM UTC, comment #13: 

Found the error and applied the two patches here:
http://hg.savannah.gnu.org/hgweb/octave/rev/def608acdfa9
http://hg.savannah.gnu.org/hgweb/octave/rev/5a0543de1e47

Marking as ready for test.

Markus Mützel <mmuetzel>
Project Member
Tue 15 Oct 2019 08:17:51 AM UTC, comment #12: 

The modifications to addpath seem to break it. I'll have a look into it.
The first patch modifying canonicalize_file_name should be fine though.

Markus Mützel <mmuetzel>
Project Member
Wed 09 Oct 2019 12:40:14 PM UTC, comment #11: 

The attached patch canonicalizes the path before adding it to the search path in addpath. It should be applied on top of file #47568.

@Rik: Is this the kind of modification to addpath you were thinking about?

(file #47632)

Markus Mützel <mmuetzel>
Project Member
Tue 24 Sep 2019 06:53:02 PM UTC, comment #10: 

The attached patch is basically the same as the previous one apart from it also putting the drive letter in upper case.

Fwiw, "canonicalize_file_name" never supported UNC paths.

(file #47568)

Markus Mützel <mmuetzel>
Project Member
Sat 21 Sep 2019 04:36:40 PM UTC, comment #9: 

With the attached path, I see the following on Windows 10 64bit:

>> canonicalize_file_name ('C:\WINDOWS')
ans = C:\Windows
>> canonicalize_file_name ('C:\WINDOWS\SYSTEM32')
ans = C:\Windows\System32
>> canonicalize_file_name ('c:\WiNdOwS/SYSWOW64')
ans = c:\Windows\SysWOW64

It doesn't work for UNC paths though. However, I don't know whether we are supporting these anyway.

It also doesn't change the case of the drive letter. Maybe we should always return a capital letter?

I am also not sure whether it is OK to conditionally include windows.h in the core file.

Octave 6 crashes when running the test suite (with and without the patch). Therefore, it is hard to judge whether it is save to apply this change. The tests that were running until the crash looked ok.

(file #47557)

Markus Mützel <mmuetzel>
Project Member
Fri 20 Sep 2019 03:02:41 PM UTC, comment #8: 

As we require that a directory already exists when we add it to the path, we might be able to call GetLongPathNameW [1] to query (and add) the "canonical" path of that directory.

Fwiw, here is what I see on Windows 10 with Octave 5.1.0:

>> canonicalize_file_name ('C:\USERS\Markus')
ans = C:\USERS\Markus
>> canonicalize_file_name ('C:\Users\Markus')
ans = C:\Users\Markus

[1]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getlongpathnamew

Markus Mützel <mmuetzel>
Project Member
Thu 09 May 2019 07:18:38 PM UTC, comment #7: 

Enjoy your, I hope, vacation.  Can take this up later.

Rik <rik5>
Project Administrator
Thu 09 May 2019 07:09:09 PM UTC, comment #6: 

if you look at what javaaddpath does in comment #2 it turns out it does the right thing. It checks with the OS or the exact case.
Whether there are other checks that make sure the file name matches I don't know.

Anyway I'm out for  week.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 09 May 2019 05:51:25 PM UTC, comment #5: 

If it does a verbatim copy then canonicalize_file_name handles only the file separator part of the duplication.  But it would still be possible to add two equivalent directory with different cases

addpath ('C:\USERS')
addpath ('C:\Users')
path

Rik <rik5>
Project Administrator
Thu 09 May 2019 05:39:35 PM UTC, comment #4: 

> What does this return on Windows?


(Didn't try, no time, I'll be away for a week.)

But see comment #2, 2nd verbatim block. It does the right thing, i.e., verbatim copy the actual path including the right case.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 09 May 2019 05:31:40 PM UTC, comment #3: 

I checked on the code in javaclasspath.m and it is:

for i = 1:numel (varargin)
  clspath = varargin{i};
  if (! ischar (clspath))
    error ("javaaddpath: CLSPATH must be a string");
  endif

  new_path = canonicalize_file_name (tilde_expand (clspath));
  if (isfolder (new_path))
    if (new_path(end) != filesep ())
      new_path = [new_path, filesep()];
    endif
  elseif (! exist (new_path, "file"))
    error ("javaaddpath: CLSPATH does not exist: %s", clspath);
  endif

  success = javaMethod ("addClassPath", "org.octave.ClassHelper", new_path);

So, just as suspected, it calls canonicalize_file_name to make the name standard before adding it to the path, and that standardization eliminates the duplicates that differ by file separator.

Does canonicalize_file_name also handle case?

What does this return on Windows?

canonicalize_file_name ('C:\USERS\PHILIP')

Rik <rik5>
Project Administrator
Tue 07 May 2019 06:15:17 AM UTC, comment #2: 

Thanks for picking up, Rik.

>> canonicalize_file_name ('C:/Users/Philip')
ans = C:\Users\Philip
>> canonicalize_file_name ('C:\Users\Philip')
ans = C:\Users\Philip
>>

What matters here is simply consistent fileseps and ignoring casing.
I checked with javaaddpath: that is doing the exact right thing:

GNU Octave, version 6.0.0
:
>> javaclasspath
   STATIC JAVA PATH

      - empty -

   DYNAMIC JAVA PATH

      C:\Users\philip\java\poi-4.0.0.jar
      C:\Users\philip\java\poi-ooxml-4.0.0.jar
:
>> javaaddpath('C:/Users/Philip/java/poi-4.0.0.jar')
>> javaclasspath
   STATIC JAVA PATH

      - empty -

   DYNAMIC JAVA PATH

      C:\Users\philip\java\poi-4.0.0.jar
      C:\Users\philip\java\poi-ooxml-4.0.0.jar
:

so there we have example code that can be mimicked.

Philip Nienhuis <philipnienhuis>
Project Member
Mon 06 May 2019 10:35:37 PM UTC, comment #1: 

In Octave, what does this return?

canonicalize_file_name ('C:/Users/Philip')
canonicalize_file_name ('C:\Users\Philip')

If they return the same string then we could add a call to canonicalize_file_name in addpath before checking whether a path is a duplicate.

Rik <rik5>
Project Administrator
Sun 05 May 2019 08:41:43 AM UTC, original submission:  

AFAIK addpath() normally recognizes if its argument is already in the PATH and if so, doesn't add it to the PATH a second time.
But on Windows, where both '\' and '/' are accepted as file separators and path-/filenames are case-insensitive, it gets confused by different fileseps and different case in otherwise identical arguments, so the PATH can end up as follows:

>> path

Octave's search path contains the following directories:

.
C:/Users/Philip/Desktop/getij/m-files
C:/Users/Philip/Desktop/Getij/m-files
C:\Users\philip\desktop\getij\m-files
C:\Programs\Octave\OCTAVE~1.0_2\mingw64\share\octave\packages\windows-1.3.1
:

Of course it's no big deal, Octave works fine anyway, but it is unneeded cluttering of the search path.

Severity => Minor

Philip Nienhuis <philipnienhuis>
Project Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #47632:  bug56267_part2_canonicalize_addpath.patch added by mmuetzel (2KiB - application/octet-stream)
file #47568:  bug56267_win32_canonicalize_case_v2.patch added by mmuetzel (1KiB - application/octet-stream)
file #47557:  bug56267_win32_canonicalize_case.patch added by mmuetzel (1KiB - application/octet-stream)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by philipnienhuis (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-11-12 philipnienhuis Attached File- => Added File_not_found.PNG, #47851
    2019-11-04 philipnienhuis Dependencies- => bugs #57163 is dependent
    2019-11-02 mmuetzel StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2019-10-16 rik5 Carbon-CopyRemoved 72865 => -
    2019-10-15 mmuetzel StatusPatch Submitted => Ready For Test
    2019-10-09 mmuetzel Attached File- => Added bug56267_part2_canonicalize_addpath.patch, #47632
        StatusNone => Patch Submitted
    2019-09-24 mmuetzel Attached File- => Added bug56267_win32_canonicalize_case_v2.patch, #47568
    2019-09-21 mmuetzel Attached File- => Added bug56267_win32_canonicalize_case.patch, #47557

    Back to the top


    Powered by Savane 3.5