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

Fix failure in listdir when server uses a locale (1.17) #986

Closed
wants to merge 5 commits into from

Conversation

bz2
Copy link
Contributor

@bz2 bz2 commented Jun 6, 2017

Fixes #985

SFTPAttributes uses the locale-aware %b directive for the abbreviated month name with time.strftime, but was not decoding the result on Python 2.7.

Add a strftime alias in py3compat that will always return unicode, and resolve the mixing of bytes and text in SFTPAttributes methods.

Add a test at the listdir level, and a more specific test for the SFTPAttributes asbytes method.

@bz2 bz2 force-pushed the 1.17_SFTPAttributes_locale branch from 6cc5e7e to e504f7c Compare June 6, 2017 22:25
@bz2
Copy link
Contributor Author

bz2 commented Jun 6, 2017

Tried enabling the new tests for ci based on the Travis docs suggestion:

- sudo apt-get update && sudo apt-get --reinstall install -qq language-pack-en language-pack-de

But that failed with:

This job is running on container-based infrastructure, which does not allow use of 'sudo', setuid, and setgid executables.

Not sure if they have a way of enabling locale support in containers. It's not really essential.

@bitprophet
Copy link
Member

Thanks for this, will try to review it in more depth later.

Re: travis: we could potentially tell Travis to run us on the non-containerized, sudo-capable images; my other projects have to do this anyways because their integration suites need to test interactions with sudo. Means a bit of a speed hit but Paramiko's suite is one of the slower ones I manage now anyway (sob) so it's not like it really matters a lot.

@bz2
Copy link
Contributor Author

bz2 commented Jun 8, 2017

Got an alternative spelling for the langpack install from Travis support which works with containers, see latest passing run.

Will just note as well that this change shares the first two revs with #971 - if that gets merged there's a teeny bit less to review here.

bz2 added 5 commits June 11, 2017 16:35
Fixes paramiko#985

SFTPAttributes uses the locale-aware %b directive for the
abbreviated month name with time.strftime, but was not
decoding the result on Python 2.7.

Add a strftime alias in py3compat that will always return
unicode, and resolve the mixing of bytes and text in
SFTPAttributes methods.

Add a test at the listdir level, and a more specific test
for the SFTPAttributes asbytes method.
@bz2 bz2 force-pushed the 1.17_SFTPAttributes_locale branch from d17ac87 to e61b9fe Compare June 11, 2017 17:01
@bz2
Copy link
Contributor Author

bz2 commented Jun 11, 2017

Have rebased on top of latest 1.17 changes and added a changelog entry.

@bitprophet bitprophet added this to the Next bugfix milestone Jun 12, 2017
@bitprophet
Copy link
Member

At this point I'm not sure I want to truck with non-critical changes on the 1.x line; would you be opposed to rebasing this onto 2.0 instead? Thanks!

@bz2
Copy link
Contributor Author

bz2 commented Jun 12, 2017

I'm happy to propose this against 2.0 as well, but is there any reason not to just land it on the 1.X series?

I can probably get a debian patch applied to fix to make our test suite work, but that's easier if the same patch is applied upstream.

bz2 added a commit to bz2/paramiko that referenced this pull request Jun 12, 2017
@ploxiln
Copy link
Contributor

ploxiln commented Jun 12, 2017

I'd humbly argue for this being appropriate for 1.18 - the non-test-related changes aren't too big.

@bitprophet
Copy link
Member

The main reason is just that it's a much larger pain in the ass the cross the 1.x->2.x barrier because I was an idiot and put out a 1.x feature release (1.18) after 2.0 landed. This means one cannot simply "merge up" from 1.x to master anymore, or one will pollute 2.0.x with the feature changes from 1.18.x. Instead, one has to do two separate PRs or otherwise cherry-pick; and it's easy to mess up doing this, too.

If the contributors involved have the time to cross all the T's and dot all the I's such that everything including changelog entries is set up, with 2x PRs for 1.17 and 2.0, and I can simply merge-then-merge-up...that's mostly acceptable. But eventually 1.x is going to have to die anyways (see: all the reasons 2.0 and Cryptography happened) so it's only a matter of when, in my mind.

@bitprophet
Copy link
Member

bitprophet commented Jun 13, 2017

In this particular case, all the T's and I's have been crossed and dotted appropriately because #992 exists ( ❤️ @bz2) so...sure! Why not. But that's still my rationale for why, by default, I'd really prefer to limit how much stuff "must" go back to 1.x, and I'd rather users find ways to get on 2.0 when possible.

@ploxiln
Copy link
Contributor

ploxiln commented Jun 13, 2017

Agreed 😁

@bitprophet
Copy link
Member

Much later: I've moved on and the 1.x series is unsupported at this point (arguably a good thing given PyCrypto remains completely dead!) so I'm going to look at merging #992 but close out this one.

@bitprophet bitprophet closed this Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants