Isolate platform-specific code and add Windows versions #119

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@pfmoore
Contributor
pfmoore commented Feb 27, 2014

No description provided.

@pfmoore pfmoore referenced this pull request Feb 27, 2014
Closed

port to win32 #113

@bitprophet
Member

This looks good overall, random thoughts (feel free to rebuke; do not want to bikeshed this too hard, just raise some stuff so it's on record):

  • Torn on this "the module looks different per platform" approach vs the other PR's "you could, if you really wanted, access the posix functions on windows" approach (though having it be in its own module is still good). I don't like hiding things from users who might have corner case needs, unless it makes things a lot easier for us.
  • Wondering if it'd be smarter to import from plat inside util so users of util don't have to also know about / import plat. Again, torn, neither approach seems clearly better.
@pfmoore
Contributor
pfmoore commented Feb 27, 2014

I'm away for a couple of days, so I'll think on this and get back to you. If nothing else, I've bombarded you with a lot of stuff, so you'll probably be glad of me giving you some peace for a bit!

@bitprophet
Member

Think on it all you want, and just make sure you have dstufft bug me if I miss any comments (I have...a very large unread count on Github right now 😢). Thanks again!

@pfmoore
Contributor
pfmoore commented Mar 3, 2014

After some thought, the idea of having both the posix and windows functions available doesn't seem worth it - the windows version will fail on posix because it uses windows APIs, and the posix function will fail on windows because termios is not available there. The only corner case I can see is cygwin, and that detects as "not windows" - so it'll get the posix version (as at present) but could use the windows one. I'd rather assume that's not needed until someone using cygwin comes up with a use case.

As regards importing plat in util, that's probably worth it as it reduces churn in the calling module (only cli, admittedly). I'll update the patch for that.

@pfmoore
Contributor
pfmoore commented Mar 3, 2014

OK, I've pushed a rework of this patch as noted above, plus a minor fix that I swear I tested originally but works now...

@pfmoore
Contributor
pfmoore commented Mar 3, 2014

@bitprophet the travis build failed here, but it's failing in code I didn't touch. Looks like a Python 2/3 compatibility issue introduced by commit a9c426f

@pfmoore
Contributor
pfmoore commented Mar 20, 2014

Ping...

@bitprophet
Member

Pong! Sorry, been consumed with paramiko work and then personal/life issues. Will keep this open in a tab and try to reply over the weekend or early next week. Thanks!!

@pfmoore
Contributor
pfmoore commented Apr 27, 2014

Any chance you can look at this? Pip is using invoke for some admin tools, and without this patch they don't work on Windows :-(

@izevaka
izevaka commented Jun 17, 2014

+1 would really really like Windows support.

@pfmoore
Contributor
pfmoore commented Jul 8, 2014

Looks like this has now suffered from bit rot, as it no longer merges cleanly. Is there any point in me updating the PR? If it's just going to languish again, I'd rather not bother (at the moment I'm not a user of invoke, as it doesn't work on Windows, and from the progress of this patch, it looks like Windows support isn't really a priority...)

@adrianandreias

+1 for Windows support :)

@pfmoore
Contributor
pfmoore commented Aug 5, 2014

OK, one last try. I've created PR #162 which does nothing but make 3 imports that don't work on Windows optional. With that patch, invoke on Windows won't fail immediately.

@bitprophet, please can you review #162 and assuming there's no major issue, apply it? Then this patch can be closed as no longer worth the effort.

@bitprophet
Member

Arglbargl. Really sorry this keeps getting dropped. (Windows isn't a priority, but that wasn't why - overall project was just not getting love for a while.)

Suspect it won't be hard to update this for latest master, I'll give it a look-see now and make a final call on whether it's worthwhile (and make the changes, if it is.)

I did just merge #162 (thanks for it!) and that will be out on PyPI within the afternoon.

@bitprophet bitprophet added the Bug label Aug 25, 2014
@bitprophet
Member

Quickly re-reviewing this, it seems to solve a handful of things:

  • Imports that aren't valid on Windows. Should be solved by #162 already for the most part I think, depending on how this goes I might shuffle that around.
  • ANSI support. I haven't yet picked a "for reals" color lib (tho colorama is one; see Fabric #101). Thinking it might be cleanest to simply tell currently-colored things to just not do so on Windows, instead of quietly supporting colorama and then possibly changing that later.
    • EDIT: actually, we don't emit any colors (besides bold in one place) at present. What does importing colorama even do here? May leave it out for now and can add back in post-discussion.
  • PTY size detection. Straight up dual implementation; aside from the earlier discussion on where it should live/how it should be loaded, clearly worth using about as-is. I'll probably do some middle ground platform-sensitivity wrapper for now that can be expanded later.
@bitprophet bitprophet added a commit that referenced this pull request Aug 25, 2014
@bitprophet bitprophet Rework the pty_size portion of #119 f3f529b
@bitprophet bitprophet added a commit that referenced this pull request Aug 25, 2014
@bitprophet bitprophet Changelog update re #119 96a79d4
@bitprophet bitprophet added a commit that referenced this pull request Aug 25, 2014
@bitprophet bitprophet Rework the pty_size portion of #119 885041f
@bitprophet bitprophet added a commit that referenced this pull request Aug 25, 2014
@bitprophet bitprophet Changelog update re #119 2b0bfd5
@bitprophet
Member

I've uploaded a copy of this material with my own adjustments/reorganizations to a new branch, https://github.com/pyinvoke/invoke/compare/119-int. If somebody can confirm it works as intended on Windows, I'll merge it for the next bugfix release.

I know I've been super bad about revisiting this/similar tickets, but I'm currently on an "OSS leave" at my dayjob, so I actually have the time to commit. I'll be AFK on a trip from Weds->Tues but otherwise I should notice replies within a day or so. Thanks!

@bitprophet bitprophet added this to the 0.8.3 milestone Aug 25, 2014
@Ivoz
Contributor
Ivoz commented Aug 26, 2014

Windows 8, Anaconda Python 2.7.7, 119-int branch checked out

(tt) C:\Users\Matthew\tt>pip install -e .\invoke
Obtaining file:///C:/Users/Matthew/tt/invoke
  Running setup.py (path:C:/Users/Matthew/tt/invoke\setup.py) egg_info for packa
ge from file:///C:/Users/Matthew/tt/invoke

    warning: no files found matching '*' under directory 'docs'
    warning: no previously-included files matching '*' found under directory 'do
cs\_build'
    warning: no previously-included files matching '*.pyc' found under directory
 'tests'
    warning: no previously-included files matching '*.pyo' found under directory
 'tests'
Installing collected packages: invoke
  Running setup.py develop for invoke

    warning: no files found matching '*' under directory 'docs'
    warning: no previously-included files matching '*' found under directory 'do
cs\_build'
    warning: no previously-included files matching '*.pyc' found under directory
 'tests'
    warning: no previously-included files matching '*.pyo' found under directory
 'tests'
    Creating c:\users\matthew\tt\lib\site-packages\invoke.egg-link (link to .)
    Adding invoke 0.8.2 to easy-install.pth file
    Installing inv-script.py script to C:\Users\Matthew\tt\Scripts
    Installing inv.exe script to C:\Users\Matthew\tt\Scripts
    Installing invoke-script.py script to C:\Users\Matthew\tt\Scripts
    Installing invoke.exe script to C:\Users\Matthew\tt\Scripts

    Installed c:\users\matthew\tt\invoke
Successfully installed invoke
Cleaning up...

(tt) C:\Users\Matthew\tt>cd pip

(tt) C:\Users\Matthew\tt\pip>invoke -h
Usage: invoke-script.py [--core-opts] task1 [--task1-opts] ... taskN [--taskN-op
ts]

Core options:
  --no-dedupe                      Disable task deduplication.
  -c STRING, --collection=STRING   Specify collection name to load.
  -d, --debug                      Enable debug output.
  -e, --echo                       Echo executed commands before running.
  -h [STRING], --help[=STRING]     Show core or per-task help and exit.
  -H STRING, --hide=STRING         Set default value of run()'s 'hide' kwarg.
  -l, --list                       List available tasks.
  -p, --pty                        Use a pty when executing shell commands.
  -r STRING, --root=STRING         Change root directory used for finding task
                                   modules.
  -V, --version                    Show version and exit.
  -w, --warn-only                  Warn, instead of failing, when shell
                                   commands fail.


(tt) C:\Users\Matthew\tt\pip>invoke -l
Available tasks:

  generate.authors
  generate.installer


(tt) C:\Users\Matthew\tt\pip>invoke generate.authors
[generate.authors] Generating AUTHORS
[generate.authors] Collecting author names
@pfmoore
Contributor
pfmoore commented Aug 26, 2014

Confirmed, same results as @Ivoz on Windows 7 64-bit, Python 3.4.

@bitprophet
Member

Looks like it's working then, awesome :) I'll merge to master now, and put a release out today, though as above I may not be able to respond quickly to bug reports until next week.

EDIT: it's in master as of now. Will put out an 0.9.0 release in a little while today.

@bitprophet bitprophet closed this Aug 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment