bugGNU Octave - Bugs: bug #57083, pkg list affected by mixup of 8.3...

 
 

bug #57083: pkg list affected by mixup of 8.3 and LFN path names

Submitter:  Philip Nienhuis <philipnienhuis>
Submitted:  Sat 19 Oct 2019 09:03:12 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
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 29 Oct 2019 12:17:33 AM UTC, comment #27: 

Oops.  Too much cleanup.  I restored the testif block around the BIST test for is_same_file here: https://hg.savannah.gnu.org/hgweb/octave/rev/e7df1df55f0e.


Rik <rik5>
Group administrator
Mon 28 Oct 2019 05:45:53 PM UTC, comment #26: 

@Rik: Thank you for overhauling the "is_same_file" function.
I noticed that you removed the condition (! is_pc ()) from the test:

%!assert (is_same_file ("~", tilde_expand ("~")))


This test fails on Windows. I think this is because "~" is not an alias for the home directory on Windows. It is perfectly fine to create a file or directory with that name.

Markus Mützel <mmuetzel>
Group administrator
Mon 28 Oct 2019 04:18:14 PM UTC, comment #25: 

Philip - see bug #51632

Mike Miller <mtmiller>
Group Member
Sun 27 Oct 2019 06:55:55 PM UTC, comment #24: 

@Markus: It would be great if Octave could deal with spaces and any other odd characters throughout the interpreter.  However, there have been problems in the past with installing Octave in to a directory with spaces and also with accessing external commands, like ghostscript, when the path to the external command contains spaces.  You can use the Savannah Bug Search interface to search for "path spaces" and come up with a list of reports.

At least this one, bug #53124, is still open.  The others have been closed because we have worked around the issues on Windows by requiring that Octave be installed in a path without spaces.  Relaxing that requirement would be nice, but is not essential.

Rik <rik5>
Group administrator
Sun 27 Oct 2019 06:47:06 PM UTC, comment #23: 

@Philip: From a software engineering perspective, limiting the scope of change is a useful principle as it also limits the adverse effects that a change can have.  I think using ispc() to restrict the change to Windows platforms is the correct thing to do because it means we need not write BIST tests for Linux and Mac platforms.

I polished up your changeset and checked it in here https://hg.savannah.gnu.org/hgweb/octave/rev/0cedd1e23c1f.  Marking this bug as fixed and closing report.

Rik <rik5>
Group administrator
Sun 27 Oct 2019 05:55:18 PM UTC, comment #22: 

@Markus: Thank you for the new is_same_file function.  I made a few changes I think improved the implementation in this cset https://hg.savannah.gnu.org/hgweb/octave/rev/24618a510749.

Rik <rik5>
Group administrator
Sun 27 Oct 2019 11:56:53 AM UTC, comment #21: 

@Mike;
Which bug report did you refer to in comment #7 ?
I couldn't find it, I got "drowned" in loads of unrelated reports.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 26 Oct 2019 10:48:16 PM UTC, comment #20: 

New patch attached.

Tested on Linux, Windows and with mxe-octave installer (that will create the global octave-packages file during installation). Listing, uninstalling, installing packages & rebuilding package lists work OK now.

I've put if "(ispc)" blocks around the relevant statements (calls to fixup_paths.m) in pkg.m, install.m and uninstall.m as pkg.m wasn't affected on Linux. W/o them it still works on Linux, so maybe the if's can be dropped.

@Markus, comment #15:
AFAIK there are still some issue when Octave is installed in a path containing spaces in the name, but IIRC that related to invoking command line functions through "system ()" or eval/feval calls.
The mxe-octave NSIS-installer as it stands won't let you install in a path containing spaces.

(file #47751)

Philip Nienhuis <philipnienhuis>
Group Member
Sat 26 Oct 2019 06:15:17 PM UTC, comment #19: 

Good to hear you are making progress.
"dir_in_loadpath" doesn't work anyway: It returns any absolute path whether it is in the load path or not, as long as it exists in the file system.
I wonder where this behaviour is useful... and rather different from what one might expect from its name.

Markus Mützel <mmuetzel>
Group administrator
Sat 26 Oct 2019 06:04:14 PM UTC, comment #18: 

I got my idea to work beautifully now for an existing Window installation.
I just added this private function:

## <texinfo>
function pkg_list = fixup_paths (pkg_list)

  for ii=1:numel (pkg_list)
    pkg_list{ii}.dir = canonicalize_file_name (pkg_list{ii}.dir);
    pkg_list{ii}.archprefix = canonicalize_file_name (pkg_list{ii}.archprefix);
  endfor

endfunction


and added calls to it in pkg.m (rebuild clause in switch), install.m and uninstall.m.

Next:
- testing on linux
- testing installation of a crossbuild. That doesn't need all that build time, I know a few tricks :-) (Besides I can run the mxe "installation" in mxe-octave/dist/ on Linux directly from Windows.

I looked at dir_in_loadpath but that isn't going to work for installing / uninstalling packages (the 2nd part of this extended bug report).

Philip Nienhuis <philipnienhuis>
Group Member
Sat 26 Oct 2019 05:37:13 PM UTC, comment #17: 

@Philip: I get your point. Using is_same_file seems unreasonable in the function you pointed to. But what about the FIXME note?:
"Couldn't dir_in_loadpath() be used here?"
If that function doesn't work atm, we could probably insert a canonicalize_file_name somewhere to fix it.

Markus Mützel <mmuetzel>
Group administrator
Sat 26 Oct 2019 05:20:45 PM UTC, comment #16: 

Thx Markus, turning it into an easy reloctable .oct file is a Good Idea.

BTW I'm entering relatively long posts here to keep the discussion going in order to arrive at an optimal solution. I hope no one is offended or irritated.

is_same_file() won't easily fix the original problem ("pkg list" showing no loaded packages) as the pertinent if conditions are not about comparing files but rather about finding path name strings in a long text constructed by path().

Of course I can do s/th like (here for installed_packages.m):

:
tmppath = strsplit (path (); pathsep);
## Find which packages are loaded and thus appear in the path
for i = 1:numel (installed_pkgs_lst)
  if (any (cellfun (@(x) is_same_file (x, installed_pkgs_lst{i}.dir), tmppath, "uni", 0)))
    ....


(yet untested, but ripe for vectorizing) however similar constructs need to be added in several other places in pkg.m and private/ functions.
I'm hoping that by just choosing one path name style (i.e., LFN) in the octave_packages files the existing code can be kept as-is (with the exception of the strrep()s for replacing fileseps) and more intensive surgery can be avoided.
IOW presently I'm believe more in Rik's philosophy :-)

But we'll see what the best approach is.

AFAICS is_same_file() could be a very useful addition to the user functions. I wonder if it works with symlinks on *nix and (quite relevant but often overlooked) NTFS "joins" and SUBST etc. on Windows.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 26 Oct 2019 02:19:54 PM UTC, comment #15: 

@Philip: Attached is a dynamically loadable version of that function that you can build with

mkoctfile is_same_file.cc


No need to do a complete cross build.

@all: I don't know the reasons why the 8.3 paths are used. But maybe they have become obsolete?
Octave core now can handle paths with spaces and non-ASCII characters on Windows generally.
However, there might still be some places where paths aren't correctly quoted or as such...

(file #47748)

Markus Mützel <mmuetzel>
Group administrator
Sat 26 Oct 2019 01:31:59 PM UTC, comment #14: 

@Markus,

_It sounds like you already identified all points in pkg that need changing_

If only that were true ...

pkg.m and its functions are a beast. It's all every complicated and big and the path names pop up in various places.
I tried to identify all places where a package directory is defined. private/install.m is an obvious candidate but als for "pkg rebuild" in pkg.m itself some fixes are due.

The underlying issue is that mxe-octave builds are configured with 8.3 style path names for OCTAVE_HOME and various prefixes. The reason is avoiding problems when Octave is installed in a path containing spaces in subdir names.
OTOH, AFAICS pkg.m works fine with LFN path names.

My current idea is to do this:
1. grep for 'save (global_list' and 'save (local_list' in share octave/packages/pkg/
2. before each occurrence, add in a call to a new private function
3. create that function which does nothing more than invoke canonicalize_file_name on all path names in in each item of the package struct and replace those path names.

That solution won't interfere with why the 8.3 style path names were configured in the first place, and users can see "real" paths when typing "pkg list". In addition fileseps won't need to be adapted before comparison at various places in pkg.m and friends.

@Markus:
is_same_file could be a solution.
Does it also work for paths rather than files? (I guess so).

I'll first see if I can go along the lines sketched above. I agree with Rik that we'd better use one type of file/path name and I interpret it as hiding 8.3 filenames for user functions.
An mxe crossbuild, needed to get is-same_file working, takes > 2 hours here so is_same_file will have to wait a little anyway.

Later on we can decide what the best option would be.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 26 Oct 2019 12:37:10 PM UTC, comment #13: 

The attached patch adds a new Octave function "is_same_file". Using the underscores in the function name it should be pretty low risk to clash with any future Matlab function.
If that is preferred, we could also make this function internal by adding the double underscores.

@Philip: It sounds like you already identified all points in pkg that need changing. So could you please check if you can use the new function instead of canonicalize_file_name?

(file #47747)

Markus Mützel <mmuetzel>
Group administrator
Sat 26 Oct 2019 12:06:04 PM UTC, comment #12: 

@Rik:
I think the cset for bug #57267 you mentioned in comment #3 has had a few more consequences on Windows than just "pkg list". It turns out "pkg unload" won't work either for packages (global or local) installed in paths with 8.3 style filenames in pkg prefix.

As to "pkg unload" and "pkg list", a crossbuild from Oct. 8 works fine, one from Oct. 17 fails. In between Markus' patches for bug #57267 were pushed.

Some investigation shows that the patch for this bug report has nothing to do with "pkg unload" failing; the patch itself is just fine and harmless, it doesn't change the return arguments of private/installed_packages.m either. But it's just a cosmetic fix.
"pkg unload" and "pkg list" failing are mere manifestations of a deeper lying bug here.

At the bottom of this is that pkg.m on Windows stores 8.3 style filenames in the octave_packages files and for any action compares these 8.3 style path names to the LFN path names that are now in the path. So I tend to think that a more profound fix for pkg.m is required.

AFAICS Octave happily loads .m, .oct and .mex files from paths containing spaces in subdir names. A fix might boil down to simply writing LFN file names (by invoking canonicalize-file_name) to octave_packages files as that's where pkg.m relies on for all its actions.
I experimented with manually adapting all 8.3 tyle path names in octave_packages to LFN ad that seems to solve all problems with pkg.m due to the fix for bug #57267.

I'll try to prepare a better cset.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 26 Oct 2019 12:03:33 PM UTC, comment #11: 

Thank you, Philip, for pointing out this bug.

We have the C++ function "octave::same_file". Maybe it would be good if we exposed this function to Octave script code.
That would probably better fit what is intended here than comparing strings representing file paths.

I could start working on this if nobody minds.

Markus Mützel <mmuetzel>
Group administrator
Sat 26 Oct 2019 11:42:44 AM UTC, comment #10: 

It may be more of a project than anticipated, but I'm sure that the software design should only use unique paths in comparisons.  Until that is done we are going to keep having problems where a choice of representation leads to a miscompare.

Rik <rik5>
Group administrator
Sat 26 Oct 2019 09:56:50 AM UTC, comment #9: 

Hmm, it seems the 8.3 versus LFN issue lies deeper; with the patch "pkg unload" doesn't unload global packages anymore, undoubtedly related to the path representation on Windows. I suppose the rest of pkg.m's functions expects 8.3 style pathnames.

I only saw this now as pkg.m has no BIST tests.

I'll investigate further. I hope a limited adaptation of my patch can fix this.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 25 Oct 2019 06:47:45 PM UTC, comment #8: 

Patch attached. Tested on Linux and Windows.

As regards Mike's comment, the last hunk changes 8.3 pathnames into full (LFN) path names in pkg.m's output as well (see verbatim blocks in comment #6). That is not what Mike meant but it may make the bug he mentioned more prominent, esp. on Windows.
Personally I find the LFN paths much more meaningful than the 8.3 style paths. The latter only fullfil internal Octave tasks (i.e., make Octave stuff work when it is installed in a path containing spaces in the name) but are meaningless for many Windows users.

If only the actual bug fix is desired that last hunk can be dropped.

(file #47741)

Philip Nienhuis <philipnienhuis>
Group Member
Thu 24 Oct 2019 10:22:41 PM UTC, comment #7: 

There is already an open bug report about changing the way the global prefix works, so that the OCTAVE_HOME part is automatically removed / inserted as needed, so that the entire Octave tree is relocatable without needing the "pkg rebuild" step. Some cosmetic work could be done along with that to remove the OCTAVE_HOME from the "pkg list" output.

Mike Miller <mtmiller>
Group Member
Thu 24 Oct 2019 09:46:53 PM UTC, comment #6: 

right, I already got it working on Windows.
pkg list now shows (w. fixed Octave):

>> pkg list
Package Name         | Version | Installation directory
---------------------+---------+-----------------------
     communications  |   1.2.1 | ...\mingw64\share\octave\packages\communications-1.2.1
            control *|   3.2.0 | ...\Octave-6.0.0_20191017L\mingw64\share\octave\packages\control-3.2.0
     data-smoothing  |   1.3.0 | ...\mingw64\share\octave\packages\data-smoothing-1.3.0
:


rather than (w. Octave from before bug):

>> pkg list
Package Name         | Version | Installation directory
---------------------+---------+-----------------------
     communications  |   1.2.1 | ...\OCTAVE~2.0_2\mingw64\share\octave\packages\communications-1.2.1
            control *|   3.2.0 | ...\Octave\OCTAVE~2.0_2\mingw64\share\octave\packages\control-3.2.0
     data-smoothing  |   1.3.0 | ...\OCTAVE~2.0_2\mingw64\share\octave\packages\data-smoothing-1.3.0
:



I need to check the fix on Linux first, somewhere next weekend I'll have a cset.

While there, I find the listing of package installation subdirs not very enlightening. Maybe pkg should first mention Octave's install directory (OCTAVE_HOME) before mentioning any package and/or show global package dirs relative to OCTAVE_HOME or so.
But that's for another bug report.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 24 Oct 2019 09:29:35 PM UTC, comment #5: 

I think you are correct, and we should be comparing apples to apples.  The function canonicalize_file_name will convert any path  to a single, unique representation.  We should use that on any paths we have before comparing them to one another.

Rik <rik5>
Group administrator
Thu 24 Oct 2019 09:17:39 PM UTC, comment #4: 

Your hunch is a good one.

For e.g., geometry I get in the stanza you mention:

tmppath = ....;C:/Programs/Octave/Octave-6.0.0_20191017L/mingw64/share/octave/packages/geometry-4.0.0;...


while the string it is compared with looks like:

debug> strrep (installed_pkgs_lst{i}.dir, '\', '/')
ans = C:/Programs/Octave/OCTAVE~4.0_2/mingw64/share/octave/packages/geometry-4.0.0


but:

debug> strrep (canonicalize_file_name (installed_pkgs_lst{i}.dir), '\', '/')
ans = C:/Programs/Octave/Octave-6.0.0_20191017L/mingw64/share/octave/packages/geometry-4.0.0


gets me the right string.

Looking at it from a little distance I get the impression that the strrep() statements to morph the Windows fileseps to forward slashes serves no purpose anymore here. But the short pathnames (exclusively for Windows, the ones with "...\OCTAVE~4.0_2\..." rather than "...\Octave-6.0.0_20191017L\...") occur elsewhere in the path() as well and appear to clobber up.

Perhaps the way to go here is to use canonicalize_file_name() to reconstruct the (on Windows) native long file names (LFN) everywhere in the private/installed_packages.m function, keep the fileseps as-is and only then compare.
IOW something along the lines of

strfind (path, canonicalize_file_name (installed_pkgs_lst{i}.dir))


(Of course I already see a few gotchas but those can be tackled.)
AFAIU "pkg list" isn't a time-critical function so a little more processing time, if any, won't hurt.

IMO another benefit is that "pkg list" would then return installed package pathnames that are much more understandable to users then the short path names that are returned currently (compare the second verbatim block with the first or third).

What do you think?
If you agree I'll try to come up with a cset along these lines

Philip Nienhuis <philipnienhuis>
Group Member
Wed 23 Oct 2019 10:21:06 PM UTC, comment #3: 

I think you might need to try some sort of bisection.  The code that determines whether a package is loaded is in scripts/pkg/private/installed_packages.m.


  ## Now check if the package is loaded.
  ## FIXME: Couldn't dir_in_loadpath() be used here?
  tmppath = strrep (path (), '\', '/');
  for i = 1:numel (installed_pkgs_lst)
    if (strfind (tmppath, strrep (installed_pkgs_lst{i}.dir, '\', '/')))
      installed_pkgs_lst{i}.loaded = true;
    else
      installed_pkgs_lst{i}.loaded = false;
    endif
  endfor
  for i = 1:numel (local_packages)
    if (strfind (tmppath, strrep (local_packages{i}.dir, '\', '/')))
      local_packages{i}.loaded = true;
    else
      local_packages{i}.loaded = false;
    endif
  endfor
  for i = 1:numel (global_packages)
    if (strfind (tmppath, strrep (global_packages{i}.dir, '\', '/')))
      global_packages{i}.loaded = true;
    else
      global_packages{i}.loaded = false;
    endif
  endfor


If I had to guess, the problem is that something has changed in the way paths are represented on Windows.  Either this is in path() or in the calculation of local_packages and global_packages variables.

The most likely suspect is this change on Oct. 9th


changeset:   27497:5a0543de1e47
user:        Markus Mützel <markus.muetzel@gmx.de>
date:        Wed Oct 09 14:35:03 2019 +0200
summary:     Add canonical path when using addpath (bug #56267).




Rik <rik5>
Group administrator
Wed 23 Oct 2019 09:48:03 PM UTC, comment #2: 

Makes no difference.
I just made a new build, after I manually wiped all Octave build files from the "install" dir (mingw64/) in the mxe-octave build tree and tried again.

I have no mxe builds between 8 and 17 Oct so it is a bit hard to "bisect".
The installed versions (8, 18 and 23 Oct) all use the same user .octaverc and the one from 8 Oct works fine with it so it must be in Octave itself.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 23 Oct 2019 03:48:56 PM UTC, comment #1: 

This doesn't appear to happen on Linux, so this is MS Windows specific.

Is there any difference if you specifically load a package, versus loading a package like optim that depends on several others and autoloads the ones it needs?

Rik <rik5>
Group administrator
Sat 19 Oct 2019 09:03:12 AM UTC, original submission:  

Since a few days, when listing the OF packages with "pkg list", loaded global packages aren't shown with an "*" asterisk.
The last installed version where it worked fine was a tip from Oct. 8.

That some globally installed packages are loaded is easily verified using "which <some_loaded_package_function>"

:
              nurbs  |  1.3.13 | ...\mingw64\share\octave\packages\nurbs-1.3.13
                ocs  |   0.1.5 | ...\mingw64\share\octave\packages\ocs-0.1.5
            octproj *|   1.1.5 | C:\Users\philip\octave\octproj-1.1.5
             odepkg  |   0.8.5 | ...\mingw64\share\octave\packages\odepkg-0.8.5
             optics  |   0.1.4 | ...\mingw64\share\octave\packages\optics-0.1.4
              optim  |   1.6.0 | ...\mingw64\share\octave\packages\optim-1.6.0
:
:
              video  |   1.2.4 | ...\mingw64\share\octave\packages\video-1.2.4
            windows  |   1.3.1 | ...\mingw64\share\octave\packages\windows-1.3.1
             zeromq  |   1.5.0 | ...\mingw64\share\octave\packages\zeromq-1.5.0
>> which actxserver
'actxserver' is a function from the file C:\Programs\Octave\Octave-6.0.0_20191018L\mingw64\lib\octave\packages\windows-1.3.1\x86_64-w64-mingw32-api-v53+\__COM__.oct
>>


Formally I think it's a regression but this bug is merely cosmetic.

Philip Nienhuis <philipnienhuis>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #47751:  bug_57083_pkg_LFN_paths.cset added by philipnienhuis (6KiB - application/octet-stream)
file #47748:  is_same_file.cc added by mmuetzel (2KiB - application/octet-stream)
file #47747:  bug57083_is_same_file.patch added by mmuetzel (2KiB - application/octet-stream)

 

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 rik5 (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 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-10-29 rik5 StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2019-10-28 mmuetzel StatusFixed In Progress
        Open/ClosedClosed Open
    2019-10-27 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2019-10-26 philipnienhuis Attached File- Added bug_57083_pkg_LFN_paths.cset, #47751
        StatusIn Progress Patch Submitted
    2019-10-26 mmuetzel Attached File- Added is_same_file.cc, #47748
    2019-10-26 mmuetzel Attached File- Added bug57083_is_same_file.patch, #47747
    2019-10-26 philipnienhuis Summarypkg list does not indicate loaded global packages pkg list affected by mixup of 8.3 and LFN path names
    2019-10-26 philipnienhuis StatusPatch Submitted In Progress
    2019-10-25 philipnienhuis Attached File- Added bug_57083_pkg_canonicalize_file_name.cset, #47741
        StatusNone Patch Submitted
    2019-10-23 rik5 Item GroupNone Regression

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code