Change script_helper to use universal_newlines=True in _assert_python

[bitdancer]

BPO	18299
Nosy	@warsaw, @bitdancer, @ceronman, @universecoder
PRs	python/cpython#13847 python/cpython#13908

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['easy', 'type-feature'] title = 'Change script_helper to use universal_newlines=True in _assert_python' updated_at = user = 'https://github.com/bitdancer' ``` bugs.python.org fields: ```python activity = actor = 'shihai1991' assignee = 'none' closed = False closed_date = None closer = None components = [] creation = creator = 'r.david.murray' dependencies = [] files = [] hgrepos = [] issue_num = 18299 keywords = ['patch', 'easy'] message_count = 9.0 messages = ['191854', '228595', '293762', '293792', '293794', '293963', '344748', '344975', '345489'] nosy_count = 4.0 nosy_names = ['barry', 'r.david.murray', 'ceronman', 'Pranav Deshpande'] pr_nums = ['13847', '13908'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue18299' versions = ['Python 3.5'] ```

[bitdancer]

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.)

[83d2e70e-e599-4a04-b820-3814bbdb9bef]

Just a gentle reminder guys.

[07613be8-a377-4cf2-b654-a585219dfd1b]

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

[vstinner]

I really hate scripthelper 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?

[vstinner]

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 scripthelper 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?

[07613be8-a377-4cf2-b654-a585219dfd1b]

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?

[shihai1991]

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.

[vstinner]

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.

[shihai1991]

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