unbreq plugin: log standard outputs of failed commands

[mkoncek]

In case that subprocess.run fails, the raised exception does not print the outputs of the command. Those outputs are very important for debugging and should be logged.
I was not sure about the exact format:

• one log event per one line of output?
• one log event per stream?
• ERROR / WARNING / INFO - mapping to stdout / stderr?

An alternative I considered was:

if len(process.stdout) != 0:
	getLog().error("stdout:\n%s", process.stdout.strip())
if len(process.stderr) != 0:
	getLog().error("stderr:\n%s", process.stderr.strip())

[gemini-code-assist]

Code Review

This pull request introduces a helper function, _run_subprocess, to centralize subprocess calls and log their output on failure, which is a great improvement for debuggability. The refactoring is applied consistently throughout the plugin. My feedback includes a couple of suggestions for the new helper function to enhance the logging format and make its error handling more flexible for the different use cases within this file.

[gemini-code-assist]

The new _run_subprocess function always raises an exception on failure. This is a good default, but some call sites, like in _PostDepsHook, previously ignored failures by using check=False. This pull request changes that behavior to fail-fast.

If this is not intended, consider adding a check: bool = True parameter to _run_subprocess to control whether it raises an exception. You could then call it with check=False in _PostDepsHook to preserve the old behavior.

If you decide to do this, please also note that the logic following the call in _PostDepsHook should be updated to check process.returncode, as the existing if mount_options_process: is always truthy.

[mkoncek]

Good point but yes, I believe if the plugin can not invoke /usr/bin/findmnt then it probably can not behave correctly, so the new behaviour is good. I should remove the questioning comment though.

[gemini-code-assist]

The current implementation logs stdout and stderr line by line, which can make logs hard to read if they are interleaved with other messages. As you considered in the pull request description, logging each stream as a single block with a header would be more readable.

Additionally, using mockbuild.util.cmd_pretty to format the command for the exception is more robust than " ".join as it correctly handles arguments with spaces.

Suggested change

	for line in process.stdout.splitlines():
	getLog().error("%s", line)
	for line in process.stderr.splitlines():
	getLog().error("%s", line)
	raise subprocess.CalledProcessError(
	process.returncode, " ".join(process.args), process.stdout, process.stderr
	)
	if process.stdout:
	getLog().error("stdout:\n%s", process.stdout.strip())
	if process.stderr:
	getLog().error("stderr:\n%s", process.stderr.strip())
	raise subprocess.CalledProcessError(
	process.returncode, mockbuild.util.cmd_pretty(process.args), process.stdout, process.stderr
	)

[xsuchy]

This starts looking like do_with_status() https://github.com/rpm-software-management/mock/blob/main/mock/py/mockbuild/util.py#L527

[mkoncek]

Good point, I will try to see if it could be replaced.

[mkoncek]

This PR has grown in size from the initial one, but there should be no more changes.