Strict config option deprecation #316

m-blaha · 2023-02-22T10:12:58Z

The PR introduces new config options

skip_broken - for solver to enable resolving problems by skipping problematic packages
skip_unavailable - to not fail when requested package is not available

Deprecated strict option (if explicitely set) is used as a default value for these new options.

Also all occurrences of strict usage in the codebase are replaced by skip_broken / skip_unavailable.

Fixes #269

This is the first step to deprecate and stop using `strict` config option. The problem with strict is that it has two meanings. If set to false, it does: 1. allow to skip packages that are not available in repositories (for install command) 2. allow solver to resolve depsolve problems by skipping packages that are causing these problems After the change the functionality of `strict` option will be implemented using two options - `skip_broken` and `skip_unavailable`.

In case the value of `strict` has been explicitely set, use it's negated value as the new defalt value for `skip_broken` and `skip_unavailable` options.

Now that `--skip-broken` does not set `strict` (which did both skip_broken and skip_unavailable) config option, but only `skip_broken` we need a way how to set also `skip_unavailable`.

m-blaha · 2023-02-22T13:07:26Z

The failing CI test is expected - it uses --skip-broken option which before this patch used to set strict config option. Now with separated skip_broken and skip_unavailable options the test needs to be adapted: rpm-software-management/ci-dnf-stack#1227

Now this option is one of the global options so we do not need to define it locally.

j-mracek · 2023-02-23T09:26:00Z

libdnf/base/goal.cpp

@@ -496,10 +496,16 @@ GoalProblem Goal::Impl::add_group_specs_to_goal(base::Transaction & transaction)
    std::vector<std::tuple<std::string, transaction::TransactionItemReason, comps::GroupQuery, GoalJobSettings>>
        groups_remove, groups_install;
    for (auto & [action, reason, spec, settings] : group_specs) {
-        bool strict = settings.resolve_strict(cfg_main);
+        bool skip_unavailable = settings.resolve_skip_unavailable(cfg_main);
        if (action != GoalAction::INSTALL && action != GoalAction::REMOVE) {
            transaction.p_impl->add_resolve_log(


Because calls of add_resolve_logs() uses a different value for the last bool argument (skip_broken, skip_unavailable, ...) - I would recommend to add a description to that private method and what about to not call the argument as strict. What do you think?

The only meaning of this argument is to distinguish whether the error or warning level is gonna be logged.
So I was thinking that strict in this case kind of make sense. How about calling it is_error?

What about to use: logger::Level instead? It will be mor generic and it will state what it does.

enum class Level : unsigned int { CRITICAL, ERROR, WARNING, NOTICE, INFO, DEBUG, TRACE };

Makes sense. Although bool argument was easier to use.

I've added a commit changing the bool strict argument of logging functions to libdnf::Logger::Level log_level.

j-mracek · 2023-02-23T09:35:18Z

dnf5/main.cpp

-    skip_broken->set_const_value("false");
-    skip_broken->link_value(&config.strict());
+    skip_broken->set_const_value("true");
+    skip_broken->link_value(&config.skip_broken());


I think that description of both option is on incorrect place. Those options are applicable only for a set of transaction commands - for upgrade does not make sense to allow skip-broken and so on. What about to use a similar approach like we have for security options --security

That's a good point. I wasn't completely sure about making them global options. I'll move them to specific commands.

The change (move from global options to specific commands) was a bit bigger than expected. I added it as 3 new commits to this PR - basically to make reviewer's life easier (and my also :)

Feel free to create an issue for dnf5 project. I am fine with not implementing what I suggested in this PR.

It's already here.

These options can be useful also for dnf5 plugins.

Added SkipBrokenOption and SkipUnavailableOption shared options. Also libdnf::cli::session::BoolOption costructor has been extended by optional argument `linked_option` to enable direct linking of BoolOption to configuration option.

Instead of creating global options, use the shared variants only in commands where needed.

Logging functions (transaction.p_impl->report_not_found() and transaction.p_impl->add_resolve_log()) used bool parameter to distinguish at which level the message should be logged. This patch changes the parameter directly to libdnf::Logger::Level making the semantics of the attribute more clear.

j-mracek · 2023-02-27T07:10:26Z

LGTM

m-blaha added 3 commits February 20, 2023 10:17

conf: Set default values of skip_* options

1853628

In case the value of `strict` has been explicitely set, use it's negated value as the new defalt value for `skip_broken` and `skip_unavailable` options.

Replace strict with skip_broken and skip_unavailable

f505232

m-blaha added the ready for review label Feb 22, 2023

j-mracek self-assigned this Feb 22, 2023

dnf5: Add new --skip-unavailable command line option

34cce1f

Now that `--skip-broken` does not set `strict` (which did both skip_broken and skip_unavailable) config option, but only `skip_broken` we need a way how to set also `skip_unavailable`.

m-blaha mentioned this pull request Feb 22, 2023

Enable install-non-existent.feature for dnf5 rpm-software-management/ci-dnf-stack#1227

Merged

builddep: Remove --skip-unavailable option

e186edb

Now this option is one of the global options so we do not need to define it locally.

j-mracek reviewed Feb 23, 2023

View reviewed changes

m-blaha added 4 commits February 23, 2023 12:25

dnf5: Move shared_options.hpp to include/dnf5

75ffe93

These options can be useful also for dnf5 plugins.

dnf5: New shared options for skip_*

4d6c95e

Added SkipBrokenOption and SkipUnavailableOption shared options. Also libdnf::cli::session::BoolOption costructor has been extended by optional argument `linked_option` to enable direct linking of BoolOption to configuration option.

dnf5: Use shared skip_* options

c38923b

Instead of creating global options, use the shared variants only in commands where needed.

j-mracek merged commit 2fcd630 into main Feb 27, 2023

j-mracek deleted the mblaha/strict_deprecation_2 branch February 27, 2023 07:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strict config option deprecation #316

Strict config option deprecation #316

m-blaha commented Feb 22, 2023

m-blaha commented Feb 22, 2023

j-mracek Feb 23, 2023 •

edited

Loading

m-blaha Feb 23, 2023

j-mracek Feb 23, 2023

m-blaha Feb 23, 2023 •

edited

Loading

m-blaha Feb 23, 2023

j-mracek Feb 23, 2023

m-blaha Feb 23, 2023

m-blaha Feb 23, 2023

j-mracek Feb 23, 2023

m-blaha Feb 23, 2023

j-mracek commented Feb 27, 2023

Strict config option deprecation #316

Strict config option deprecation #316

Conversation

m-blaha commented Feb 22, 2023

m-blaha commented Feb 22, 2023

j-mracek Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-blaha Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-mracek commented Feb 27, 2023

j-mracek Feb 23, 2023 •

edited

Loading

m-blaha Feb 23, 2023 •

edited

Loading