libdnf5: enable/disable plugins via Base API

[softwaresale]

• Add Base::enable_plugins and Base::disable_plugins API to (dis|en)able plugins programmatically
• Add Plugins:set_plugin_enablement to configure loaded plugins

I am unsure if implementation discussion should happen in PRs or in Issues. If these kinds of questions should be posted in an issue instead, I can move the discussion there. I have a few questions about my implementation:

1. Is my Base API acceptable? In the issue there is just one init_plugins method that both enables and disable plugins at the same time. I decided to have two separate methods as it seems cleaner if the programmer only wants to enable/disable at one time. I am open to changing to match the DNF4 style.
2. I use Base to track which plugins should be enabled/disabled, and then do plugin enablement/disablement in Plugins during Base::setup. Is this design adequate? or is there something I'm missing?
3. I use plugin id/name to keep track of which plugins should be disabled. Is there a better/more unique identifier that I should use?
4. I am unsure where to place tests.

Thanks in advance!

Fixes #391

[jrohel]

@softwaresale Thanks for the PR.

1. API: The idea of creating Base::enable_plugins and Base::disabled_plugins seems OK.

2. and 3. I see problems here.

Description of enable_plugins:
A collection of plugins that should be enabled. If these plugins are loaded and not enabled by default, then they will be enabled.

Plugins that are disabled in the configuration file are not loaded. So they cannot be enabled with your enable_plugins method. And if they are enabled in the configuration file, they have enabled set to true. In other words, this method can only enable plugins that were previously disabled using your second disable_plugins method. Nothing more. I find it unusable in practice.

Description of disable_plugins:
A collection of plugins that should be disabled. If these plugins are loaded, they will not be initialized

This will work. But it is not consistent with the config file. If the plugin is disabled in the configuration file, it is not loaded at all. This ensures that it cannot affect libdnf5 in any way. If the plugin is disabled using your disable_plugins method it will be loaded. So there is a difference between whether we disable the plugin in the configuration file or by your method.

I'm considering if you could modify the PR to behave the same as when the plugin is disabled in the config file. But that brings up the second problem. Until the plugin is loaded, it is not possible to call the method returning its name from it.

Can you please wait a little longer with this PR? I am dealing with another issue related to this.

Note: PR did not pass tests in Pre Commit. clang-format failed.
There is a ".clang-format" configuration file and a "clang-format" reformatting script in the repository.
Some information is here: https://github.com/rpm-software-management/dnf5/blob/main/CONTRIBUTING.md

[jrohel]

@softwaresale
I apologize for waiting. I modified the Plugins::load_plugin method in libdnf5.

And I propose these changes in your PR:

1. Remove Plugins::set_plugins_enabled method. Instead, pass the list of enabled/disabled plugins to the Plugins::load_plugin method. So add an argument to Plugins::load_plugins and Plugins::load_plugin methods. Method
   Plugins::load_plugin will first check whether the plugin is enabled/disabled in the passed list and if not it will follow the content of the enabled option in the configuration file.
   This will make disabling and enabling the plugin using the API have the same result as using the configuration file.

2. Add glob pattern support for plugin name. DNF4 also has this support. Use sack::match_string function with sack::QueryCmp::GLOB argument.

3. What if the plugin is listed in both lists - enabled and disabled? I think disabled will win in DNF4. This allows for example to enable just one plugin with glob patterns: disable *, enable <plugin_name>.
   However, it is more general to apply the rules in the order they were entered. This means having both enable and disable requests in one list that will preserve the order of enable/disable requests. The last matching enable/dissale request will be applied to the plugin.

[softwaresale]

Of course! I'm glad you got back to me at all lol. Those changes seem pretty straight-forward. I'll have updates pushed soon

[softwaresale]

How should Base know about plugins that are enabled or disabled? I added a new param to Plugins::load_plugins. That new call needs to be made from base though. How should I make that happen?

[jrohel]

@softwaresale

How should Base know about plugins that are enabled or disabled?

1. There was probably a misunderstanding. You made more changes than I suggested. I wrote "Remove Plugins::set_plugins_enabled method." But, I didn't write that you should remove Base::disable_plugins and Base::enable_plugins methods. Put them back and your problem will be solved.

2. I wrote about having all enable/disable requests in one list that preserves their order. This can be arranged with a simple changes in the original PR.
   Replace the line std::unordered_map<std::string, bool> plugin_enablement; with PreserveOrderMap<std::string, bool> plugin_enablement;
   And add the line plugin_enablement.erase(plugin_id); before calling plugin_enablement.insert(...) in the implementation of Base::disable_plugins and Base::enable_plugins. This ensures that if the same pattern has already been added to the map before, it is removed and the new one is inserted at the end. The order is therefore preserved and unnecessary (repeating) patterns are discarded. The first matching from the end is used. Repeating patterns are really useless.

3. The Plugins::load_plugins and Plugins::load_plugin methods can then directly receive plugin_enablement as an argument.
   And in Plugins::load_plugin this argument is used and only if the plugin is not found in the list, the value from the configuration file is used. Example code:

    bool is_enabled;
    bool is_enabled_set{false};
    for (auto it = plugin_enablement.rbegin(); it != plugin_enablement.rend(); ++it) {
        if (sack::match_string(plugin_name, sack::QueryCmp::GLOB, it->first)) {
            is_enabled = it->second;
            is_enabled_set = true;
            break;
        }
    }
    if (!is_enabled_set) {
        enum class Enabled { NO, YES, HOST_ONLY, INSTALLROOT_ONLY } enabled;
        const auto & enabled_str = parser.get_value("main", "enabled");
        if (enabled_str == "host-only") {
            enabled = Enabled::HOST_ONLY;
        } else if (enabled_str == "installroot-only") {
            enabled = Enabled::INSTALLROOT_ONLY;
        } else {
            try {
                enabled = OptionBool(false).from_string(enabled_str) ? Enabled::YES : Enabled::NO;
            } catch (OptionInvalidValueError & ex) {
                throw OptionInvalidValueError(M_("Invalid option value: enabled={}"), enabled_str);
            }
        }
        const auto & installroot = base->get_config().get_installroot_option().get_value();
        is_enabled = enabled == Enabled::YES || (enabled == Enabled::HOST_ONLY && installroot == "/") ||
                     (enabled == Enabled::INSTALLROOT_ONLY && installroot != "/");
    }

Basically, apart from the loop, not much code has been added. The original code is moved to the condition and will only be executed if the plugin was not found in the plugin_enablement list.

When you wrote "Those changes seem pretty straight-forward", I thought you were going to make small changes to the original PR. Not that you throw it all away, including the API in Base, and start over.
As I wrote, we did not understand each other.

[softwaresale]

Yeah, I'm sorry I completely misinterpreted what you asked.

I wrote "Remove Plugins::set_plugins_enabled method." But, I didn't write that you should remove Base::disable_plugins and Base::enable_plugins methods. Put them back and your problem will be solved.

I totally conflated these two for some reason. I felt like I was supposed to be going in a different direction and not fixing what I already had.

My bad. I'll fix it soon.

[jrohel]

@softwaresale The PreserveOrderMap class template I mentioned in the previous comment is available in the libdnf5 namespace. So, as I wrote, std::unordered_map<std::string, bool> can be replaced by PreserveOrderMap<std::string, bool>. I mention this just to be sure, to avoid misunderstandings and you not implementing your own class.

[jan-kolarik]

@softwaresale Hello, please do you still plan to work on this?

[jrohel]

@softwaresale I'm sorry, but I'm going to close this PR. We have waited a long time for a response. But, we need to solve the issue. So, I prepared another PR.