DnfContext: fix handling of default module profiles

[jlebon]

Currently, default profiles are encoded in modulemd-defaults, which are
separate from the modulemd spec. E.g. in Fedora, it's in:
https://pagure.io/releng/fedora-module-defaults

The ModulePackage::getDefaultProfile() function only queried for the
modulemd data, not modulemd-defaults data. Eventually, once MBS learns
about defaults in modulemds Fedora may start using it, but until then
and regardless we need to support defaults in modulemd-defaults, i.e.
using container->getDefaultProfiles(), just like dnf does.

[jlebon]

/cc @sgallagher for commit message accuracy

Eventually, once MBS learns about defaults in modulemds Fedora may start using it

Is there a link tracking this work BTW? If so, I'll throw the link in there. I looked in https://pagure.io/fm-orchestrator/issues but didn't find anything.

[jlebon]

Now passing CI! 🎉

[sgallagher]

Sorry, I'm on training this week so I'm just now seeing this. The defaults in the repositories will NOT be included in the modulemd-stream metadata, they will still be delivered as modulemd-defaults documents in the repository YAML.

The change was packager-side only: you can specify the defaults in the modulemd-packager v3 documents and MBS will convert that into an appropriate modulemd-defaults document.

It was done this way specifically to avoid needing to change how DNF interprets the data.

[jlebon]

Ack thanks. So it sounds like we could rip out ModulePackage::getDefaultProfiles entirely in that case.

[jlebon]

Updated second commit now to just drop that functionality!

[j-mracek]

Thank you very much for the patch. I did not know that we have ModuleProfile getDefaultProfile() for module package, because it is not used in dnf. Unfortunately we cannot remove it because we have to keep compatibility and ModulePackage is exposed in Python bindings. May I ask you to replace https://github.com/rpm-software-management/libdnf/pull/1278/commits/b74270dc3821b44dde31472568d4b4dcea93a68c by note or recommendation to use getDefaultProfile() from ModuleContainer. Thanks a lot. The rest looks good.

[jlebon]

@j-mracek Thanks for the feedback, I updated the PR!

[j-mracek]

I see that throw std::runtime_error is using in the method several times. But this is C API and with GError ** error, therefore I would prefer that it will directly sets GError. At the end it sets GError because it catches std::runtime_error and transforms it into GError. What's your opinion?

[jlebon]

The reason we throw and catch here is to match dnf's semantics of processing all the inputs before throwing an error. This also matches what functions like dnf_context_module_enable do, where messages are queued up during each iteration if we encounter errors. (And actually, this is what we're doing here too, where in the catch block we queue up a message.)

[j-mracek]

I am really sorry, I get it now. You throw std::runtime_error because you don't want to set g_error (std::runtime_error in not catched by following macro).

#define CATCH_TO_GERROR(RET)                                                   \
catch (const libdnf::Goal::Error& e) {                                       \
    g_set_error_literal(error, DNF_ERROR, e.getErrCode(), e.what());           \
    return RET;                                                                \
} catch (const libdnf::Error& e) {                                             \
    g_set_error_literal(error, DNF_ERROR, DNF_ERROR_INTERNAL_ERROR, e.what()); \
    return RET;                                                                \
}

But this is a wrong approach. gboolean dnf_context_module_install() is C API function therefore it must not thow any C++ exception. For people that uses C++ it is not a problem because the can catch it, but for C users it is unsolvable.

I propose to replace throw std::runtime_error("No default profile found for " + latest->getFullIdentifier());
by

g_set_error_literal();
return FALSE; 

[j-mracek]

It is true that dnf_context_module_enable trow an error, but it happen only when the internal logic of the code would be incorrect. Therefore I expect to terminate program without possibility to catch it when it happen. It is possible to use assert() instead.

[jlebon]

But this is a wrong approach. gboolean dnf_context_module_install() is C API function therefore it must not thow any C++ exception. For people that uses C++ it is not a problem because the can catch it, but for C users it is unsolvable.

The exceptions we throw here never actually bubble to the caller. They're immediately caught and converted to messages.

[jlebon]

It's more like we're using exceptions for flow control. We could equivalently inline the messages.emplace_back() and continue at each site where we currently throw if you prefer, though that would make things more verbose.

[jlebon]

@j-mracek Any more thoughts on this? Would like to have the rpm-ostree side of this merged this week hopefully. :)

[j-mracek]

LGTM