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

port to win32 #113

Closed
wants to merge 1 commit into from
Closed

port to win32 #113

wants to merge 1 commit into from

Conversation

mindw
Copy link

@mindw mindw commented Jan 26, 2014

Really fast and brutal fix to some win32 issues

@mindw
Copy link
Author

mindw commented Feb 6, 2014

ping?

@bitzl
Copy link

bitzl commented Feb 16, 2014

Windows support would be great!

@pfmoore
Copy link
Contributor

pfmoore commented Feb 26, 2014

I would also like to see Windows support. Is there any particular issue with this patch - it looks OK to me from a quick glance?

I would be happy to contribute time to testing and working on Windows support if the project needs people with Windows expertise.

@Ivoz
Copy link
Contributor

Ivoz commented Feb 27, 2014

https://github.com/kennethreitz/envoy might be helpful for subprocess running

@pfmoore
Copy link
Contributor

pfmoore commented Feb 27, 2014

On Windows, a number of the tests fail. But this is likely to be because the tests are not written to be portable, and this PR does not update the tests. I can look at creating a PR to make the tests portable to Windows, but it's going to be a bit of work. One thing - can someone tell me how to run a subset of the tests (one test or one test file) under spec? The suggestions I've found for nose don't seem to work for me with spec - I am tending to get "0 tests run", for example with spec cli or spec cli.py or spec tests/cli.py or spec tests/cli (you get the idea...)


try:
from .vendor import pexpect
except:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What actual exception (or exceptions, if there's >1 likely) is pexpect throwing under Windows? Please try to catch that instead :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be an import error - from the import of pty within pexpect - because pty doesn't exist on Windows. It might be better just to do a straight platform check round the pty stuff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we're modifying pexpect at all, just vendoring it, so I'd prefer to avoid touching it if possible. If these changes work well enough as-is and we can just change this line to except ImportError:, that's fine too. Just don't want a bare except.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what I meant was in runner.run, just do

if pty and sys.platform == 'win32': raise RuntimeError("ptys not supported")

Then there's no need to import pexpect at all, and we can skip it altogether on Windows.

I'm currently putting together an alternative patch that puts the platform-specific stuff in a single file. It might make for a cleaner structure for future work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, and yea given the constraints of ptys on Windows, that makes sense. And big +1 on corralling platform specific stuff; again my philosophy for Windows support on my projects is basically "POSIX is 1st class citizen; Windows is 2nd class and lives off to the side". I don't like having to do that but it's the nature of the platforms & their distributions amongst the userbase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. The only thought I have is that sometimes the "POSIX first class" philosophy can lead to non-portable code which could actually be trivially made portable but wasn't simply because nobody thought about portability. But it's up to us Windows users to keep you honest on that one :-)

@pfmoore
Copy link
Contributor

pfmoore commented Feb 27, 2014

By the way, pip has switched to using invoke for some of its release process scripts, and I can confirm that with this patch, invoke works on Windows for that usage.

@@ -2,3 +2,5 @@ docs/_build
.coverage
*.egg-info
.tox

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any particular reason for the blank line?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None. It's added automatically by GitExt.

@bitprophet
Copy link
Member

Thanks for the contributions! Left a bunch of line comments, will wait for responses/changes.

@pfmoore Use spec --tests=tests/whatever.py, spec is still technically a nose plugin and that's a nose flag; it's what inv test uses: https://github.com/pyinvoke/invocations/blob/master/invocations/testing.py#L14

@bitprophet
Copy link
Member

@pfmoore Oh, also, I would support updating the tests to be portable, but hopefully in a way that doesn't make them significantly more verbose/boilerplatey. I want to support Windows but I also don't want to sacrifice significant code clarity for it :(

If it's easier to just have a quick high level integration test file that can be run by Windows-using devs (esp given Travis-CI doesn't do Windows, there's no good way to have strong regression testing for the platform) then that might be a better approach.

@pfmoore
Copy link
Contributor

pfmoore commented Feb 27, 2014

@bitprophet Thanks for the test invocation hint - not sure how I didn't try that variation!

Test portability would probably need to be a mix of skipping the ones that will never work on Windows (pty stuff), a bit of fiddling round things like line ending assumptions. Probably the biggest one would be the OS commands being run in the tests - they use bash syntax in a few places, and assume a standard Unix command set is available. That can be fixed, but it could be a bit intrusive - I'll see how it goes.

A short term approach might be to just have a high-level integration test for now, with phased changes to the main tests to improve coverage as we go along.

One other thing I spotted in the tests is the ANSI colour support. Windows doesn't use ANSI codes, so that'll show up as garbage. How unhappy would you be with changes to the way that's done? It could be a dependency on colorama (which does colour in a cross-platform way) or vendoring it, or some wrappers around an optional dependency, falling back to ANSI on Unix and black-and-white on Windows?

Maybe the best approach would be a "compatibility" module that encapsulated the stuff that differs between platforms: That would keep the impact on code clarity minimal.

@bitprophet
Copy link
Member

@pfmoore:

  • Portability: The phased test support sounds reasonable, I'd like to prioritize getting the tool functional for Windows users, and wouldn't want "make the tests perfect on Windows" to hold that up.
  • Colors: I need to port more of these Invoke-labeled Fab tickets over, but see Fabric #101 for my previous thoughts on color - colorama is on that list IIRC so if it does this well that might be a +1 for it. EDIT: Looks like 'blessings' was the top-of-mind (lol business terms) last time I looked at that ticket; we should see whether it handles x-platform. But let's keep that discussion over there.
  • Compatibility module: as per my sub-comment higher up, my default is to treat POSIX as the primary implementation and have code branch off into Windows specific overrides as needed. I am not 100% wedded to that but IMO it makes things easier to reason about for what I see as the primary case, especially in situations where Windows-specific code is only in a few spots.

@bitprophet
Copy link
Member

Merging this discussion into #119 so it's not in two places. Thanks!

@bitprophet bitprophet closed this Aug 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants