Improve logging of requires

[bajertom]

Pull Request Checklist

• implement the feature
• extend the test coverage
• include a release note

dependencies_to_test gathers 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:

---------------------------✄-------------------------------------
        prepare task #3: requires on default-0
        how: install
        summary: Install required packages
        name: requires
        where: default-0
        package: sed, grep, coreutils and 5 more
        warn: Installation failed, trying again after metadata refresh.
        package: sed, grep, coreutils and 5 more
        fail: Command 'rpm -q --whatprovides sed grep coreutils bash absent wrong nonexistent missing || dnf5 install -y  sed grep coreutils bash absent wrong nonexistent ridiculous' returned 1.
    
        failed packages: 
            missing: required by /tests/FIRST
            absent: required by /tests/FIRST, /tests/SECOND
            wrong: required by /tests/FIRST, /tests/SECOND
            nonexistent: required by /tests/FIRST
    report
        how: display
        summary: 2 pending
    cleanup
---------------------------✄-------------------------------------

Test coverage needed?

Fixes #4302

[happz]

Pull Request Checklist

• implement the feature
• extend the test coverage
• include a release note

dependencies_to_test gathers 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?

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".

Output:

---------------------------✄-------------------------------------
        prepare task #3: requires on default-0
        how: install
        summary: Install required packages
        name: requires
        where: default-0
        package: sed, grep, coreutils and 5 more
        warn: Installation failed, trying again after metadata refresh.
        package: sed, grep, coreutils and 5 more
        fail: Command 'rpm -q --whatprovides sed grep coreutils bash absent wrong nonexistent missing || dnf5 install -y  sed grep coreutils bash absent wrong nonexistent ridiculous' returned 1.
    
        failed packages: 
            missing: required by /tests/FIRST
            absent: required by /tests/FIRST, /tests/SECOND
            wrong: required by /tests/FIRST, /tests/SECOND
            nonexistent: required by /tests/FIRST
    report
        how: display
        summary: 2 pending
    cleanup
---------------------------✄-------------------------------------

Test coverage needed?

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.

[bajertom]

I have addressed comments and suggestions in b46f5c7
If recommended packages can not be installed, tmt will not raise an exception and therefore I had to use CommandOutput instead and parse the packages from there.

For the "unattributed_packages" - I don't know if that can even happen that packages would not be required/recommended - but maybe the "essential requires" like beakerlib and /usr/bin/flock could fall into this category?

[happz]

Please, decorate with @abc.abstractmethod to make clear to linters this is an abstract method.

[happz]

Or not. After seeing the actual methods, how about moving the patterns into classes, making something like _PACKAGE_NAME_IN_PM_OUTPUT_PATTERNS = [...], and the default implementation of this method can do what all the methods do: iterate over a list of patterns, match, yield. They are really repeating the same set of lines, right? The only thing that's different is the actual set of patterns.

[bajertom]

Moved to classes. Should the compiles themselves be also inside a class?

[happz]

Oh yes, why not?

class ApkEngine(PackageManagerEngine):
    install_command = Command('add')


    _engine_class = ApkEngine

    _PACKAGE_NAME_IN_PM_OUTPUT_PATTERNS = [
        re.compile(r'unable to locate package\s+([^\s]+)', re.IGNORECASE),
        re.compile(r'ERROR:\s+([^\s:]+):\s+No such package', re.IGNORECASE)
    ]

[happz]

Would (?:E:\s+)? be possible? Then we would need just one pattern, with the E: prefix being optional, the rest seems to be the same.

[bajertom]

I've used the non-capture group like you suggested.

[bajertom]

I've addressed the comments in latest commits, now extending the tests remains.