Summary

Deprecate execute(), allow it to return strings or bytes and make stripping whitespace from the output optional.

Review Request #13283 — Created Sept. 22, 2023 and updated Nov. 21, 2023, 3:09 p.m.

Information

Owner

maubin

Repository

RBTools

Branch

release-5.x

Bugs

Depends On

Blocks

13282

Reviewers

Groups

rbtools

People

Description

Our rbtools.hooks.common.execute() function uses subprocess.Popen and

Popen.communicate to execute commands and read their output. Between Python 2

and 3, subprocess.Popen(...).communicate() changed from returning a tuple of

strings to a tuple of bytes. This breaks some of our hook util methods in

rbtools.hooks.git where we try to perform string operations on the output

of execute(), but we receive bytes instead of strings.
Python 3.7 added a text parameter to Popen to control whether file

objects, stdin, stdout, and stderr are opened in text mode or binary mode.

We make use of that parameter by adding our own text parameter to execute()

to control whether to return the stdout output as a string or bytes. This

defaults to True to match the previous behavior.
This also makes it optional to strip whitespace from the output, instead of

always stripping it. This is useful when dealing with commands where we

need to preserve the whitespace in the output, such as with git diff.
This changes also deprecates the rbtools.hooks.common.execute() function

in favour of rbtools.utils.process.run_process(), which is more

future-proof and has better type safety.

Testing Done


Ran into an error when running the git pre-receive hook, which came

  from the clash between string and bytes. Applied this fix and ran

  the hook again, saw that the error was gone.
Ran unit tests.

Commits

Summary	ID
Ensure that the execute method returns strings and not bytes.	424c2ac935f8cbca0dbf919ce4f2b2992739c159

Issues

Description	From	Last Updated
While we're here, let's change this from "Executes and returns" to "Execute and return"	david	Sept. 26, 2023, 10:52 a.m.
Might want to specify here that this only applies when text=True.	david	Oct. 30, 2023, 10:50 a.m.

flake8 passed.

JSHint passed.

rbtools/hooks/common.py (Diff revision 1)

Show all issues

While we're here, let's change this from "Executes and returns" to "Execute and return"

Commits:

	Summary	ID
	Ensure that the execute method returns strings and not bytes.	b94f0f5726dd28b4e0d7f2db9afdaf8d41b88c5f
	Ensure that the execute method returns strings and not bytes.	921420575482c61627cc5bb5e505e8f81bb5e897

Diff:

Revision 2 (+36 -6)

Show changes

rbtools/hooks/common.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Summary:

-	Ensure that the execute method returns strings and not bytes.
+	Allow the execute method to return strings or bytes and make stripping whitespace from the output optional.

Description:

		Our `rbtools.hooks.common.execute()` method uses `subprocess.Popen` and
		`Popen.communicate` to execute commands and read their output. Between Python 2
		and 3, `subprocess.Popen(...).communicate()` changed from returning a tuple of
		strings to a tuple of bytes. This breaks some of our hook util methods in
		`rbtools.hooks.git` where we try to perform string operations on the output
		of `execute()`, but we receive bytes instead of strings.

		Python 3.7 added a `text` parameter to `Popen` to control whether file
		objects, stdin, stdout, and stderr are opened in text mode or binary mode.
~		This change sets that parameter to `True` so that we can receive strings
~		instead of bytes.
	~	We make use of that parameter by adding our own `text` parameter to `execute()`
	~	to control whether to return the stdout output as a string or bytes. This
	+	defaults to `True` to match the previous behavior.
	+
	+	This also makes it optional to strip whitespace from the output, instead of
	+	always stripping it. This is useful when dealing with commands where we
	+	need to preserve the whitespace in the output, such as with `git diff`.

Commits:

	Summary	ID
	Ensure that the execute method returns strings and not bytes.	921420575482c61627cc5bb5e505e8f81bb5e897
	Ensure that the execute method returns strings and not bytes.	0e4cabeba8b1268d29bf4f4468916dfd42b0a722

Diff:

Revision 3 (+86 -8)

Show changes

rbtools/hooks/common.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

rbtools/hooks/common.py (Diff revision 3)

Show all issues

Might want to specify here that this only applies when text=True.

Commits:

	Summary	ID
	Ensure that the execute method returns strings and not bytes.	0e4cabeba8b1268d29bf4f4468916dfd42b0a722
	Ensure that the execute method returns strings and not bytes.	0d97af01419d46f88f103a45fd8368e58c092c6e

Diff:

Revision 4 (+88 -8)

Show changes

rbtools/hooks/common.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

This change looks fine, but we should probably deprecate this method in favor of the modern run_process() code. We'll still need to keep this method around for existing hook scripts for now, though.

maubin Nov. 21, 2023, 1:18 p.m.

Sounds good, I'll deprecate it in this change.

Change Summary:


Deprecates the execute function.

Summary:

-	Allow the execute method to return strings or bytes and make stripping whitespace from the output optional.
+	Deprecate execute(), allow it to return strings or bytes and make stripping whitespace from the output optional.

Description:

~		Our `rbtools.hooks.common.execute()` method uses `subprocess.Popen` and
	~	Our `rbtools.hooks.common.execute()` function uses `subprocess.Popen` and
		`Popen.communicate` to execute commands and read their output. Between Python 2
		and 3, `subprocess.Popen(...).communicate()` changed from returning a tuple of
		strings to a tuple of bytes. This breaks some of our hook util methods in
		`rbtools.hooks.git` where we try to perform string operations on the output
		of `execute()`, but we receive bytes instead of strings.

		Python 3.7 added a `text` parameter to `Popen` to control whether file
		objects, stdin, stdout, and stderr are opened in text mode or binary mode.
		We make use of that parameter by adding our own `text` parameter to `execute()`
		to control whether to return the stdout output as a string or bytes. This
		defaults to `True` to match the previous behavior.

		This also makes it optional to strip whitespace from the output, instead of
		always stripping it. This is useful when dealing with commands where we
		need to preserve the whitespace in the output, such as with `git diff`.
	+
	+	This changes also deprecates the `rbtools.hooks.common.execute()` function
	+	in favour of `rbtools.utils.process.run_process()`, which is more
	+	future-proof and has better type safety.

Commits:

	Summary	ID
	Ensure that the execute method returns strings and not bytes.	0d97af01419d46f88f103a45fd8368e58c092c6e
	Ensure that the execute method returns strings and not bytes.	424c2ac935f8cbca0dbf919ce4f2b2992739c159

Diff:

Revision 5 (+106 -8)

Show changes

rbtools/hooks/common.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.