No pywin32 #100

Closed
wants to merge 4 commits into
from

Projects

None yet

2 participants

@jaraco
Contributor
jaraco commented Nov 1, 2012

In order to simplify the code base, I've removed the pywin32-specific code (as ctypes is now required for other operations anyway).

@bitprophet
Member

I'm Windows-challenged, is the implication here that for Python 2.5 and up (which I'm fine limiting future feature releases to, folks still on 2.4 or older can keep using Paramiko 1.7) our use of win32 is 100% replaceable by the stdlib ctypes module?

My experience with Fabric has been that there's like a half dozen Windows related Python modules and their existence or lack thereof varies depending on which Windows, which dev env you're using (cygwin, ActiveState etc etc etc) and so on.

This looks innocuous enough but I want to make sure it's not gonna screw somebody over :)

@jaraco
Contributor
jaraco commented Dec 1, 2012

That's right. The pywin32 dependency was superfluous, which I think is proven by the fact that ctypes has been an absolute requirement (even when pywin32 was used for the API calls). I had some small hesitation myself that removing the pywin32 (which was previously preferred) would cause problems, but I've been using pure Python versions with good success.

I'm very confident that the ActiveState version will work very well with this code. The ActiveState distribution is essentially the stock Python plus pywin32 plus some niceties around environment setup.

I'm somewhat less confident about a cygwin environment, because I'm not at all familiar with Python on cygwin. That said, I believe this code will be more portable, not less. If anything were to work on cygwin, I would expect ctypes to be more likely to work than pywin32. Since ctypes was already used to construct the appropriate structures, I still believe there's little risk to this change.

If it turns out there's a need to support pywin32 for any reason, I'll commit to restore that functionality (which will make the subsequent requests based on this one more challenging, but I'm up to it).

@bitprophet
Member

Works for me.

Can you do me a favor and update NEWS with a changelog entry for this? Following the existing format -- ticket number, credit yourself, etc. There should be a "1.10.0" section at the top by now, which is where this should live.

Do that and I will be able to hit the magical merge button :D thanks!

@bitprophet
Member

Rebased + merged \o/

@bitprophet bitprophet closed this Mar 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment