search

PyFilesystem / pyfilesystem

Python filesystem abstraction layer
http://pyfilesystem.org/
BSD 3-Clause "New" or "Revised" License
287 stars 63 forks source link

Fixing dokan support discussion... #256

Open apocalypse2012 opened 8 years ago

apocalypse2012 commented 8 years ago

Python 2.7.9 64 bit and 32 bit Dokan 0.7.4 FS 0.5.4 Windows 8.1

from fs.memoryfs import MemoryFS from fs.expose import dokan fs = MemoryFS() mp = dokan.mount(fs,"Q") Traceback (most recent call last): File "", line 1, in File "C:\Python27\lib\site-packages\fs\expose\dokaninit.py", line 936, in mount check_ready(mp) File "C:\Python27\lib\site-packages\fs\expose\dokaninit.py", line 913, in check_ready raise OSError("dokan mount process seems to be hung") OSError: dokan mount process seems to be hung

dokan.mount(fs,"Q",foreground=True) Traceback (most recent call last): File "", line 1, in File "C:\Python27\lib\site-packages\fs\expose\dokaninit.py", line 929, in mount raise OSError("Dokan failed with error: %d" % (res,)) OSError: Dokan failed with error: -5

I tried the newer versions before I realized that 8.0 was API breaking. I've read through all the message boards I can find with no apparent 'fix'. Dokan 0.6.0 no longer appears to be available on the Dokan project page at Github. I

lurch commented 8 years ago

Somebody started working on updated Dokan support (#241 and #236) but I dunno if they ever got any further :-/ Ping @zommuter

See also https://github.com/PyFilesystem/pyfilesystem/search?q=dokan&state=open&type=Issues

zommuter commented 8 years ago

Thanks for the ping, @lurch. Unfortunately that is currently at the very bottom of my TODO list, and lacking more experience with Dokan I doubt I can fix this on my own :(

Liryna commented 8 years ago

:+1: I am willing to help on this ! @arekbulski I seen you forked pyfilesystem, did you plan to update dokan implentation ?

I will try to take a look at it in the next days and see what changes should be made. Dokan 1.0.0 should be implemented since it is near to be release stable.

lurch commented 8 years ago

It would be great to see working Dokan support in pyfilesystem again :-)

arekbulski commented 8 years ago

I am willing to review pyfilesystem, especially documentation. But I am also looking forward to seeing Dokan working with pyfilesystem, tell me how can I help. @Liryna

Liryna commented 8 years ago

@arekbulski Ok, I just wanted to know if you planned to update the wrapper yourself.

I began to update most of the natives commandes https://github.com/Liryna/pyfilesystem without implementing it

from ctypes import *

try:
    DokanMain = windll.Dokan.DokanMain
    DokanVersion = windll.Dokan.DokanVersion
except AttributeError:
    raise ImportError("Dokan DLL not found")

Where does the dllimport happen ? How to set library name ?

arekbulski commented 8 years ago

You import windll from ctypes, which is a library loader. Member name is used as library name that is DLL name to be loaded. And for the library object, member name is used as function name. Simple example:

In [13]: ctypes.windll
Out[13]: <ctypes.LibraryLoader at 0x247b7dcdc88>
In [11]: ctypes.windll.msvcrt
Out[11]: <WinDLL 'msvcrt', handle 7ff829220000 at 0x247b8359f98>
In [12]: ctypes.windll.msvcrt.memset
Out[12]: <_FuncPtr object at 0x00000247B829FEE8>

In your code Dokan is the library name, DokanMain/DokanVersion are imported functions. You might limit it to from ctypes import windll if you choose.

Liryna commented 8 years ago

Thank you for your answer @arekbulski :heart: So windll.dokan1 should load dokan1.dll ?

arekbulski commented 8 years ago

I think so, one way to know for sure.

Liryna commented 8 years ago

Yes I will make the test :smiley: I don't have access to my dev environment so I am just reviewing the code for now.

Liryna commented 8 years ago

I am currently able to start the mount of a dokan device with my last changes https://github.com/Liryna/pyfilesystem I see that Mounted operation is called, the python is correctly called but I get the error run-time check failure the value of ESP directly when the function return here: https://github.com/Liryna/pyfilesystem/blob/master/fs/expose/dokan/__init__.py#L805 I understood that this error means that the mapping python <-> C is not correct but I am unable to find what I did wrong. :( (first time I do this :smile: )

If someone could guide me, it would be nice !

https://github.com/dokan-dev/dokany/blob/master/dokan/dokan.h#L111 https://github.com/Liryna/pyfilesystem/blob/master/fs/expose/dokan/libdokan.py#L112

arekbulski commented 8 years ago

Throw it at Stack Overflow. I am not familiar enough with it.

I do not understand why USHORT is not taken from c_ushort.

Liryna commented 8 years ago

Probably in case USHORT was not define in earliest version of ctypes. I removed it and forced c_ushort, same behaviour :cry: @lurch @zommuter are you familiar with ctypes ?

arekbulski commented 8 years ago

Not surprised. There is no point in supporting dinosaur-old versions of Python.

Liryna commented 8 years ago

My bad, I found that all call work with dokan1.dll in Release mode and not in Debug as I was doing. Now I get proper call from dokan.

CreateFile fail here https://github.com/Liryna/pyfilesystem/blob/master/fs/expose/dokan/__init__.py#L453 when \ directory is requested 

execpt Path contains invalid characters: \

I am not familiar with fs api and the code seems to not have been updated since 3 years (dokan part). It there one of the pyfilesystem that could help me to review fs api calls ?

arekbulski commented 8 years ago

Here is the isdir() from MemoryFS and normpath() that it uses. From what I read in these methods, fs objects require/expect paths formatted in their way. Notice that normpath does not recognize backslash at all. It is not recognized as one of ('..', '.', '') so it gets passed as a component, normpath('\') should give '/' right? isdir('\') will give False which will lead to further problems.

https://github.com/Liryna/pyfilesystem/blob/master/fs/memoryfs.py#L321 https://github.com/Liryna/pyfilesystem/blob/master/fs/path.py#L20 https://github.com/Liryna/pyfilesystem/blob/master/fs/path.py#L66

Liryna commented 8 years ago

:+1: @arekbulski You are exactly right normpath('\') is giving '\' :cry:

I had to add in all functions: path = path.replace('\', '/')

I don't know if this is the best way but I am now able to run the test.

I am just not able to write the test file and will look into this.

Liryna commented 8 years ago

Pull request created: https://github.com/PyFilesystem/pyfilesystem/pull/258

I am able to Create/Delete/List/Rename/Read/Write files without issues. As an example, Word 2016 successfully open a file, read/write and save it.

arekbulski commented 8 years ago

Well, it is you who is doing all the hard work. I am just good at code analysis. :smiley:

Liryna commented 8 years ago

A pleasure :+1: I always wanted dokan being compatible with python.

lurch commented 8 years ago

normpath('\') is giving '\'

That's correct - as far as pyfilesystem is concerned, a file named \ is totally fine. 'input paths' to pyfilesystem should always be separated only by / characters ( http://docs.pyfilesystem.org/en/latest/concepts.html#paths ), and any conversion to/from backslashes (to handle the underlying OS / API calls) should be done inside the FS code. Have a look at OSFS to see if that helps?

Liryna commented 8 years ago

You are right self.fs.isdir (OSFS) does raise the error at some point with \ as input path. Does that mean OSFS should be fixed to handle such case ? I will remove the replace if thats the case.

lurch commented 8 years ago

Does that mean OSFS should be fixed to handle such case ?

No. As I already tried to explain, pyfilesystem has the concept of (abstract) "pyfilesystem paths", and (concrete) "system paths". A pyfilesystem path might be something like /some/dir/file.txt and on Linux this might map to a (OSFS) system path of /home/lurch/testing/some/dir/file.txt, and on Windows it might map to a (OSFS) system path of C:\Users\Andrew\Documents\testing\some\dir\file.txt. The user-code talking to pyfilesystem should only ever use forward-slashes to separate directories, and it's up to the library-code inside pyfilesystem to do any necessary manipulations to convert the pyfilesystem-paths to and from system-paths as necessary.

It's kinda difficult to explain clearly, but it means that user-code making use of pyfilesystem is always identical, no matter what platform it's running on, and what the backend filesystem is. (and this is also why pyfilesystem has no 'current directory' concept, because some of the backend filesystems don't support that)

Liryna commented 8 years ago

All right :+1: , so If I understand correctly my wait to convert path coming from Dokan CreateFile/Cleanup/Close operation like \, \folder, \folder\file.txt to "pyfilesystem paths" with path.replace('\\', '/') is correct ?

If not, I have no idea what to do since dokan directly send the path like that and there is no way to change it.

arekbulski commented 8 years ago

I totally understand and agree with the concept behind abstract paths. However on Windows backslashes must be converted to a recognized separator or path splitting will lead to incorrect results. Remeber Dokan is Windows only. Btw windows syscalls work with slashes just as well as far I was told.

lurch commented 8 years ago

I'm afraid I've never looked at any of the 'expose' classes in pyfilesystem, so maybe I'm getting confused between which paths are pyfilesystem-paths, and which paths are system-paths? :-S

But anyway, IMHO it'd be much cleaner to have just a couple of functions that convert Dokan-paths to and from pyfilesystem-paths, and use those in each of the other functions; rather than sprinkling path.replace calls everywhere ;)

Given that you've updated all the docstrings to say that the Dokan class now supports mounting to an arbitrary path, rather than (just) a drive-letter, have you tested that? Does there need to be any code adding and removing this extra path-info from the Dokan-paths? (apologies if that's a stupid question, but I've never looked at dokan code)

Liryna commented 8 years ago

I have added the extract method to convert paths.

lurch commented 8 years ago

Should this issue be closed, and progress tracked in #258 instead?

arekbulski commented 8 years ago

No, let is use this Issue as a floor for free discussion and PR for specific code changes. Could you change the title to something more like "Dokan main thread"?

lurch commented 8 years ago

@Liryna Have you tested mounting to a path instead of to a drive? (see https://github.com/PyFilesystem/pyfilesystem/pull/258#discussion-diff-72960386 and https://github.com/PyFilesystem/pyfilesystem/pull/241#issuecomment-170021043 ). Could you update the docstrings to clarify how it works?

Liryna commented 8 years ago

doc updated with an example. Yes, this has been tested. It make no difference for the python. everything is transparent.

lurch commented 8 years ago

Thanks for the doc-update :+1:

Liryna commented 8 years ago

Et voilà ! :smiley: Added space

arekbulski commented 8 years ago

@Liryna Could you look at these issues and advise what to do about them? I am starting to see you as our main and only Dokan maintainer, if that is alright? I will try to work on those too.

159 #175 #150 #149 #110

Liryna commented 8 years ago

@lurch could you make a full review of the pull request in one time ? This will make us win some time. Keep in mind that this pull request add support of dokan 1.0.0.

Further changes should be made in another contribution.

Liryna commented 8 years ago

Can be close https://github.com/PyFilesystem/pyfilesystem/issues/159, https://github.com/PyFilesystem/pyfilesystem/issues/150 Not dokan https://github.com/PyFilesystem/pyfilesystem/issues/175 Never seen this behaviour and probably python related https://github.com/PyFilesystem/pyfilesystem/issues/149, https://github.com/PyFilesystem/pyfilesystem/issues/110 should be tested otherwise can be closed

arekbulski commented 8 years ago

Sorry, I am too sleepy and tired to read clearly. Pardon (trying french). Ping @lurch Lets close some!

lurch commented 8 years ago

@lurch could you make a full review of the pull request in one time ? This will make us win some time.

Sorry, I was just commenting on things as and when I saw them. I wasn't expecting you to react to my comments so quickly! ;-)

lurch commented 8 years ago

Closed a couple of issues (thanks!), but will leave the others open pending further testing.

@Liryna If I do get round to testing your updated Dokan support, and it all seems to work okay, do you mind if I wait until Dokan 1.0 is officially released before merging #258 , rather than relying on the Dokan RC version?

arekbulski commented 8 years ago

I think there is no point. Versioning schemes used in practice are mumbo jumbo anyway. RC is just another way of saying stable but had major changes. Same goes for 0.9 not being less stable than 1.0, its just previous number +1. Those are just fancy numbers. Tell me, am I wrong on this?

lurch commented 8 years ago

To me, RC signifies ReleaseCandidate, and implies "this is very close to the final code, so please help us test it, and we'll be releasing the real final version very soon". I'd rather wait until Dokan is non-RC, so that people (i.e. users of pyfilesystem wanting to experiment with Dokan) will be installing the final release of Dokan, rather than the soon-to-be-replaced RC version. I have no concerns about stability, as I assume Dokan being on RC4 means that most of the major bugs have been found and fixed already.

Liryna commented 8 years ago

Current pyfilesystem implementation of dokan is only compatible with dokan 0.5.6. I am even not aware if there is still somewhere a valid link to download this version. Even if someone found it, it is by far unstable and probably not correctly working on current windows version.

As you whish to keep this implementation until dokan 1.0.0 is released as stable. Current RC and the next stable will have only fix reported by the community but API will remain as it is.

Le 2 août 2016 1:15 AM, "Andrew Scheller" notifications@github.com a écrit :

To me, RC signifies ReleaseCandidate, and implies "this is very close to the final code, so please help us test it, and we'll be releasing the real final version very soon". I'd rather wait until Dokan is non-RC, so that people (i.e. users of pyfilesystem wanting to experiment with Dokan) will be installing the final release of Dokan, rather than the soon-to-be-replaced RC version. I have no concerns about stability, as I assume Dokan being on RC4 means that most of the major bugs have been found and fixed already.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/PyFilesystem/pyfilesystem/issues/256#issuecomment-236736831, or mute the thread https://github.com/notifications/unsubscribe-auth/AEHJe-NHdBI1HkeL4-bKBddW-qYzAjrMks5qbn4kgaJpZM4JNAUY .

lurch commented 8 years ago

It's not that I "don't trust" the current RC version of Dokan, but surely it'd be better to advise end-users of pyfilesystem to just go and install https://github.com/dokan-dev/dokany/releases/latest (which is what https://github.com/dokan-dev/dokany/wiki/Installation recommends) rather than expecting them to seek out whatever the latest pre-release version is?

Yes, I'm aware the current Dokan support in pyfilesystem is horribly broken, which is why I'm looking forward to being able to merge this (hopefully) working version :-) How far away is the final release of Dokan stable?

arekbulski commented 8 years ago

Liryna made the same point. Code we are working on will work with past-RC versions as well. There is no point in keeping it back.

arekbulski commented 8 years ago

@Liryna How about you mention what Lurch is talking about in the docstring? "Latest dokan binary needs to be installed, RC or otherwise."

apocalypse2012 commented 8 years ago

As an end user, I would prefer access. Document the dependency details, certainly. But it is a reasonable assumption that an RC has a stable interface at least. It is easy for me to understand that the docker version required is not production ready, but I still need to develop my code in the meantime. Right now, I can't.

Sent from my iPhone

On Aug 2, 2016, at 4:38 AM, Arkadiusz Bulski notifications@github.com wrote:

@Liryna How about you mention what Lurch is talking about in the docstring? "Latest dokan binary needs to be installed, RC or otherwise."

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

arekbulski commented 8 years ago

It is not an assumption that iterface will change, Liryna confirmed it.

Liryna commented 8 years ago

I think it is better to give the possibility to a end user to use pyfilesystem with 1.0.0 RC or futur stable rather than the current implementation that can absolutely not used since as explained before, there is no way to find the dokan version 0.5.3.

The futur stable will normally be released this month depending of the community feedback that currently seems to be good!

Liryna commented 8 years ago

Interface will not change at all. This pull request will remain as it is even after stable release.

lurch commented 8 years ago

Fair enough, looks like I've been outvoted shrug As I already said, I wasn't expecting the interface or the pyfilesystem-side of Dokan to change at all before or after RC / final versions, merely the Dokan download link. (I guess @apocalypse2012 shows I was wrong in assuming that people would prefer to install the stable version of Dokan, rather than a pre-release version)

I'm happy to go with the majority decision and go ahead and merge #258 anyway, once I've tested it as working. @Liryna have you tested it with different versions of Python, and on different versions of Windows?