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

c.put confusion about string vs file-like object in local #1749

Closed
peteruhnak opened this Issue May 14, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@peteruhnak

peteruhnak commented May 14, 2018

According to transfer.put() docs, I should be able to either provide local path to a file (as a string), or a file-like object.

When using the former (string), I can specify in the remote path only the target directory. This is used in some examples:

c.put('myfiles.tgz', '/opt/mydata')

But when I try to do the same ( c.put('file.txt', '/home/me/dir'), I always end up with IOError: Failure, because it is actually behaving as if the local path was a file-like object and not a string.

It fails specifically on paramiko.transport.sftp.sftp._log: [chan 0] open('/home/me/dir', 'wb')

The solution is to always provide the full path, e.g.

c.put('file.txt', '/home/me/dir/file.txt')

But that is inconsistent with the docs and examples.

Finally, having to provide the full path to $HOME is less than ideal, but there seems to be issue for that already #1653

  • Tested in Windows 10
  • Fabric 2.0.0
  • Paramiko 2.4.1
  • Invoke 1.0.0
@hoffa

This comment has been minimized.

Show comment
Hide comment
@hoffa

hoffa May 29, 2018

Any updates on this? Connection.put with remote as directory path string currently raises IOError.

(Also thanks for all the work on Fabric et al!)

Fabric 2.1.3
Paramiko 2.4.1
Invoke 1.0.0

hoffa commented May 29, 2018

Any updates on this? Connection.put with remote as directory path string currently raises IOError.

(Also thanks for all the work on Fabric et al!)

Fabric 2.1.3
Paramiko 2.4.1
Invoke 1.0.0

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jun 22, 2018

Member

Sounds like a legit bug, will take a look later. Patches (and tests! since ours all pass :( implying we're missing some...) welcome.

Member

bitprophet commented Jun 22, 2018

Sounds like a legit bug, will take a look later. Patches (and tests! since ours all pass :( implying we're missing some...) welcome.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 24, 2018

Member

Looking at this now, I'm not actually sure the issue is assumption of FLO, I think it's instead just a straight up lack of implementing correct remote path munging - in other words, we're asking the SFTP server to open the directory as a file to write, instead of tacking on the local file's basename to the remote directory path.

Partial debug log from Fabric's end:

invoke.transfer.put: Massaged relative local path 'setup.py' into '/Users/jforcier/Code/oss/fabric/setup.py'
invoke.transfer.put: Uploading '/Users/jforcier/Code/oss/fabric/setup.py' to '/Users/jforcier/tmp/'
paramiko.transport.sftp.sftp._log: [chan 2] open(b'/Users/jforcier/tmp/', 'wb')

Then part of the traceback shows we're throwing that directory path into CMD_OPEN, in sftp_client.py:

t, msg = self._request(CMD_OPEN, filename, imode, attrblock)

Which eventually hits that same file/class' error handling around what the SFTP server said was wrong with the IO operation (which is why it's just "Failure" because of course it is...AFAIK that's OpenSSH's fault.)


Anyway, time to write some tests to prove this and then figure out if it's a logic oops (I thought we were constructing an appended path at some point) or an actual straight up missing piece of functionality.

Member

bitprophet commented Jul 24, 2018

Looking at this now, I'm not actually sure the issue is assumption of FLO, I think it's instead just a straight up lack of implementing correct remote path munging - in other words, we're asking the SFTP server to open the directory as a file to write, instead of tacking on the local file's basename to the remote directory path.

Partial debug log from Fabric's end:

invoke.transfer.put: Massaged relative local path 'setup.py' into '/Users/jforcier/Code/oss/fabric/setup.py'
invoke.transfer.put: Uploading '/Users/jforcier/Code/oss/fabric/setup.py' to '/Users/jforcier/tmp/'
paramiko.transport.sftp.sftp._log: [chan 2] open(b'/Users/jforcier/tmp/', 'wb')

Then part of the traceback shows we're throwing that directory path into CMD_OPEN, in sftp_client.py:

t, msg = self._request(CMD_OPEN, filename, imode, attrblock)

Which eventually hits that same file/class' error handling around what the SFTP server said was wrong with the IO operation (which is why it's just "Failure" because of course it is...AFAIK that's OpenSSH's fault.)


Anyway, time to write some tests to prove this and then figure out if it's a logic oops (I thought we were constructing an appended path at some point) or an actual straight up missing piece of functionality.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 24, 2018

Member

OK, nope, all the munging is around empty or relative remote paths, there's no is-directory check.

Member

bitprophet commented Jul 24, 2018

OK, nope, all the munging is around empty or relative remote paths, there's no is-directory check.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 24, 2018

Member

While I'm in here, wondering how to handle FLOs too, in the case where remote path is a directory.

In v1, we looked at FLOs' .name attribute because that's part of the Python FLO API...but apparently only for debug log purposes (in #699). Whoops. I looked over our and Paramiko's code and don't see anything actually using that as a "filename" clue.

Anyway, so here in v2 we have the opportunity to address that for reals, or to just throw an explicit exception saying "whoa hey you need to give a non-directory remote file path for these". Probably both, since not all FLOs necessarily have the name attribute anyways.

Member

bitprophet commented Jul 24, 2018

While I'm in here, wondering how to handle FLOs too, in the case where remote path is a directory.

In v1, we looked at FLOs' .name attribute because that's part of the Python FLO API...but apparently only for debug log purposes (in #699). Whoops. I looked over our and Paramiko's code and don't see anything actually using that as a "filename" clue.

Anyway, so here in v2 we have the opportunity to address that for reals, or to just throw an explicit exception saying "whoa hey you need to give a non-directory remote file path for these". Probably both, since not all FLOs necessarily have the name attribute anyways.

bitprophet added a commit that referenced this issue Jul 25, 2018

bitprophet added a commit that referenced this issue Jul 25, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 25, 2018

Member

OK, that's all fixed up, will be out in next bugfix release 🎉

Member

bitprophet commented Jul 25, 2018

OK, that's all fixed up, will be out in next bugfix release 🎉

@bitprophet bitprophet closed this Jul 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment