Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows file references broken in P8 #3233

Closed
astares opened this issue Apr 25, 2019 · 19 comments · Fixed by #3281
Closed

Windows file references broken in P8 #3233

astares opened this issue Apr 25, 2019 · 19 comments · Fixed by #3281

Comments

@astares
Copy link
Member

astares commented Apr 25, 2019

Windows 10

'C:\Dokumente und Einstellungen' asFileReference directories

works in Pharo 7 and gives empty Array (when not readable) - but not anymore in Pharo 8

Same for

'C:\Dokumente und Einstellungen' asFileReference children
@svenvc
Copy link
Contributor

svenvc commented Apr 25, 2019

Does it fail on the space or what ?

Can you be more specific ?

And how come this did not get caught by unit tests ?

@AtharvaKhare
Copy link

Windows 10, Pharo 8 64bit:

Tried 'C:\Program Files' asFileReference and it works.
Tried 'C:\Documents and Settings' asFileReference and it throws FileException '/C:/Documents and Settings'.
err1
err1 5
err2

@astares
Copy link
Member Author

astares commented Apr 25, 2019

@AtharvaKhare

I guess the "Document and Settings" is a shortcut and is not readable in your system. Right?

image

@astares
Copy link
Member Author

astares commented Apr 25, 2019

@svenvc It seems to happen when you have this (or other permission restriced folders) on windows.
But in Pharo 7 it worked while on Pharo 8 it fails - on the same system.

So I guess something changed in the image or FilePlugin.

@AtharvaKhare
Copy link

AtharvaKhare commented Apr 25, 2019

@astares
Yes, "Documents and Settings" is a shortcut, and I do not have permissions to view it (without changing any settings).

Opening a shortcut of some other folder works.

@dionisiydk
Copy link
Contributor

Little observation:
We should introduce kind of UnrecognizedFileException for such cases which will keep unknown errorNumber.
Now it is quite confusing to see base error class.

@akgrant43
Copy link
Collaborator

What's the value of errorNumber?

@astares
Copy link
Member Author

astares commented Apr 25, 2019

When I run

"'C:\Dokumente und Einstellungen' asFileReference children"

errorNumber is -9

@akgrant43
Copy link
Collaborator

Thanks, Torsten. -9 is Can't Open Directory, which is consistent with it being a permissions problem.

On Linux in Pharo 8 I get the same error as shown above when trying to access a directory that I don't have permissions for. But in Pharo 7 I get a DirectoryDoesNotExist error.

I agree with Dennis that we should have a better error (e.g. CantOpenDirectory), but I think that both returning an empty array and raising DirectoryDoesNotExist is incorrect behaviour - the directory clearly does exist, and may contain entries.

Sven: I haven't looked, but am fairly confident that this isn't covered by the existing tests. No file system tests were changed between Pharo 7 & 8 (that I remember).

@astares
Copy link
Member Author

astares commented Apr 25, 2019

@akgrant43 Please note that returning an empty array is correct behavior as I asked for the children or directories within the directory, not the directory itself.
This was working in Pharo 7 and before - but is broken in Pharo 8.

So I'm not asking for the directory itself (which is existing and returned) - but anything below (entries, subdirs, children, ...)

@akgrant43
Copy link
Collaborator

To get the entries below the directory you open the directory and read it, so if you don't have permission to do that you can't know what it contains, whether it is or isn't empty, e.g. if root has access to the files:

$ sudo ls -lR a
a:
total 4
drwx------ 2 root root 60 Apr 26 07:49 b
-rwx------ 1 root root  9 Apr 26 07:49 c.txt

a/b:
total 4
-rw-rw-r-- 1 root root 9 Apr 26 07:49 d.txt

but I don't:

$ ls -R a
ls: cannot open directory 'a': Permission denied

Even if I have access to the specific file, I can't get to it if I don't have access to the containing directory:

$ cat a/b/d.txt
cat: a/b/d.txt: Permission denied

What am I missing?

Cheers,
Alistair

@astares
Copy link
Member Author

astares commented Apr 29, 2019

Simple - you miss the obvious: I talk about WINDOWS here, so a unix example might not help. ;)

It's totally broken for this platform. Whatever has changed it looks like it was never tested on Windows:

FileSystem root directories first inspect

crashes hardly in Pharo 8 but works in Pharo 7

image

@astares
Copy link
Member Author

astares commented Apr 29, 2019

I have a "D:" drive without a CD in it, so I guess it is not readable.

Difference seems to be:

in P7:

FileSystem root directories second size 

this returns 0

In P8 this gives

image

@akgrant43
Copy link
Collaborator

Simple - you miss the obvious: I talk about WINDOWS here, so a unix example might not help. ;)

True, and I realise that, but I don't have easy access to a Windows system, so was hoping that it would translate. Unfortunately it doesn't.

It's totally broken for this platform. Whatever has changed it looks like it was never tested on Windows:

FileSystem root directories first inspect

crashes hardly in Pharo 8 but works in Pharo 7

image

OK, this is a separate issue to what you originally raised.

I'll respond to the next post.

@akgrant43
Copy link
Collaborator

I have a "D:" drive without a CD in it, so I guess it is not readable.

Difference seems to be:

in P7:

FileSystem root directories second size 

this returns 0

In P8 this gives

image

Pharo 7 appears to be inconsistent:

FileSystem root directories second size 

returns 0, but:

FileSystem root directories second inspect 

triggers a "DirectoryDoesNotExist Path / 'D:'". Pharo 8 is at least consistent: it triggers the does not exist error in both cases. Of course, the fact that basic tools like the inspector don't handle it isn't good.

Going back to the original issue about 'C:\Documents and Settings':

Attempting to read the directory in a DOS box:

C:\>dir "Documents and Settings"
 Volume in drive C has no label.
 Volume Serial Number is E801-3564

 Directory of C:\Documents and Settings

File Not Found

Which is consistent with Pharo 8.

C:\>dir /a:h
 Volume in drive C has no label.
 Volume Serial Number is E801-3564

 Directory of C:\

27/05/2015  09:23 AM    <DIR>          $Recycle.Bin
14/02/2019  06:38 PM    <DIR>          Boot
19/01/2011  08:46 AM               211 Boot.BAK
20/01/2013  08:52 PM               355 Boot.ini.saved
06/02/2019  05:01 AM           407,542 bootmgr
11/04/2018  10:29 PM                 1 BOOTNXT
04/08/2018  08:32 PM             8,192 BOOTSECT.BAK
29/04/2019  10:18 PM    <DIR>          Config.Msi
22/08/2013  09:23 AM    <JUNCTION>     Documents and Settings [C:\Users]
...

Shows that it is a "JUNCTION". I'll have to look at this. Pharo 8 handles symbolic links, but I don't remember seeing a junction. (cygwin displays it in the same way as a symbolic link).

I should have added in my previous post: The double "FileDoesNotExist: '/C:/WORLD/...'" is because the GT inspector adds the '..' entry and Pharo 8 isn't handling the pseudo root directory on Windows.

I'll start with that and then dig in to the other issues.

@akgrant43
Copy link
Collaborator

Quick update: I believe I've resolved all of the issues raised above except the "Document and Settings" issue which will require an update to the FileAttributesPlugin to handle Windows junction entries.

I'll add some automated tests and submit a PR.

@astares
Copy link
Member Author

astares commented May 1, 2019

Thank you!

akgrant43 pushed a commit to akgrant43/pharo that referenced this issue May 1, 2019
Unix file systems have a common root which is handled by the OS file system.  Windows doesn't have this, so the Pharo FileSystem needs to have a pseudo-root entry to be consistent across platforms.

Fixes: pharo-project#3233
@akgrant43
Copy link
Collaborator

The attached screen shot shows the new behaviour.

In particular:

FileSystem root directories first inspect

doesn't trigger the FileDoesNotExistException (because FileSystem on Windows now has a root entry and the inspector handles entries that can't be read).

Screenshot from 2019-05-01 11-45-44

PR on the way.

@akgrant43
Copy link
Collaborator

PR: #3281

Issue for the junction directory problem: #3282. This issue should be restricted to the problems with Windows root directory and inspection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants