Add infrastructure to augment ProcessExecutionFailure

[Eric-Arellano]

There are at least two instances where we want to be able to dynamically enrich failed processes with Pants-specific information: #11941 and #13136.

When enriching, we still want to preserve the original ProcessExecutionFailure though, as it's proved useful for debugging how we dump the argv and std{out,err}.

I suggest that when call sites want to enrich a failure, they should switch from using ProcessResult to FallibleProcessResult, and then manually raise ProcessExecutionFailure rather than relying on this rule to do it automatically:

pants/src/python/pants/engine/process.py

Lines 246 to 270 in 189c933

	@rule
	def fallible_to_exec_result_or_raise(
	fallible_result: FallibleProcessResult,
	description: ProductDescription,
	process_cleanup: ProcessCleanupOption,
	) -> ProcessResult:
	"""Converts a FallibleProcessResult to a ProcessResult or raises an error."""
	if fallible_result.exit_code == 0:
	return ProcessResult(
	stdout=fallible_result.stdout,
	stdout_digest=fallible_result.stdout_digest,
	stderr=fallible_result.stderr,
	stderr_digest=fallible_result.stderr_digest,
	output_digest=fallible_result.output_digest,
	platform=fallible_result.platform,
	metadata=fallible_result.metadata,
	)
	raise ProcessExecutionFailure(
	fallible_result.exit_code,
	fallible_result.stdout,
	fallible_result.stderr,
	description.value,
	process_cleanup=process_cleanup.val,
	)

ProcessExecutionFailure can have a new argument like extra_info: str (or list[str]) that gets added to the resulting error message. TBD whether it should be prepended or appended, and how the delimiter should look.

--

I suggest a prework change to add the classmethod ProcessExecutionFailure.from_process(process: FallibleProcessResult) for DRY.

[Eric-Arellano]

#13896 is an interesting instance of augmenting error messages, but via a log rather than ProcessExecutionFailure.

@benjyw @stuhood @kaos any opinions on the UX of showing a log above the failure, vs. putting in the failure itself.

[kaos]

any opinions on the UX of showing a log above the failure, vs. putting in the failure itself.

I was looking for a way to add some additional context to the ProcessExecutionFailure, and would prefer that to what I have in #13896 with logging just before raising the error.

For me it makes sense to present it something like this:

10:50:18.30 [ERROR] 1 Exception encountered:

  ProcessExecutionFailure: Process 'Building docker image test-example-synth:1.2.5' failed with exit code 1.
stdout:

stderr:
#1 [internal] load build definition from Dockerfile.test-example-synth

[Additional information]

So that the additional information is at the end, i.e. in case of a lot of output etc on stdout/stderr, it will still be on screen.

[Eric-Arellano]

Cool, that's what I was envisioning too. My only question is how to delimit what is Pants vs the process itself. Maybe something like ---------- to divide the section?

[kaos]

There's no clear delimiter between stdout and stderr either. And too much delimitation may cause the information to disconnect too much.

How about indenting all lines from the stdout/stderr, say two (or four, or one tab?) spaces, so each line that begins flushed left is all the default delimitation we need..