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

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

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

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

peteruhnak opened this issue May 14, 2018 · 6 comments

Comments

@peteruhnak
Copy link

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
Copy link

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
Copy link
Member

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

@bitprophet
Copy link
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.

@bitprophet
Copy link
Member

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

@bitprophet
Copy link
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.

bitprophet added a commit that referenced this issue Jul 25, 2018
bitprophet added a commit that referenced this issue Jul 25, 2018
@bitprophet
Copy link
Member

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

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

No branches or pull requests

3 participants