PyWin32 dependency could be optional for Fabric on Windows platform. #669

Merged
merged 2 commits into from Jul 5, 2012

Projects

None yet

2 participants

@diyan
Contributor
diyan commented Jun 14, 2012

Hello devs,

I would like make Fabric less dependent on PyWin32 library on Windows platform.

There are only few places exists and looks like it it possible add additional checks for such cases.

It would be great if you have a time to look into this first small change.

I'm opened to any suggestions if they could help in pushing this changes into main upstream project.

Also I'm going to ask on one more pull requests in future.

diyan added some commits Jun 14, 2012
@diyan diyan Rewrite _get_system_username into more cross-platform way while keepi…
…ng fallback into PyWin32 library under Windows. Add .idea/ info gitignore file.
49afc32
@diyan diyan Update _rc_path method. Fallback into PyWin32 library only if os.path…
….expanduser method returns unchanged path.
0a94630
@bitprophet
Member

Our use of Windows specific code has changed a few times over the years, so at this point I have no idea what is actually going to work on all Windows platforms. E.g. some assumptions made that work on "normal" Windows break for cygwin users, or breaks for Enthought users, or etc.

If you can take the time to double check the history of the files being modified / related issues here on github, to make sure your change will not break things for other users, that would be greatly appreciated. I can't test out Windows changes myself so I can only accept patches that are 99% sure not to break things :) Thanks!!

@diyan
Contributor
diyan commented Jul 3, 2012

Hello Jeff,

As you asked, I have checked commit history back into 4 years of Fabric development in order to track how Windows-related code were changed.

You can see my changes in two functions - _rc_path and _get_system_username in state.py.

My change in _rc_path function in state.py file was based on implementation below...
https://github.com/fabric/fabric/blob/f7f402ccf68f9fb9e16fab135968363e51d85c4a/fabric.py#L1178

...which was a little bit changed in commit below. This is was the last commit where implementation contains a change related to Windows platform, after that this portion of code was just moved around modules and functions during code refactoring.
https://github.com/fabric/fabric/blob/447571728eba441d150af12077c1fc40db75f5dc/fabric/subroutines.py#L52

Commit below related to Python/Cygwin/Windows behavior...
diyan@b254048#L0R21

...and my change should not affect Python/Cygwin distribution because it always returns fabric.state.win32 == false:
http://docs.python.org/library/sys.html#sys.platform

Change in _get_system_username were maded on top of this implementation:
https://github.com/fabric/fabric/blob/447571728eba441d150af12077c1fc40db75f5dc/fabric.py#L49

I changed call pwd.getpwuid into getpass.getuser function which is supported on both Unix and Windows systems:
http://docs.python.org/library/getpass.html#getpass.getpass

Also I tried keep original implementation where possible.

You may see that for win32 cases there are exists fallback into original implementation:
https://github.com/diyan/fabric/blob/0a94630aaf4af6d6609aa858d6ffebf500c13d3a/fabric/state.py#L47
https://github.com/diyan/fabric/blob/0a94630aaf4af6d6609aa858d6ffebf500c13d3a/fabric/state.py#L61

Thank you for your feedback.

In case if you need anything else from me, please let me know.

@bitprophet
Member

Hi @diyan -- thanks so much for taking the time to look into that. Between your notes & the fact that your changes do look like they will fall back to the current implementation on failure, I think this is sufficient.

I'm not sure it's worth backporting to 1.3/1.4 (partly because those should be stable and this isn't a critical fix) but I will merge into master (1.5) in a moment.

@bitprophet bitprophet merged commit 0a94630 into fabric:master Jul 5, 2012
@bitprophet bitprophet added a commit that referenced this pull request Jul 5, 2012
@bitprophet bitprophet Update changelog, AUTHORS re #669 62abc4e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment