[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[hai shi]

Change by hai shi :


--
nosy:  -shihai1991

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[STINNER Victor]

Change by STINNER Victor :


--
nosy:  -vstinner

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[hai shi]

hai shi  added the comment:

spawn_python in script_helper is good enough, so this bug looks like cloud be 
closed.

--

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[hai shi]

Change by hai shi :


--
pull_requests: +13781
pull_request: https://github.com/python/cpython/pull/13908

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[STINNER Victor]

STINNER Victor  added the comment:

I dislike script_helper API. I doesn't allow to pass arbitrary keyword 
parameters to subprocess.Popen. IMHO a new API should be added, but I didn't 
check if script_helper already contains such API or not.

--

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[hai shi]

Change by hai shi :


--
keywords: +patch
pull_requests: +13724
stage: needs patch -> patch review
pull_request: https://github.com/python/cpython/pull/13847

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[hai shi]

hai shi  added the comment:

>it's a pain to extend the API (I don't want to use yet another __xxx custom 
>keyword)
Adding __xxx in run_python_until_end function would increase the complexity but 
it looks like a unified function.

--
nosy: +shihai1991

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[Manuel Cerón]

Change by Manuel Cerón :


--
nosy: +ceronman

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[Pranav Deshpande]

Pranav Deshpande added the comment:

I am afraid I didn't make myself clear. 
I am a beginner when it comes to open source contribution and have decided to 
take up this issue. I did some basic research about the issue and found this 
file:

cpython/Lib/test/support/script_helper.py

The function _assert_python calls run_python_until_end which calls 
subprocess.Popen which takes the parameter universal_newlines=True.

That is all I could discover. Could you guide me further regarding this?

--

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[Mark Lawrence]

Changes by Mark Lawrence :


--
nosy:  -BreamoreBoy

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[STINNER Victor]

STINNER Victor added the comment:

> Even if there turn out to be a few uses where having bytes is required, it 
> would probably be worth catering to the common case and making those 
> exceptional cases use Popen directly.  (Or alternatively provide 
> assert_python_xxx_binary helpers.)

I would prefer the opposite: *always* use script_helper rather than Popen() 
directly in tests. We have to check why some tests use directly Popen?

I know that in test_faulthandler for example, I chose to use directly Popen 
because script_helper always enable faulthandler, there is no option to disable 
this behaviour, and as I wrote in my previous comment, it's a pain to extend 
the API (I don't want to use yet another __xxx custom keyword).

Maybe we need differently API levels in script helper, the lowest level would 
return a Popen object but add -I, -E and/or -X faulthandler.

By the way, I'm using more and more functions like the one I added to 
Lib/test/eintrdata/eintr_tester.py:

@contextlib.contextmanager
def kill_on_error(proc):
"""Context manager killing the subprocess if a Python exception is 
raised."""
with proc:
try:
yield proc
except:
proc.kill()
raise

Such helper should also be moved to script_helper.

For examle, in my perf project I have these two helper functions:

@contextlib.contextmanager
def popen_killer(proc):
try:
yield
except:
# Close pipes
if proc.stdin:
proc.stdin.close()
if proc.stdout:
proc.stdout.close()
if proc.stderr:
proc.stderr.close()
try:
proc.kill()
except OSError:
# process already terminated
pass
proc.wait()
raise


def popen_communicate(proc):
with popen_killer(proc):
return proc.communicate()


Or maybe we should add back these features directly into the subprocess module?

--

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[STINNER Victor]

STINNER Victor added the comment:

I really hate script_helper API: keyword arguments are only used to pass 
environment variables, so the function uses hackish parameteres like 
__cleanenv=True...

I would prefer to pass keywords unchanged to Popen() to be able to use 
universal_newlines=True for example.

Since we have +10k tests, I suggest to not touch the existing API but add yet 
another API: a new function in support/script_helper.py.

What do you think?

--
nosy: +haypo

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[Pranav Deshpande]

Pranav Deshpande added the comment:

Hello, I would like to work on this issue. Could you guide me on so as how to 
proceed with this?

--
nosy: +Pranav Deshpande

___
Python tracker 

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[Mark Lawrence]

Mark Lawrence added the comment:

Just a gentle reminder guys.

--
nosy: +BreamoreBoy
versions: +Python 3.5 -Python 3.4

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

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[R. David Murray]

New submission from R. David Murray:

A look at a random selection of tests that use script_helper indicates that 
using universal_newlines=True would either simplify or make no difference to 
the usage of the script_helper assert_python functions in the majority of the 
tests that use it.  Even if there turn out to be a few uses where having bytes 
is required, it would probably be worth catering to the common case and making 
those exceptional cases use Popen directly.  (Or alternatively provide 
assert_python_xxx_binary helpers.)

--
keywords: easy
messages: 191854
nosy: r.david.murray
priority: normal
severity: normal
stage: needs patch
status: open
title: Change script_helper to use universal_newlines=True in _assert_python
type: enhancement
versions: Python 3.4

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18299
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue18299] Change script_helper to use universal_newlines=True in _assert_python

[Barry A. Warsaw]

Changes by Barry A. Warsaw ba...@python.org:


--
nosy: +barry

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18299
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com