put adds unwanted backslash on recursive dir #604

Closed
jaraco opened this Issue Apr 3, 2012 · 10 comments

Projects

None yet

5 participants

@jaraco
Contributor
jaraco commented Apr 3, 2012

I've created a sample dir tree to put:

mkdir -p home/.ssh
touch home/.ssh/dat

Then create a simple fabric script to put that tree on my remote home dir:

>cat fabfile.py
from fabric.api import run, put

def update_profile():
    put('home/.*', './')
    # Fabric 1.5a ends up creating a folder named "\.ssh"
    run('ls -d *.ssh')

Invoking it produces the following:

> fab -H vdeploy update_profile
[vdeploy] Executing task 'update_profile'
[vdeploy] put: home\.ssh\dat -> ./\.ssh/dat
[vdeploy] run: ls -d *.ssh
[vdeploy] out: \.ssh


Done.
Disconnecting from vdeploy... done.

As you can see, it creates a folder not with the same name as the one on the local file system, but instead prefixes it with a backslash.

My guess the backslash is created because this script is run on Windows and Fabric doesn't recognize the backslash when traversing the path.

If someone familiar with the fabric code base would point me to where this might be occurring, I can probably put together a patch/pull request.

As an aside, it seems to take eight backslashes to remove that one backslash:

run('rm -R \\\\\\\\.ssh')

That is, there's one backslash in the file system, doubled to escape for bash, doubled again to escape for Python. What does the third doubling escape for? If I run it from the bash prompt, I only have to provide two backslashes (i.e. "rm -R \.ssh").

@gumho
gumho commented May 24, 2012

Having this issue too. I'm 100% it's only an issue on Windows platform.

@dieresys

I had a similar problem and created the following patch in fabric.sftp.put_dir:

index abaa8ef..cc37532 100644
--- a/fabric/sftp.py
+++ b/fabric/sftp.py
@@ -271,6 +271,7 @@ class SFTP(object):

         for context, dirs, files in os.walk(local_path):
             rcontext = context.replace(strip, '', 1)
+            # normalize pathname separators with POSIX separator
+            rcontext = rcontext.replace(os.sep, '/')
             rcontext = rcontext.lstrip('/')
             rcontext = posixpath.join(remote_path, rcontext)

The main problem is put_dir doesn't know how to handle backslashes:

  • It doesn't strip leading backslashes
  • posixpath.join treat backslashes as a valid character for a posix name (which is true).
  • Therefore self.exists(rcontext) gets confused and returns false.

I'm sure there is a better way to handle this, but it worked for me.

@bitprophet
Member

@dieresys Thanks for diagnosing this -- we don't usually test on Windows (Win support is purely on a user-contributed basis, we don't have the interest or cycles to do otherwise.)

This (os.walk resulting in backslash-delimited file paths on Windows and this not translating well to always-POSIX-style remote paths) seems pretty legit to me. I acknowledge that the patch might have some issues in corner cases, but IIRC we've always moved towards presuming the remote end uses POSIX (forward slashes) so I don't expect this to cause serious problems.

If you could make a pull request for this that includes an update to docs/changelog.rst (following the style of existing entries, including credit for @jaraco for catch and yourself for patch) I'll merge it. Thanks agan!

(p.s. if possible, I recommend using hub pull-request to turn this issue into the PR itself. Might not be doable since you didn't author it, in which case, no biggie.)

@oliverjanik
Contributor

Bump. I hope this will get fixed. It's amazing nobody noticed this problem for so long.

@bitprophet
Member

@oliverjanik Not really that amazing, Windows users are a reasonably small minority for Fabric.

That aside -- I don't see an active PR for this, so feel free to make one yourself with the above diff + my requested docs changes! :)

@oliverjanik oliverjanik referenced this issue Oct 30, 2012
Merged

Fixed #604 #768

@oliverjanik
Contributor

Can you merge this? Thanks.

@jaraco
Contributor
jaraco commented Dec 4, 2012

I reviewed the patch. It looks good to me. Thanks for the patch.

@oliverjanik
Contributor

There is a problem with the patch. I have made a mistake. I will fix it shortly.

@oliverjanik
Contributor

Fixed and tested.

@bitprophet bitprophet pushed a commit that closed this issue Dec 16, 2012
@oliverjanik oliverjanik Fixed #604 8877c9f
@bitprophet
Member

LGTM, sorry for the delay in merging!

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