PyFilesystem / pyfilesystem

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

FTPFS: Use a non decoded path in python3 #254

Closed Konubinix closed 4 years ago

Konubinix commented 8 years ago

In python ftplib uses unicode instead of bytes.

lurch commented 8 years ago

If this is indeed the case, would it make more sense to simply modify the _encode function? Does this behaviour vary depending on which FTP server you're connecting to?

If you can provide a testcase showing where the current code fails, that would be fantastic :-)

Konubinix commented 8 years ago

Hi,

Thank you for your answer.

Andrew Scheller notifications@github.com writes:

If this is indeed the case, would it make more sense to simply modify the _encode function?

I thought about that, but it might confuse the reader to have an _encode function that eventually does not encode the content.

Does this behaviour vary depending on which FTP server you're connecting to?

It initially failed on some synology server but I don't know the software behind.

I tried using pure-ftpd and got the same behavior.

If you can provide a testcase showing where the current code fails, that would be fantastic :-)

Setup a ftp server on localhost. Then run the python3 code: --8<---------------cut here---------------start------------->8--- From fs import ftpfs f = ftpfs.FTPFS("localhost") f.listdir(".") --8<---------------cut here---------------end--------------->8---

You should get the error: --8<---------------cut here---------------start------------->8--- TypeError: cannot use a string pattern on a bytes-like object --8<---------------cut here---------------end--------------->8---

If not, could you please tell which ftp server your are using?

Konubinix GPG Key : 7439106A Fingerprint: 5993 BE7A DA65 E2D9 06CE 5C36 75D2 3CED 7439 106A

lurch commented 8 years ago

Ahhh, I think I see what's going on now - it's not an FTP-specific issue, but just the fact that the FTPFS code is trying to naively append a bytes-like string to a unicode-like string, which you can't do in Python3. So the fix will be a little bit more involved than the PR you've submitted here, as there are other places where the code is trying to do naive string-joining. I'll see if I can knock something up this evening...

lurch commented 8 years ago

...work in progress, but it's proving to be a bit trickier than expected.

Konubinix commented 8 years ago

Ahh I see... Actually, I can see a few problems to solve:

  1. the lack of coercion between bytes and unicode in python3, causing the expression u'X' + b'Y' fail in python3 while it implicitely worked in python2
  2. the ftplib does not expect the same type : in python2, it expected bytes (aka str), in python3, it expects unicodes (aka str also :-))

By taking a deeper look at the code, I undertand that the purpose of _encode is to correctly encode the string given to self.ftp.

Then, I think the most proper solution would be to manipulate unicode everywhere in the code, making sure that all the calls to methods of slef.ftp use _encode and make _encode either encode in python2 or do nothing in python3.

@lurch, do you think that would do it? If so, I think I can propose something.

Konubinix commented 8 years ago

I have adjusted my pull request to make ftpfs work as I said. I tested on a local pure-ftp server and found no problem so far.

I tried, with f an ftpfs object: f.open(u"tést").write(u"éè".encode("utf-8")) print(f.open(u"tést"))

lurch commented 8 years ago

Yup, your updated PR is pretty much exactly what I'd done in my local version :-) But there are additional fixes needed in order to get all the ftpfs unit-tests passing under Python3, which I'll hopefully have time to get back to this evening. (I started fixing sftpfs to be Python3 compatible at the same time too, now that Paramiko supports Python3)

Konubinix commented 8 years ago

Andrew Scheller notifications@github.com writes:

Yup, your updated PR is pretty much exactly what I'd done in my local version :-)

That's good to read :-).

But there are additional fixes needed in order to get all the ftpfs unit-tests passing under Python3,

How can I run those tests?

which I'll hopefully have time to get back to this evening. (I started fixing sftpfs to be Python3 compatible at the same time too, now that Paramiko supports Python3)

I will wait for your fixes then. Thank you for your time and effort.

Konubinix GPG Key : 7439106A Fingerprint: 5993 BE7A DA65 E2D9 06CE 5C36 75D2 3CED 7439 106A

lurch commented 8 years ago

How can I run those tests?

The ftpfs and sftpfs unit-tests are currently disabled for Python3, because when they were written pyftpdlib (python FTP server) and paramiko (python SFTP server) didn't support Python 3.

But the pyfilesystem unit-tests use nose and tox

Konubinix commented 8 years ago

Hi. Have you found any time to work on this matter ?

lurch commented 8 years ago

Have you found any time to work on this matter ?

I did (I made some progress), but I'm afraid I forgot about it! I'm busy for the next two evenings, so I'll try to look at it again on Thursday. (Feel free to remind me again if I haven't updated this issue by next week)

lurch commented 8 years ago

Just adding this link as a reminder that there's other problems with pyftpdlib support in pyfilesystem with python3 https://groups.google.com/forum/#!topic/pyfilesystem-discussion/Niv4lFcVEBA

Konubinix commented 8 years ago

FYI, I have been using this fork for as long as it exists. It did not appear to break so far.