pythonw py3k fixes for issue #1226 #1245

gbrandonp · 2012-01-08T01:56:21Z

first commit strictly fixes Python 3 issues for any platform for file objects, and the second commit is to avoid pythonw.exe immediate failure upon start with Python 3.

Fixes 1226

takluyver · 2012-01-08T22:45:59Z

IPython/utils/io.py

-stdin = IOStream(sys.stdin)
-stdout = IOStream(sys.stdout)
-stderr = IOStream(sys.stderr)
+stdin = sys.stdin if not sys.stdin else IOStream(sys.stdin)


Readability: I think this would be clearer as IOStream(sys.stdin) if sys.stdin else None. Having the 'working' case first seems to make more sense, and it makes it clearer what's happening in the other case.

Functionality: Will anything try to write to IPython.utils.io.stdout when it's None? Is it better to set them up as some sort of black hole? Just speculation, I haven't searched through to see where they're used.

Something like this (utils.io.IOStream has a fallback argument):

devnull = open(os.devnull, 'a') stdin = IOStream(sys.stdin, fallback=devnull) stdout = IOStream(sys.stdout, fallback=devnull) stderr = IOStream(sys.stderr, fallback=devnull)

They are used a little more that my first search turned up.

About the IOTerm object defined just above this: it references sys.std{in,out,err} and creates IOStream objects from them, but should these lines go above that and have the IOTerm class use utils.io.std{in,err,out} already defined as utils.io.IOStream objects?

I'm not sure - maybe @fperez can throw more light on what this section of the code is doing.

@gbrandonp, yes: IOTerm can use io.std* as you suggest, since they've already been defined with the proper fallback.

OK. Done, although I didn't find any actual usage of IOTerm anywhere

fperez · 2012-01-20T04:26:19Z

@takluyver, status of this one? You've looked at it closer than me, so I'm happy to delegate to you here.

takluyver · 2012-01-20T11:50:09Z

There's a bit of clear up to do in utils.text and utils.py3compat, as discussed above. We should also make sure the changes to external libraries are sent upstream (path.py and pexpect), even if they are just doc changes. Hmm, and pexpect upstream is kind of me, now.

fperez · 2012-01-20T19:58:46Z

OK, it looks like you have a good handle on it, I'll leave it to you.

gbrandonp · 2012-01-20T20:43:41Z

Yes, I need to get back to the utils.text cleanup. I looked at it briefly, and didn't have an immediate solution that I could quickly knock out in a few minutes. I'll be home this weekend, so I'll try to get to it sometime before Monday.

fperez · 2012-01-29T01:45:09Z

Just pinging on this one, it looks like we're close to ready but the longer these linger, the higher the risk of a merge conflict against something else.

gbrandonp · 2012-01-30T15:03:58Z

I think what I have will work, though I didn't move all the encoding stuff to a new location. In places where there was already a dependence on py3compat, I used py3compat.getdefaultencoding() instead of sys.getdefaultencoding(); however, I left the couple of remaining uses where py3compat wasn't already imported. Here are the remaining references:

core/magic.py: commented code that uses sys.getdefaultencoding(), so a textual reference instead of a code one
deathrow/gui/wx/ipython_view.py: thought it best not to touch things under deathrow
external/path/_path.py: a couple of uses, which I considered changing, but I'm not sure I should add dependencies on modules in utils

takluyver · 2012-01-30T16:00:48Z

Leaving those three things is fine. I'll check through the places where you've replaced sys.getdefaultencoding() with py3compat.getdefaultencoding() - there may be valid reasons to keep using sys in some places.

takluyver · 2012-02-23T12:22:47Z

@gbrandonp : Just checking whether you've got time to make those last few tweaks - if not, I can grab the branch and do it.

takluyver · 2012-02-23T12:23:12Z

(Didn't mean to close/reopen, I just hit the wrong button)

fperez · 2012-04-15T06:13:39Z

IPython/utils/py3compat.py


 orig_open = open

 def no_code(x, encoding=None):
    return x

+# to deal with the possibility of sys.std* not being a stream at all
+def get_stream_enc(stream, default=None):
+    if not hasattr(stream, 'encoding') or not stream.encoding:


This function should have a docstring.

See our developer docs for guidelines on docstrings

fperez · 2012-04-15T08:24:41Z

This is looking really close to merge, right? With the above tiny fix to the docstring it should be ready to merge, and it will be nice to also knock off #1226 along the way. @gbrandonp, let us know if you need a hand with any of this, I'd love to merge this sooner rather than later.

takluyver · 2012-04-23T14:39:59Z

@gbrandonp : Have you got any more time to work on this? I think it should just be a matter of adding a docstring and putting getdefaultencoding() into its own module, so utils.py3compat can use it. If not, let us know, and one of us will pick it up.

gbrandonp · 2012-04-26T15:37:26Z

There were a couple of tests that failed, however those appear to be related to special characters and long filenames, so I suspect they may be a general problem for tests on windows. I'll see if I can try the patch on a Linux box.

takluyver · 2012-04-26T15:49:43Z

This is the approximate baseline for failing tests on Windows, although it's a month old now. (@jstenar, if you could do a new test run at some point, that would be great):

https://jenkins.shiningpanda.com/ipython/job/ipython-windows-py27/

gbrandonp · 2012-04-26T16:08:56Z

No unexpected tests failed when I ran with py3k on Linux.

takluyver · 2012-04-26T16:24:20Z

Thanks, Brandon, I think that's all looking OK. Unfortunately it's got a merge conflict with something in master. Can you rebase the branch? If you're not sure about rebasing, we can give you a brief run down.

The file type is gone in py3k, and all file objects are under the new io hierarchy. Since open(...) is supported on both Python 2 and 3, use it instead of file in all instances. This isn't caught by 2to3.

The print statement works without error, even though the stdio file objects are instances of None. Since they are instances of None, however, the encoding attribute will not exist, so protect blind attribute access of std{in,out,err} by using a new function, get_stream_enc.

Since IOStream instances require a valid fallback stream, use the locally defined std{in,out,err} instead of sys.std{in,out,err} in IOTerm's __init__ method. Note that the local std{in,out,err} are IOStream instances as well, that fall back to os.devnull

gbrandonp · 2012-04-26T17:03:34Z

Rebase seemed to be fine (rebase is my life since we have subversion at work, and I use git to deal with it).

I just forced the push back to GitHub for this rebased branch. Was that the right thing to do?

takluyver · 2012-04-26T17:19:38Z

Yep, that's right (for IPython, at least - we take a relatively gung-ho attitude to force pushing pull requests). I'll take a final look at it this evening, and then it can go in.

gbrandonp · 2012-04-26T17:24:49Z

OK. Thanks. The GitHub web interface is nice, but a little foreign to me; I feel more comfortable with gitk and the command line tools under Linux. This whole windows thing started when I wanted to be able to do some my work offline on my laptop.

takluyver · 2012-04-26T17:40:03Z

Have you been doing all that editing in Github's web interface?
Impressive! Git is available for Windows, although I don't think it
feels quite native there.

gbrandonp · 2012-04-26T17:47:59Z

No, i meant double-checking the history rewriting that I did. I've got msys-git on windows, and I often use it via SmartGit. The rebase and force-push I did from my linux box, since that's where I'm most comfortable.

takluyver · 2012-04-26T20:13:14Z

IPython/external/path/_path.py

@@ -602,7 +602,7 @@ def write_text(self, text, encoding=None, errors='strict', linesep=os.linesep, a
        u'\x85', u'\r\x85', and u'\u2028'.

        (This is slightly different from when you open a file for
-        writing with fopen(filename, "w") in C or file(filename, 'w')
+        writing with fopen(filename, "w") in C or open(filename, 'w')


Don't forget to forward this change to upstream: https://github.com/dottedmag/path.py

I think it is already fixed upstream, maybe we can also update the file with the new version ?

Nope, upstream still has file(: https://github.com/dottedmag/path.py/blob/master/path.py#L641

We could update the file, although there hasn't been an actual release for some time. That would be a separate PR, anyway.

Hmm. Should I drop this change, or really forward it upstream? It was a change in the docstring, but alas, would be wrong for Python 3.

Is it wrong for Python 3? It looks OK to me. I'd forward it upstream, but it's up to you.

The original comment regarding file would be wrong for Python 3, but it's in a docstring. I just sent a diff to the upstream owner, so I'll call that good and I'll let my current changes from file to open stand.

takluyver · 2012-04-26T20:28:42Z

Once those remaining changes are done, I'll merge this, so if anyone else wants to look over it, now's the time.

for the encode/decode functions, use a module-level cached value of the default encoding given by encoding.getdefaultencoding(). This presumes the value returned by getdefaultencoding isn't going to change over time.

jstenar · 2012-04-28T21:25:13Z

I'll run a test when I return home on may 1st

/Jörgen

26 apr 2012 kl. 17:49 skrev Thomas Kluyver reply@reply.github.com:

This is the approximate baseline for failing tests on Windows, although it's a month old now. (@jstenar, if you could do a new test run at some point, that would be great):

https://jenkins.shiningpanda.com/ipython/job/ipython-windows-py27/

Reply to this email directly or view it on GitHub:
#1245 (comment)

jstenar · 2012-04-28T21:28:29Z

I'll run a test when I return home on may 1st.

/Jörgen

26 apr 2012 kl. 17:49 skrev Thomas Kluyver reply@reply.github.com:

This is the approximate baseline for failing tests on Windows, although it's a month old now. (@jstenar, if you could do a new test run at some point, that would be great):

https://jenkins.shiningpanda.com/ipython/job/ipython-windows-py27/

Reply to this email directly or view it on GitHub:
#1245 (comment)

gbrandonp · 2012-05-04T06:17:05Z

OK. So i replaced calls everywhere from encoding.getdfaultencoding() to encoding.DEFAULT_ENCODING, which gets set when the encoding module is initially imported. This should prevent unnecessary function calls in code.

gbrandonp · 2012-05-04T06:17:45Z

Please let me know if there are other changes needed.

takluyver · 2012-05-04T10:27:55Z

IPython/zmq/iostream.py

@@ -69,7 +69,7 @@ def write(self, string):
        else:
            # Make sure that we're handling unicode
            if not isinstance(string, unicode):
-                enc = text.getdefaultencoding()
+                enc = encoding.getdefaultencoding()


This is the only instance that didn't get converted to use the constant. Is that deliberate?

No it wasn't purposeful. When i get home i can change it, or maybe i can
take a look at that 3.2 failure and fix that miss along with it.
On May 4, 2012 4:27 AM, "Thomas Kluyver" <
reply@reply.github.com>
wrote:

@@ -69,7 +69,7 @@ def write(self, string):
else:
# Make sure that we're handling unicode
if not isinstance(string, unicode):

enc = text.getdefaultencoding()

enc = encoding.getdefaultencoding()

This is the only instance that didn't get converted to use the constant.
Is that deliberate?

Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/1245/files#r773316

I don't think the 3.2 failure is anything to do with this pull request. If you want to change this case for consistency, I'll run the tests again and merge this.

~~Amended the commit.~~ Added a new commit to fix this since it looks like you may have merged.

Sorry for the delay. I was getting on a flight when I saw the email, and didn't have network access for a while.

takluyver · 2012-05-04T10:29:18Z

Thanks. I was actually only thinking of using a constant for the cases in py3compat, but I don't see any harm in doing it all this way. Let's see what the testing bot from PR #1685 makes of this.

takluyver · 2012-05-04T10:51:30Z

Test results for commit 89ac151 merged into master
Platform: linux2

python2.7: OK (libraries not available: matplotlib pymongo wx wx.aui)
python3.2: Failed, log at https://gist.github.com/2594090 (libraries not available: pymongo wx wx.aui)

Not available for testing:python2.6, python3.1

takluyver · 2012-05-04T11:06:47Z

The single test failure with Python 3.2 appears in master as well. I'll file a separate bug for that.

gbrandonp · 2012-05-05T04:36:30Z

OK. I'm done now.

takluyver · 2012-05-05T11:25:27Z

Test results for commit 13bd783 merged into master
Platform: linux2

python2.7: OK
python3.1: OK (libraries not available: matplotlib pymongo qt wx wx.aui zmq)
python3.2: Failed, log at https://gist.github.com/2601690 (libraries not available: pymongo wx wx.aui)

Not available for testing:

takluyver · 2012-05-05T11:33:12Z

OK, that looks fine, and the failing test isn't related to this, so I'll merge it now. Thanks for your patience!

pythonw py3k fixes for issue #1226

pythonw py3k fixes for issue ipython#1226

takluyver reviewed Jan 8, 2012
View reviewed changes

takluyver closed this Feb 23, 2012

takluyver reopened this Feb 23, 2012

fperez reviewed Apr 15, 2012
View reviewed changes

gbrandonp and others added 6 commits April 26, 2012 10:44

substitute open(...) for file(...)

15f8e05

The file type is gone in py3k, and all file objects are under the new io hierarchy. Since open(...) is supported on both Python 2 and 3, use it instead of file in all instances. This isn't caught by 2to3.

moved getdefaultencoding from text to py3compat

521fe81

remove unnecessary function call

56f06ce

Feedback from pull request ipython#1245

6d9ee01

takluyver reviewed Apr 26, 2012
View reviewed changes

only get the default encoding one time

5ca8552

for the encode/decode functions, use a module-level cached value of the default encoding given by encoding.getdefaultencoding(). This presumes the value returned by getdefaultencoding isn't going to change over time.

takluyver reviewed May 4, 2012
View reviewed changes

gbrandonp added 2 commits May 4, 2012 22:16

saner default encoding mechanism

42a53e1

fix missed case of getdefaultencoding() -> DEFAULT_ENCODING

13bd783

takluyver added a commit that referenced this pull request May 5, 2012

Merge pull request #1245 from gbrandonp/pythonw-py3k

4cf98b4

pythonw py3k fixes for issue #1226

takluyver merged commit 4cf98b4 into ipython:master May 5, 2012

takluyver mentioned this pull request May 5, 2012

Windows GUI only (pythonw) bug for IPython on Python 3.x #1226

Closed

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Feedback from pull request ipython#1245

0dcccc8

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#1245 from gbrandonp/pythonw-py3k

926a0f9

pythonw py3k fixes for issue ipython#1226

pythonw py3k fixes for issue #1226 #1245

pythonw py3k fixes for issue #1226 #1245

Conversation

gbrandonp commented Jan 8, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fperez commented Jan 20, 2012

takluyver commented Jan 20, 2012

fperez commented Jan 20, 2012

gbrandonp commented Jan 20, 2012

fperez commented Jan 29, 2012

gbrandonp commented Jan 30, 2012

takluyver commented Jan 30, 2012

takluyver commented Feb 23, 2012

takluyver commented Feb 23, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fperez commented Apr 15, 2012

takluyver commented Apr 23, 2012

gbrandonp commented Apr 26, 2012

takluyver commented Apr 26, 2012

gbrandonp commented Apr 26, 2012

takluyver commented Apr 26, 2012

gbrandonp commented Apr 26, 2012

takluyver commented Apr 26, 2012

gbrandonp commented Apr 26, 2012

takluyver commented Apr 26, 2012

gbrandonp commented Apr 26, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takluyver commented Apr 26, 2012

jstenar commented Apr 28, 2012

jstenar commented Apr 28, 2012

gbrandonp commented May 4, 2012

gbrandonp commented May 4, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takluyver commented May 4, 2012

takluyver commented May 4, 2012

takluyver commented May 4, 2012

gbrandonp commented May 5, 2012

takluyver commented May 5, 2012

takluyver commented May 5, 2012