grass.script: Use text=True by default for subprocesses

[wenzeslaus]

Unlike in the days of Python 2, Popen has now text mode which makes everything just standard (unicode) strings and newlines just LFs. This PR s trying to switch the default from text=False to text=True deep in grass.script in the Popen class wrapper. The idea is that one can still fallback to bytes and encoding/decoding if needed which is approach taken in grass.script.task for XML. All other places should remove the encoding/decoding code. I adjusted crucial places in grass.script, some tested locally on Linux, some not. Changes in grass.script are hopeful in the sense that there should be no impact to the user code. However, some changes are needed in Python tools, suggesting that there is a risk that user code would have to change. This would prevent us from doing this before next major release (v9), but we can likely adjust the grass.script code to allow for both encoded (bytes) and unicode strings (str) as inputs. Our encode and decode utils functions already work as pass-thru when the input type is the desired output type.

I putting this out for discussion and to see how many tests will fail, and I'm especially intersted to see how this turns out on Windows.

[echoix]

See also #4517

[wenzeslaus]

I forgot about #4517, thanks. Yes, this is addressing it. I need to think over how to deal with text because universal_newlines is just terrible (although the fact that it is a terrible name 100% avoids any conflict). The Tools API (#2923) is trying to avoid the conflicts in different way - by passing these special things through constructor rather than together with the tool parameters, at least preferably - plus it has a subprocess.run-like API (accepting list of strings) which would be likely used or at least applicable in a special case when the parameter needs to be used. So it is less of an issue for the Tools API.

[wenzeslaus]

Just to understand better, these warnings are generated when xfail test passes or always? I thought only when it passes, but the wording suggests otherwise.

gui\wxpython\core\testsuite\test_gcmd.py:20
  C:\a\grass\grass\gui\wxpython\core\testsuite\test_gcmd.py:20: UserWarning: Once the test is fixed and passing, remove the @xfail_windows decorator
    @xfail_windows

[echoix]

Always shown. If a test is unintentionally fixed, it will flag as an failure (unexpected success). So we can notice it.

[echoix]

A first error I looked for, in Jupyter utils, the write on line 75 encodes the string to write to proj_input.

grass/python/grass/jupyter/utils.py

Lines 44 to 75 in cee2818

	def reproject_region(region, from_proj, to_proj):
	"""Reproject boundary of region from one projection to another.
	:param dict region: region to reproject as a dictionary with long key names
	output of get_region
	:param str from_proj: PROJ.4 string of region; output of get_location_proj_string
	:param str in_proj: PROJ.4 string of target location;
	output of get_location_proj_string
	:return dict region: reprojected region as a dictionary with long key names
	"""
	region = region.copy()
	# reproject all corners, otherwise reproj. region may be underestimated
	# even better solution would be reprojecting vector region like in r.import
	proj_input = (
	f"{region['east']} {region['north']}\n"
	f"{region['west']} {region['north']}\n"
	f"{region['east']} {region['south']}\n"
	f"{region['west']} {region['south']}\n"
	)
	proc = gs.start_command(
	"m.proj",
	input="-",
	separator=" , ",
	proj_in=from_proj,
	proj_out=to_proj,
	flags="d",
	stdin=gs.PIPE,
	stdout=gs.PIPE,
	stderr=gs.PIPE,
	)
	proc.stdin.write(gs.encode(proj_input))

[wenzeslaus]

Both on macOS and Windows, in gunittest tests, there is a lot of TypeError: 'in <string>' requires string as left operand, not bytes which is a result of testing output as bytes. Fixing this in tests is not a problem, the question is whether we think that users will have such code. If they always convert using gs.decode, then it is not an issue because it works as pass-through for str. However, if they operate with the bytes, then this PR will break their code. It is certainly possible, but the real question is how common it is and whether it is worth breaking those cases for benefit of everything else working as expected.

[echoix]

I've already real an example pseudo code on the subject, in the Python docs. Let me find it back...

Ok: it was here, under displayhook

https://docs.python.org/3/library/sys.html#sys.displayhook

def displayhook(value):
    if value is None:
        return
    # Set '_' to None to avoid recursion
    builtins._ = None
    text = repr(value)
    try:
        sys.stdout.write(text)
    except UnicodeEncodeError:
        bytes = text.encode(sys.stdout.encoding, 'backslashreplace')
        if hasattr(sys.stdout, 'buffer'):
            sys.stdout.buffer.write(bytes)
        else:
            text = bytes.decode(sys.stdout.encoding, 'strict')
            sys.stdout.write(text)
    sys.stdout.write("\n")
    builtins._ = value

Is TypeError the only one or main one that is reported?

[wenzeslaus]

This goes a step further checking encoding errors. The TypeError is for bytes vs str confusion, so that would more be just the buffer check part. Are you suggesting to base our implementation on the displayhook implementation?

[echoix]

Are you suggesting to base our implementation on the displayhook implementation?

Not really, it was using buffer, which isn't always available as they warn in their docs if you redirect to another type of class

[echoix]

It's nice seeing so many new tests (possibly passing)

[echoix]

Can we already note the deprecation and planned removal (like they do for Python docs)

[wenzeslaus]

This is an internal not to remove the code. I don't really know what to say and where. The documentation was written for Python 2 and ASCII strings, so that's actually now fixed. When using universal_newlines=False, using the encoding parameter still makes sense, so no strong reason to drop that piece. What will get dropped is the fallback when you use bytes, but don't set universal_newlines=False. I guess I could review all these functions and actually fully document how they are handling stdnin, stdout, and stderr.

[echoix]

Do we have a tool (module) or extension somewhere that has an option named text that would conflict with this? (Assuming Popen and the tool kwargs are still mixed together)

[wenzeslaus]

We do, but this does not conflict because it is not mixed together anymore. From here, it goes directly to Popen parameters, the tools and its parameters are already a list of strings passed separately. That's different than run_command and family. The only issue is if you want to pass text=False to run_command - we don't support that (text is not in _popen_args in core.py which is used to make the distinction).

[echoix]

So we leave like this or have to do something