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

Don't use os.path.join for remote paths #306

Closed
bitprophet opened this Issue Aug 19, 2011 · 11 comments

Comments

Projects
None yet
2 participants
@bitprophet
Member

bitprophet commented Aug 19, 2011

Description

In most places (SFTP-ly speaking) we assume the remote end uses Unix style forward slashes because there is no reliable way to detect the remote OS, and (AFAIK) even Windows-based remote SSH/SFTP clients use forward slashes anyway.

Thus, our os.path.join policy should be to use os.path.join for local paths but not for remote paths, which should always use /.

Unfortunately, we do not do this consistently, leading Windows client users to run into problems when using put and get. fabric/sftp.py is littered with os.path.join.

So, we should:

  • Develop (or copy, ideally) our own version of os.path.join hardcoded to use a Unix-style separator
  • Use that instead of os.path.join for all remote contexts

Originally submitted by Jeff Forcier (bitprophet) on 2011-03-07 at 06:30pm EST

Attachments

Relations

  • Duplicated by #356: put command incorrectly assembles remote_path on Windows
  • Duplicated by #405: Put is using the local machines Path functions instead of the remote machine's.

@ghost ghost assigned bitprophet Aug 19, 2011

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Eric Hanchrow (offby1) posted:


done as far as can reasonably be tested, here.


on 2011-03-14 at 12:44am EDT

Member

bitprophet commented Aug 19, 2011

Eric Hanchrow (offby1) posted:


done as far as can reasonably be tested, here.


on 2011-03-14 at 12:44am EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

**** (adamjernst) posted:


Well it seems this is done, but you could also import the "posixpath" module which should be available on all platforms, and uses / as the separator. This has the advantage of hewing to a well-defined spec (POSIX).


on 2011-03-14 at 10:14pm EDT

Member

bitprophet commented Aug 19, 2011

**** (adamjernst) posted:


Well it seems this is done, but you could also import the "posixpath" module which should be available on all platforms, and uses / as the separator. This has the advantage of hewing to a well-defined spec (POSIX).


on 2011-03-14 at 10:14pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jeff Forcier (bitprophet) posted:


Chris -- can you point me to the specific commits or branch for your changes? Having a hard time parsing the general commit list for that GH link.

Adam -- is posixpath available by default on Windows? The docs are unclear and I've no Windows box to test on right now.


on 2011-04-22 at 05:11pm EDT

Member

bitprophet commented Aug 19, 2011

Jeff Forcier (bitprophet) posted:


Chris -- can you point me to the specific commits or branch for your changes? Having a hard time parsing the general commit list for that GH link.

Adam -- is posixpath available by default on Windows? The docs are unclear and I've no Windows box to test on right now.


on 2011-04-22 at 05:11pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jason R. Coombs (jaraco) posted:


confirmed: posixpath is available on all platforms by default.


on 2011-05-25 at 02:20pm EDT

Member

bitprophet commented Aug 19, 2011

Jason R. Coombs (jaraco) posted:


confirmed: posixpath is available on all platforms by default.


on 2011-05-25 at 02:20pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jason R. Coombs (jaraco) posted:


I've added another commit (jaraco@0a592f1) to my fork which incorporates the additional locations where remote paths were using os.path.join.


on 2011-05-25 at 02:30pm EDT

Member

bitprophet commented Aug 19, 2011

Jason R. Coombs (jaraco) posted:


I've added another commit (jaraco@0a592f1) to my fork which incorporates the additional locations where remote paths were using os.path.join.


on 2011-05-25 at 02:30pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Christian Long (christianmlong) posted:


I applied Jason's posixpath fixes to Fabric 1.0.1. See attached patch file. Works on Windows XP, Python 2.5.


on 2011-06-01 at 12:08pm EDT

Member

bitprophet commented Aug 19, 2011

Christian Long (christianmlong) posted:


I applied Jason's posixpath fixes to Fabric 1.0.1. See attached patch file. Works on Windows XP, Python 2.5.


on 2011-06-01 at 12:08pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jeff Forcier (bitprophet) posted:


Christian Long wrote:

I applied Jason's posixpath fixes to Fabric 1.0.1. See attached patch file. Works on Windows XP, Python 2.5.

Awesome, thanks. This was actually next on my list for tackling (trying to get 1.0.2 and 1.1 out soon) so you have good timing.


on 2011-06-01 at 05:09pm EDT

Member

bitprophet commented Aug 19, 2011

Jeff Forcier (bitprophet) posted:


Christian Long wrote:

I applied Jason's posixpath fixes to Fabric 1.0.1. See attached patch file. Works on Windows XP, Python 2.5.

Awesome, thanks. This was actually next on my list for tackling (trying to get 1.0.2 and 1.1 out soon) so you have good timing.


on 2011-06-01 at 05:09pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Yannick Jost (yanjost) posted:


Still not in 1.2 ?


on 2011-07-22 at 07:39am EDT

Member

bitprophet commented Aug 19, 2011

Yannick Jost (yanjost) posted:


Still not in 1.2 ?


on 2011-07-22 at 07:39am EDT

@luciferlu

This comment has been minimized.

Show comment
Hide comment
@luciferlu

luciferlu Aug 25, 2011

I have same issue, but seems it is not addressed in 1.2

luciferlu commented Aug 25, 2011

I have same issue, but seems it is not addressed in 1.2

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 30, 2011

Member

Protip: ticket status is a great indicator of whether something has been released or not ;) Also, changelogs.

This ticket is open, and is not listed in the 1.2 changelog, so no, it has not been fixed yet. The best way to keep track is to just watch this ticket -- I will comment (or you will see commit message notes & its status going to 'Closed') when it's fixed :)

Member

bitprophet commented Aug 30, 2011

Protip: ticket status is a great indicator of whether something has been released or not ;) Also, changelogs.

This ticket is open, and is not listed in the 1.2 changelog, so no, it has not been fixed yet. The best way to keep track is to just watch this ticket -- I will comment (or you will see commit message notes & its status going to 'Closed') when it's fixed :)

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Feb 27, 2012

Member

Fixed via #570.

Member

bitprophet commented Feb 27, 2012

Fixed via #570.

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