bugGNU Octave - Bugs: bug #60554, is_system_fcn_file fails with...

 
 

bug #60554: is_system_fcn_file fails with short file names

Submitted by:  None
Submitted on:  Sat 08 May 2021 06:37:02 AM UTC  
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  Closed Release:  6.2.0
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)

Thu 13 May 2021 08:37:47 AM UTC, comment #10: 

Seems to be working now.

Closing as fixed.

Markus Mützel <mmuetzel>
Project Member
Mon 10 May 2021 06:18:24 PM UTC, comment #9: 

Before Octave is installed, the fcnfiledir and octfiledir folders might not exist. Thus, their path cannot be canonicalized.
That was the reason, the tests were failing on some buildbots and the GitHub runners.
The buildbots that didn't fail probably have a version of Octave installed.

Hopefully fixed with this change:
https://hg.savannah.gnu.org/hgweb/octave/rev/874430de1cc7

Markus Mützel <mmuetzel>
Project Member
Sat 08 May 2021 07:12:09 PM UTC, comment #8: 

I can't reproduce the error on my build system.

Markus Mützel <mmuetzel>
Project Member
Sat 08 May 2021 06:24:44 PM UTC, comment #7: 

The Debian builders of the default branch seem to be failing the test for bug #40117 since the change from comment #4:

>>>>> processing /scratch/buildbot/workers/jwe-debian-x86_64-2/no-extras-debian/src/test/bug-40117.tst
***** test <*40117>
unwind_protect
   tmp_dir = tempname ();
   mkdir (tmp_dir);
   a_dir = fullfile (tmp_dir, "a");
   a_private_dir = fullfile (a_dir, "private");
   mkdir (a_dir);
   mkdir (a_private_dir);
   __mktestfun_40117__ (fullfile (a_dir, "main_40117.m"),
                        "function r = main_40117 ()",
                        "  r = p1_40117 ();",
                        "endfunction");
   __mktestfun_40117__ (fullfile (a_private_dir, "p1_40117.m"),
                        "function r = p1_40117 ()",
                        "  r = p2_40117 ();",
                        "endfunction");
   __mktestfun_40117__ (fullfile (a_private_dir, "p2_40117.m"),
                        "function r = p2_40117 ()",
                        "  r = 'a_p2_40117';",
                        "endfunction");
   addpath (a_dir);
   assert (main_40117 (), "a_p2_40117");
   ## Update the secondary private function, attempting to avoid
   ## filesystem timestamp resolution problems.
   pause (1);
   __mktestfun_40117__ (fullfile (a_private_dir, "p2_40117.m"),
                        "function r = p2_40117 ()",
                        "  r = 'new function!';",
                        "endfunction");
   ## Force new functions to be found.
   rehash ();
   assert (main_40117 (), "new function!");
unwind_protect_cleanup
   rmpath (a_dir);
   confirm_recursive_rmdir (false, "local");
   rmdir (tmp_dir, "s");
end_unwind_protect
!!!!! regression: https://octave.org/testfailure/?40117
ASSERT errors for:  assert (main_40117 (),"new function!")
  Location  |  Observed  |  Expected  |  Reason
     []       a_p2_40117  new function!   Strings don't match

Same for the Ubuntu GitHub runners.

The Fedora builders and the macOS runner seem to be ok though.

Also the builds from the stable branch seem to be not affected.

Did the `rehash` function break on the default branch?

Markus Mützel <mmuetzel>
Project Member
Sat 08 May 2021 05:56:17 PM UTC, comment #6: 

Good point. I pushed similar changes for those classes here:
https://hg.savannah.gnu.org/hgweb/octave/rev/161daebce243

However, I don't think we should rely on short file names at all to solve bugs with spaces in paths. They are an optional feature in NTFS file systems. And more and more PC/Laptop manufactures (like Dell) seem to disable that feature in the devices they sell. See e.g.: https://octave.discourse.group/t/help-with-mkoctfile/1122

Imho, we should resolve issues with spaces in paths by correctly quoting those paths in the affected commands. This is (partly?) done for mkoctfile in Octave 7.

I think we should use the paths as they are (and not manipulate them by default) unless we know we need it in a certain form like in the case here.

Maybe there are functions in the C++17 filesystem API that make checking if a file is in a subdirectory of a given path easier.
Maybe this?
https://en.cppreference.com/w/cpp/filesystem/relative

In any case, since we are using C++11 currently, I believe the approach with canonicalizing the paths and comparing strings works "good enough" for now. And since we can use static variables here, there shouldn't be any noticeable performance impact.

Markus Mützel <mmuetzel>
Project Member
Sat 08 May 2021 05:17:19 PM UTC, comment #5: 

Thanks, I think that octave_dld_function and octave_mex_function are also affected. Both have is_system_fcn_file () method. Both on their constructors use path functions.
Just a suggestion(taking the reverse direction) : The default paths will be stored as canonicalized form but if a function like mkoctfile needs a short path converts them to short form.

Anonymous
Sat 08 May 2021 03:47:27 PM UTC, comment #4: 

I pushed a change that should be fixing this to stable here:
https://hg.savannah.gnu.org/hgweb/octave/rev/9c76a6aefb42

Marking as ready for test.

Markus Mützel <mmuetzel>
Project Member
Sat 08 May 2021 12:07:09 PM UTC, comment #3: 

I attached a .oct function "is_system_function.cc" that tests "octave_function::is_system_fcn_file ()".

Calling:

is_system_function ('unique')

shows 1 in Octave 5 and 0 in Octave 6.

Anonymous
Sat 08 May 2021 10:15:09 AM UTC, comment #2: 

Oops. Must have hit the wrong button. Here is what I wanted to write:

Octave on Windows isn't using the hard-coded values for the home directory (or it wouldn't be able to be installed in an arbitrary directory).
Instead, it is determining its installation path from the command line.
For Octave 6, the startup scripts convert the installation directory to a short file name (if possible) to avoid potential issues with spaces in paths. See e.g. bug #59923.
If all goes to plan, that will be different for Octave 7.

So, the values you are seeing in your test file are probably the ones that are expected.

The "is_system_fcn_file" function should be able to cope with that situation though.
I thought I remembered reading about a similar issue somewhere. But I can't find a bug report for it atm.

Re-titling the report for the specific issue.

If there are other similar issues we could handle them here or open new dedicated reports.

Markus Mützel <mmuetzel>
Project Member
Sat 08 May 2021 09:57:36 AM UTC, comment #1: 

Octave on Windows isn't using the hard-coded values for the home directory (or it wouldn't be able to be installed in an arbitrary directory). At least, that is the case if started with one of the startup scripts.
Instead, it is using the value of the environment variable OCTAVE_HOME.
For Octave 6, the startup scripts convert the installation directory to a short file name to avoid potential issues with spaces in paths. See e.g. bug #59923.
If all goes to plan, that will be different for Octave 7.

So, the values you are seeing in your test file are the ones that are expected.

The "is_system_fcn_file" function should be able to cope with that though.
I thought I remembered reading about a similar issue somewhere. But I can't find a bug report for it atm.

Re-titling the report to the specific issue.

If you

Markus Mützel <mmuetzel>
Project Member
Sat 08 May 2021 06:37:02 AM UTC, original submission:  

Hello,
It seems that it is decided, since Octave 6, that all paths should be manipulated in canonicalized form. But the hard coded default paths in libinterp/corefcn/defaults.cc are stored in short form on Windows. That can result in the failure of some functions that "octave_user_function::is_system_fcn_file" is one of them.
I attached a .oct function "paths.cc" that lists those paths. For me all of the hard coded paths are printed as short/non-canonicalized form.
Thanks!

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51400:  paths.cc added by None (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)
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2021-05-13 mmuetzel StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2021-05-08 mmuetzel Item GroupIncorrect Result => Regression
    2021-05-08 mmuetzel StatusNone => Ready For Test
    2021-05-08 mmuetzel Release6.1.0 => 6.2.0
        Summaryis_system_fcn_file doesn short names => is_system_fcn_file fails with short file names
    2021-05-08 mmuetzel SummaryOctave default paths stored as short names => is_system_fcn_file doesn short names
    2021-05-08 None Attached File- => Added paths.cc, #51400

    Back to the top


    Powered by Savane 3.7