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

Windows behavior fixed, with appveyor tests passing #33

Closed
wants to merge 21 commits into from

Conversation

jeremycarroll
Copy link
Contributor

@jeremycarroll jeremycarroll commented Aug 24, 2018

This PR fixes issues on the Windows platform, particularly python2, revealed in the appveyor test failures.
In particular, encoding when making external calls with subprocess was usually incorrect.

Two specific changes on py2:

  • use strings not unicode to add to the environment before making subprocess calls
  • encode any non-ascii command line arguments (particularly ) in utf-8 before making the call.

A third change for both py2 and py3 is to change the file operations to text mode rather than binary mode, which simplifies handling of encoding.

In the tests, two further issues were addressed:

  • incorrect handling when hg (or presumably git) is absent (this issue was not visible on appveyor, but locally to me)
  • test_usage_string_fork changed to not explicit encode, and to use byte strings instead; xfailing in the most difficult case to fix: Windows on py3.

NB: I don't have a windows machine, so I am entirely reliant on appveyor for above analysis

@jeremycarroll jeremycarroll changed the title fixing appveyor WIP appveyor tests fixed Aug 24, 2018
@jeremycarroll jeremycarroll mentioned this pull request Aug 24, 2018
@jeremycarroll jeremycarroll changed the title appveyor tests fixed Windows behavior fixed, with appveyor tests passing Aug 24, 2018
def test_usage_string_fork(tmpdir, capsys):
tmpdir.chdir()

try:
out = check_output('bumpversion --help', shell=True, stderr=subprocess.STDOUT).decode('utf-8')
out = check_output('bumpversion --help', shell=True, stderr=subprocess.STDOUT)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a universal_lines parameter which also fixes encoding issues. Perhaps that'd remove the need for xfail?

Copy link
Owner

Choose a reason for hiding this comment

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

I believe it's universal_newlines FWIW

@c4urself
Copy link
Owner

c4urself commented Aug 24, 2018

@jeremycarroll great work, thank you! let's try to see if we can remove the need for xfail -- if you haven't got the time then I'm OK with a TODO that we can work on later.

Also: if you could clean up the commits a bit that'd be pretty sweet as well.

Will merge on your indication.

@jeremycarroll
Copy link
Contributor Author

OK - I will rebase onto a new branch, with cleaner commits, and try the newlines thing ... I don't think that was it: the error was occurring because the -h option prints out some text including that arrow . Then this was being past from the subprocess back into the python process, where the confusion was happening.

@jeremycarroll
Copy link
Contributor Author

This PR is replaced by a new one

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

3 participants