[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Yury Selivanov]

Yury Selivanov added the comment:

Ah, never mind, the commit message has a wrong issue number:

Issue #26801: Added C implementation of asyncio.Future.

Closing this one, will re-open #26081.

--
priority: release blocker -> normal
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Yury Selivanov]

Changes by Yury Selivanov :


--
nosy: +larry, ned.deily
priority: normal -> release blocker

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Yury Selivanov]

Yury Selivanov added the comment:

This patch introduced multiple refleaks in test_asyncgen.

--
nosy: +yselivanov
resolution: fixed -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Roundup Robot]

Roundup Robot added the comment:

New changeset 678424183b38 by INADA Naoki in branch '3.6':
Issue #26801: Added C implementation of asyncio.Future.
https://hg.python.org/cpython/rev/678424183b38

New changeset f8815001a390 by INADA Naoki in branch 'default':
Issue #26801: Added C implementation of asyncio.Future.
https://hg.python.org/cpython/rev/f8815001a390

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Thank you Martin, your comment is helpful.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Roundup Robot]

Roundup Robot added the comment:

New changeset df8652452d25 by Serhiy Storchaka in branch '3.5':
Issue #26801: shutil.get_terminal_size() now handles the case of stdout is
https://hg.python.org/cpython/rev/df8652452d25

New changeset d6e6dcef674f by Serhiy Storchaka in branch 'default':
Issue #26801: shutil.get_terminal_size() now handles the case of stdout is
https://hg.python.org/cpython/rev/d6e6dcef674f

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[STINNER Victor]

STINNER Victor added the comment:

Martin's comment is helpful and LGTM.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Martin Panter]

Martin Panter added the comment:

How about:

try:
size = os.get_terminal_size(sys.__stdout__.fileno())
except (AttributeError, ValueError, OSError):
# stdout is None, closed, detached, or not a terminal, or
# os.get_terminal_size() is unsupported
size = os.terminal_size(fallback)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Could you suggest concrete wording?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Martin Panter]

Martin Panter added the comment:

Serhiy’s patch looks worthwhile to me, though I still think a comment would 
help. There are lots of different cases being handled by those few lines:

try:
   size = os.get_terminal_size(sys.__stdout__.fileno())
except (AttributeError, ValueError, OSError)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Left testing only the most common cases: sys.__stdout__ is None or is non a 
terminal.

--
Added file: http://bugs.python.org/file42529/get_terminal_size_valueerror2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Martin Panter]

Martin Panter added the comment:

I doubt it is worth spending much effort supporting sys.__stdout__ being 
overwritten with StringIO or deleted. That seems an abuse of the “sys” module. 
Idle doesn’t even seem to alter this attribute.

But if you call stdout.close() or detach(), I think that is a more valid way to 
trigger ValueError, so Serhiy’s patch is worthwhile for that case.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Here is a patch that adds ValueError in the exceptions list and adds tests.

--
resolution: fixed -> 
status: closed -> open
Added file: http://bugs.python.org/file42525/get_terminal_size_valueerror.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[STINNER Victor]

Changes by STINNER Victor :


--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[STINNER Victor]

STINNER Victor added the comment:

Well, since Serhiy, Emmanuel and me agree that unit tests are overkill, I 
pushed the obivous and trivial fix. Thank you Emmanual for your contribution! I 
added your name to Misc/ACKS ;-)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Roundup Robot]

Roundup Robot added the comment:

New changeset e3763b5964b6 by Victor Stinner in branch '3.5':
Fix shutil.get_terminal_size() error handling
https://hg.python.org/cpython/rev/e3763b5964b6

New changeset 75f40345d784 by Victor Stinner in branch 'default':
Merge 3.5: issue #26801
https://hg.python.org/cpython/rev/75f40345d784

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Emanuel Barry]

Emanuel Barry added the comment:

To be fair, I don't think we actually need a unit test to check if 
`os.get_terminal_size` exists, as we catch any `AttributeError` at all. I'd 
want to keep the except clause there to properly handle `sys.__stdout__` being 
`None` (or simply absent). I also don't consider that I'm fixing a bug here, 
but more like an oversight. The except clause with `NameError` is obviously an 
oversight from when the function was ported from `os` to `shutil`, so I'd 
rather fix it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[STINNER Victor]

STINNER Victor added the comment:

> Honest, I don't think that we need such complex test for the case that isn't 
> occurred in wild.

Right. I'm also fine if you test manually this corner case.

The Lib/shutil.py change LGTM in get_term_size_with_test2.patch (I ignored the 
unit test).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Honest, I don't think that we need such complex test for the case that isn't 
occurred in wild. If delete os.get_terminal_size, all TermsizeTests tests fail, 
so we will know if encounter a platform without os.get_terminal_size.

Instead I suggest to add ValueError in exceptions list and add tests for 
changed sys.__stdout__: None, StringIO(), open(TESTFN, "w"). Some of these 
tests fail without AttributeError in the exceptions list.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[STINNER Victor]

STINNER Victor added the comment:

> Hmm, if `sys.__stdout__` was deleted (or set to `None`), this would also 
> raise an `AttributeError` when calling `shutil.get_terminal_size`, so I think 
> that even if `os.get_terminal_size` is guaranteed to be always present (which 
> it's not, IIUC), catching `AttributeError` would prevent that bug, too.

It would be nice to have an unit test too for this case. You can use something 
like:

with unittest.mock.patch('shutil.sys') as mock_sys:
del mock_sys.__stdout__
print(shutil.get_terminal_size((3, 3)))

(and mock also os.envion, as shown in the review.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Emanuel Barry]

Changes by Emanuel Barry :


Added file: http://bugs.python.org/file42523/get_term_size_with_test2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Emanuel Barry]

Emanuel Barry added the comment:

Hmm, if `sys.__stdout__` was deleted (or set to `None`), this would also raise 
an `AttributeError` when calling `shutil.get_terminal_size`, so I think that 
even if `os.get_terminal_size` is guaranteed to be always present (which it's 
not, IIUC), catching `AttributeError` would prevent that bug, too.

Should I write a unit test for that too? Even though this one theorically 
covers it as well, I wouldn't want people in the future to think they can 
safely remove `except AttributeError` from the code.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Emanuel Barry]

Emanuel Barry added the comment:

On posix-based OSes, `os.get_terminal_size` might not exist ( 
https://hg.python.org/cpython/file/default/Modules/posixmodule.c#l12462 ), so 
this is needed.

New patch file with tests included.

--
Added file: http://bugs.python.org/file42522/get_term_size_with_test.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[STINNER Victor]

STINNER Victor added the comment:

> On all supported platforms (including such exotic as old AIX, QNX or Minix) 
> it should be defined.

test_shutil always call shutil.get_terminal_size() and I didn't see any failure 
on the unit suite on our buildbots, even less common platforms like OpenBSD, 
AIX, OpenIndiana, etc.

So yes, os.get_terminal_size() looks to be available on all supported platforms.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[STINNER Victor]

STINNER Victor added the comment:

get_terminal_size.diff LGTM.

Would you like to try to write an unit test? Maybe using unittest.mock.patch?

--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> What platform do you get the AttributeError with? Perhaps the function is not 
> well covered in the test suite.

I guess `os.get_terminal_size()` didn't exist on ancient OSes like DOS, OS/2, 
ancient UNIXes. On all supported platforms (including such exotic as old AIX, 
QNX or Minix) it should be defined.

Tests should fail if `os.get_terminal_size()` doesn't exist.

> I think Rietveld doesn't like me because I made it a .diff file, and not a 
> .patch file, but who knows.

I think this is because the patch doesn't contain a revision number.

On Windows `os.get_terminal_size()` can raise ValueError if 
`sys.__stdout__.fileno()` is not one of 0, 1 or 2. It is worth to catch it too. 
An AttributeError is also raised if sys.__stdout__ has no the "fileno" 
attribute (StringIO or None).

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Emanuel Barry]

Emanuel Barry added the comment:

I think Rietveld doesn't like me because I made it a .diff file, and not a 
.patch file, but who knows.

It's a bit of a shot in the dark though, because I can't reproduce an 
environment where `os.get_terminal_size()` doesn't exist. I'm on Windows and 
sometimes compile the latest trunk to play around and find bugs, so it could be 
in one of those times (even though I just tried and it didn't fail).

I think that if `NameError` was to be caught before, it means the function was 
to be "maybe there, maybe not", which could very well still be the case, so it 
makes sense to use the proper exception in that case. I don't see any 
significant drawback to catching AttributeError, anyway.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Martin Panter]

Martin Panter added the comment:

The patch looks good to me. The function was originally written to be included 
in the “os” module, hence the NameError. The patch should probably be fine with 
Mercurial, but it looks like the Reitveld review system doesn’t like it :)

What platform do you get the AttributeError with? Perhaps the function is not 
well covered in the test suite.

--
nosy: +martin.panter
versions: +Python 3.5, Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

[Emanuel Barry]

New submission from Emanuel Barry:

`shutil.get_terminal_size()` will sometimes propagate `AttributeError: module 
 has not attribute 'get_terminal_size'` to the caller. The call checks for 
NameError, which makes no sense, so I guess it must be an oversight. Attached 
patch fixes it.

(diff was generated with git, I don't know if it works with Mercurial, but it's 
a trivial change anyway)

--
components: Library (Lib)
files: get_terminal_size.diff
keywords: patch
messages: 263701
nosy: ebarry
priority: normal
severity: normal
stage: patch review
status: open
title: Fix shutil.get_terminal_size() to catch AttributeError
type: behavior
Added file: http://bugs.python.org/file42511/get_terminal_size.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com