-
Notifications
You must be signed in to change notification settings - Fork 148
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 support #64
Windows support #64
Conversation
Bumpversion fails on windows because Unicode values are entered into the environment (due to `from __future__ import unicode_literals`). I can not reproduce the problem on linux. This is the error I'm seeing on windows before this change: ____ ERROR collecting tests.py _______ tests.py:36: in <module> call(["git", "--help"], shell=True) != 1, c:\python27\Lib\subprocess.py:493: in call return Popen(*popenargs, **kwargs).wait() c:\python27\Lib\subprocess.py:679: in __init__ errread, errwrite) c:\python27\Lib\subprocess.py:896: in _execute_child startupinfo) E TypeError: environment can only contain strings
Python 3x doesn't like explicit "unicode", relying on implicit unicode from `from __future__ import unicode_literals` instead.
👍 |
Even if that would be a valid excuse: A regression test would be great to make sure this bug isn't introduced again in a later version. |
This could be easily tested by running in a subprocess for which you set |
I'm assuming that's the cause (non utf8 default encoding on Windows). |
No, the bug here is that the code is attempting to store a Unicode key/value pair in the environment without encoding it. In this particular case it is possible to store the key/value as ascii (aka. a byte string). You can, of course, also use the platform calls to fetch/store Unicode in the environment, but that seems like a lot of work for no gain. That it works on linux is probably due to an automatic reinterpretation somewhere. (ps: I believe Windows uses ibm850 as the environment encoding, but that may be dependent on which country you're in..) |
To re-iterate: I cannot reproduce this problem on linux -- ie. there is no way to write a regression test that will catch this on linux that I'm aware of. Not a single test in the testsuite passes before this fix on Windows (is that a regression test when the CI system doesn't run the tests on Windows..?) I'm not sure I understand what you would like me to do here..? |
Sounds interesting, but I'm afraid I don't get how this will work..? Care to elaborate? ps: this is the line that is failing:
which is before the first test. |
On Fri, Jan 9, 2015 at 9:34 PM, Bjorn notifications@github.com wrote:
See: Try and see what works. Worth starting to use appveyor (windows CI), see Thanks, |
IIUC, then PYTHONIOENCODING only deals with stdin/sdout/stderr, while this problem is related to the environment. Even if that would let me specify a default encoding for the environment(*) what would the test need to do to be a regression test? Just as a reality check here... is it really supposed to be this difficult to get a two-character fix for an obvious bug into bumpversion? (*) you can see from https://github.com/python-git/python/blob/715a6e5035bb21ac49382772076ec4c630d6e960/Modules/posixmodule.c#L6652 that there is no encoding call in Python's putenv. |
|
|
It's the
I'd assume the maintainer doesn't have time to look closely and notice that this harmless change doesn't introduce any regressions (instead of just being plain rude and not explain that there is something very wrong with this change). Anyway, so with few test fixes this almost pass on windows. The rest of the failures look minor and unrelated (line ending issues). |
Sorry, got a little frustrated. Didn't mean to be so negative ;-) My real goal is to get Subversion support in (https://github.com/datakortet/bumpversion/tree/subversion-support), but before that I need to get changes to the vcs abstractions in the test suite, and since that is going to be a bit of work I'd prefer to work on the platform I'm sitting on daily,... hence this fix first. I am starting to wonder though, whether version bumping and vcs operations don't really belong in two separate tools...? (but that's neither here nor there). |
Folks, first, thank you for the time and interest in this little project of mine. I'm sorry for any frustrations that I may have caused :(
Yes, I'm aware of that. I can't reproduce this on windows and at the moment there also is no continuous integration setup using a windows machine. I know the tests for 0.4 passed on windows (debugged that with a friend of mine on his windows machine), but apart from that I'm clueless for the state of windows support.
Every bugfix I'm merging into this piece of software needs a regression test. Yes, I'm aware of that the change might not hurt execution in *nix environments, and not every test needs to have effect in all environments. There's two options: I write the regression test myself or it comes as part of this or another pull request. |
Same here. Especially with changes in subversion being permanent (i.e. committed to the server) and without the possibility for rewriting history (like in git/hg). I hope to address most of this once I get to implementing #38 — so one can choose to execute only the file changing part, or only the vcs part or do something in between. So maybe one tool but two different invocations :) |
Hi Filip, I really would like to write a regression test, I just need some help figuring out what it should test.. Can we agree that these are Unicode strings (due to
my contention is that the reason this works on Linux is due to a bug in Python where it passes its Unicode buffer (internally encoded as utf-8?) directly to Linux' equivalent of Python/Linux does not support putting Unicode directly into the environment as witnessed by the following command:
which on my linux machine produces:
meaning that it is expecting ascii. IOW, the On Windows there is a type error regardless of the content of the Unicode strings (even empty strings cause a type error first):
This is the reason Normally, when I think of regression tests, they are tests using the software and causing a failure before a fix and no failure after a fix. I hope I've convinced you that this is a real problem on Windows, yet there is no way (as far as I can see) to trigger this problem on Linux, at least not by writing a test that uses any of the code in |
Lets make this happen: I created a project for bumpversion on @appveyor and had my fair share of PowerShell config. Current state is "21 failed, 38 passed, 3 xfailed, 24 xpassed in 69.53 seconds" [https://ci.appveyor.com/project/peritus/bumpversion/build/18/job/vh6r93pcb4aj11ur] and that's mostly because I'm not able to configure mercurial non-interactively. The I'll be fixing tests so that appveyor is 100% green, then squash-merge this into master. |
Excellent news :-) Let me know if you need help with anything -- I'm not familiar with AppVeyor, but I know a little bit about developing on Windows. ps: it looks like AppVeyor supports chocolatey (think apt-get for windows), which would make it easier to set up mercurial ("cinst hg"? based on: http://www.appveyor.com/docs/build-configuration#installing-software). |
Using I cherry-picked your commit (w/o the test) to master, added an As for the remaining tests: I'd definitely could use some help from someone more familiar with Python in a windows environment, especially since I don't want Thanks again for your persistence! PS: Should I leave this open for you (now that your change is in master) ? |
FYI, I have this commit to fix some of the test issues ionelmc@63fea11 I think that brought it down to 2 failures on py27 env:
|
@ionelmc Thanks for the pointer! I don't believe complicating the tests by allowing ConfigParsers stupid trailing whitespace (e.g. after For Could you have a look at the current master to see what works now ? |
Ah yes, I'll take a look. The test changes aren't very good (I'd be making a PR if they were any Thanks, On Thu, Jan 15, 2015 at 1:29 PM, Filip Noetzel notifications@github.com
|
I get
on a fresh checkout of master using py2.7.8. For the line ending issue, I have the following fix (mode
the comment refers to opening the file at the beginning of It is correct to use With the |
Thanks @thebjorn, I pushed that to master as e13d8d2 .. We're now down to Do you know |
bumpversion.exe is in fact created on windows, and in later versions of Thanks, On Thu, Jan 15, 2015 at 2:38 PM, Filip Noetzel notifications@github.com
|
The |
Didn't know that, thanks! Pushed that to master here: 41a5075 |
We're down to
(https://ci.appveyor.com/project/peritus/bumpversion/build/41/job/ithxdlw9ji0sq5qh) .. and these seem to be the tricky/important ones. |
If I print out the arguments in the first failing test I get (lhs):
and (rhs):
there is a unicode/byte-string mismatch that is probably benign. I haven't looked into the failure in any more detail (I'll have some time later today (CET) if you'd like me to take a look). |
Closing this because I think the attached code doesn't fix the last few tests that are failing on appveyor (and thus are failing on windows :/ ). If anyone wants to take a look at these, i'd really like to collaborate on making these work, but in another pull-request. |
Bumpversion fails on windows because Unicode values are entered into the
environment (due to
from __future__ import unicode_literals
).I cannot reproduce the problem on linux.
This is the error I'm seeing on windows before this change: