Improve logging of requires #4430
Conversation
I'd say it should be separated, wth proper messaging, e.g. "failed to install required packages, see the details below, we're done here" vs "failed to install recommended packages, see the details below, and let's move on".
Yes. We should have a test somewhere checking whether a non-existent package failed to install, it should be enough to extend it with an assert or two to check for new messages. |
| r'No package\s+([^\s\n]+)\s+available', | ||
| r'Unable to locate package\s+([^\s]+)', | ||
| r'E:\s+Unable to locate package\s+([^\s]+)', | ||
| ] |
There was a problem hiding this comment.
Would it make sense to make these patterns package manager-specific? Let's say I'd add a plugin to support Gentoo package manager, emerge: I would need to update this list for emerge messages being decoded. Instead, we can add something like extract_package_name_from_ugly_package_manager_output() method, we can reach guest from here, and we can ask it to give us packages that somehow failed by calling the method with command output. Which patterns, stdout or stderr, that would all be owned by the package manager implementation. Plus, we don't need to run apk patterns for dnf output, and vice versa.
Second point: those patterns should be compiled, see various re.compile() calls on module level.
There was a problem hiding this comment.
👍 for having the package managers owning these.
| failed_packages = self._extract_failed_packages(exceptions) | ||
| if failed_packages: | ||
| self._show_failed_packages_with_tests(failed_packages) |
There was a problem hiding this comment.
Why not move this into the PrepareInstall plugin? Here it seems fragile because it is catching any exceptions.
| if exceptions: | ||
| # TODO: needs a better message... | ||
| failed_packages = self._extract_failed_packages(exceptions) | ||
| if failed_packages: |
There was a problem hiding this comment.
Nitpick: Using the walrus operator makes this more succinct.
| self.info('failed packages', '', 'red', shift=1) | ||
|
|
||
| for failed_package in failed_packages: | ||
| matching_tests = [] |
There was a problem hiding this comment.
You don’t need to initialize matching_tests and then overwrite it.
matching_tests = dependencies_to_tests.get(failed_package, [])
| if failed_package in dependencies_to_tests: | ||
| matching_tests = dependencies_to_tests[failed_package] | ||
| sorted_matching_tests = ', '.join(sorted(matching_tests)) | ||
| self.info(failed_package, f'required by {sorted_matching_tests}', color='red', shift=2) |
There was a problem hiding this comment.
If no matching_tests found, only required by gets reported. Maybe add an explicit check?
| """ | ||
| Extract package names from installation exceptions. | ||
| Returns a list of package names that failed to install. |
There was a problem hiding this comment.
Docstring mismatch with the return type.
| for exc in exceptions: | ||
| # Extract failed packages from RunError stderr/stdout | ||
| if isinstance(exc, tmt.utils.RunError): | ||
| error_text = '' |
There was a problem hiding this comment.
Just this maybe?
error_text = '\n'.join(
text for text in (exc.stderr, exc.stdout) if text
)There was a problem hiding this comment.
Or not merging them at all, e.g. with something like the following: an inner function to focus on a single exception, produce a sequence of package names, and then merge these "streams" into a single stream by the magic of itertools.chainining them together. This iterable can be turned into a set. Not tested, but this could be a starting point:
def _extract_failed_packages(self, exceptions: list[Exception]) -> set[str]:
def _extract_from_exception(exception: Exception) -> Iterator[str]:
if not isinstance(exc, tmt.utils.RunError):
return
if exc.stderr:
yield from guest.package_manager.extract_package_name_from_ugly_package_manager_output(exc.stderr)
if exc.stdout:
yield from guest.package_manager.extract_package_name_from_ugly_package_manager_output(exc.stdout)
return set(
itertools.chain.from_iterable(
_extract_from_exception(exc) for exc in exceptions
)
)
Pull Request Checklist
dependencies_to_testgathers both requires + recommends and tmt is going to fail when something could not be installed in the require phase already. If all required packages are available to install, but a recommended one is not, then it will fail also, but the message{package}: required by {test_name}might be a bit misleading, because it is not required, only recommended. Should the messaging be handled separately or is simply "needed by" enough to cover both requires+recommend in general?Output:
Test coverage needed?
Fixes #4302