Move all libdnf5::Base members to pImpl and remove deprecated

include/libdnf5/base/base.hpp

@@ -62,10 +62,8 @@ class Base {
 
     ~Base();
 
-    void set_download_callbacks(std::unique_ptr<repo::DownloadCallbacks> && download_callbacks) {
-        this->download_callbacks = std::move(download_callbacks);
-    }
-    repo::DownloadCallbacks * get_download_callbacks() { return download_callbacks.get(); }
+    void set_download_callbacks(std::unique_ptr<repo::DownloadCallbacks> && download_callbacks);
+    repo::DownloadCallbacks * get_download_callbacks();
 
     /// Sets the pointer to the locked instance "Base" to "this" instance. Blocks if the pointer is already set.
     /// Pointer to a locked "Base" instance can be obtained using "get_locked_base()".
@@ -82,19 +80,11 @@ class Base {
     /// The file defined in the current configuration and files in the drop-in directories are used.
     void load_config();
 
-    /// @deprecated Don't use it! It will be removed in Fedora 40. It was intended for internal use only.
-    /// Call a function that loads the config file, catching errors appropriately
-    void with_config_file_path(std::function<void(const std::string &)> func);
-
-    /// @deprecated It is redundant. It calls `load_config()`.
-    /// Loads main configuration.
-    void load_config_from_file();
-
     /// @return a reference to configuration
-    ConfigMain & get_config() { return config; }
-    LogRouterWeakPtr get_logger() { return LogRouterWeakPtr(&log_router, &log_router_gurad); }
-    repo::RepoSackWeakPtr get_repo_sack() { return repo_sack.get_weak_ptr(); }
-    rpm::PackageSackWeakPtr get_rpm_package_sack() { return rpm_package_sack.get_weak_ptr(); }
+    ConfigMain & get_config();
+    LogRouterWeakPtr get_logger();
+    repo::RepoSackWeakPtr get_repo_sack();
+    rpm::PackageSackWeakPtr get_rpm_package_sack();
 
     /// Loads libdnf plugins, vars from environment, varsdirs and installroot (releasever, arch) and resolves
     /// configuration of protected_packages (glob:).
@@ -110,13 +100,13 @@ class Base {
     bool is_initialized();
 
     // TODO(jmracek) Remove from public API due to unstability of the code
-    transaction::TransactionHistoryWeakPtr get_transaction_history() { return transaction_history.get_weak_ptr(); }
-    libdnf5::module::ModuleSackWeakPtr get_module_sack() { return module_sack.get_weak_ptr(); }
+    transaction::TransactionHistoryWeakPtr get_transaction_history();
+    libdnf5::module::ModuleSackWeakPtr get_module_sack();
 
     /// Gets base variables. They can be used in configuration files. Syntax in the config - ${var_name} or $var_name.
-    VarsWeakPtr get_vars() { return VarsWeakPtr(&vars, &vars_gurad); }
+    VarsWeakPtr get_vars();
 
-    libdnf5::BaseWeakPtr get_weak_ptr() { return BaseWeakPtr(this, &base_guard); }
+    libdnf5::BaseWeakPtr get_weak_ptr();
 
     class Impl;
 
@@ -142,19 +132,6 @@ class Base {
     // contains Pool and that has be destructed last.
     // See commit: https://github.com/rpm-software-management/dnf5/commit/c8e26cb545aed0d6ca66545d51eda7568efdf232
     ImplPtr<Impl> p_impl;
-
-    LogRouter log_router;
-    ConfigMain config;
-    repo::RepoSack repo_sack;
-    rpm::PackageSack rpm_package_sack;
-    module::ModuleSack module_sack{get_weak_ptr()};
-    std::map<std::string, std::string> variables;
-    transaction::TransactionHistory transaction_history;
-    Vars vars;
-    std::unique_ptr<repo::DownloadCallbacks> download_callbacks;
-
-    WeakPtrGuard<LogRouter, false> log_router_gurad;
-    WeakPtrGuard<Vars, false> vars_gurad;
 };
 
 }  // namespace libdnf5

libdnf5/base/base.cpp

@@ -31,7 +31,6 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 #include "libdnf5/conf/const.hpp"
 #include "libdnf5/utils/bgettext/bgettext-mark-domain.h"
 
-#include <algorithm>
 #include <atomic>
 #include <cstdlib>
 #include <filesystem>
@@ -46,17 +45,19 @@ namespace libdnf5 {
 static std::atomic<Base *> locked_base{nullptr};
 static std::mutex locked_base_mutex;
 
-Base::Base(std::vector<std::unique_ptr<Logger>> && loggers)
-    : p_impl(new Impl(get_weak_ptr())),
-      log_router(std::move(loggers)),
-      repo_sack(get_weak_ptr()),
-      rpm_package_sack(get_weak_ptr()),
-      transaction_history(get_weak_ptr()),
-      vars(get_weak_ptr()) {}
+Base::Base(std::vector<std::unique_ptr<Logger>> && loggers) : p_impl(new Impl(get_weak_ptr(), std::move(loggers))) {}
 
 Base::~Base() = default;
 
-Base::Impl::Impl(const libdnf5::BaseWeakPtr & base) : rpm_advisory_sack(base), plugins(*base) {}
+Base::Impl::Impl(const libdnf5::BaseWeakPtr & base, std::vector<std::unique_ptr<Logger>> && loggers)
+    : rpm_advisory_sack(base),
+      plugins(*base),
+      log_router(std::move(loggers)),
+      repo_sack(base),
+      rpm_package_sack(base),
+      module_sack(base),
+      transaction_history(base),
+      vars(base) {}
 
 void Base::lock() {
     locked_base_mutex.lock();
@@ -76,15 +77,15 @@ Base * Base::get_locked_base() noexcept {
 }
 
 void Base::load_config() {
-    fs::path conf_file_path{config.get_config_file_path_option().get_value()};
+    fs::path conf_file_path{p_impl->config.get_config_file_path_option().get_value()};
     fs::path conf_dir_path{CONF_DIRECTORY};
     fs::path distribution_conf_dir_path{LIBDNF5_DISTRIBUTION_CONFIG_DIR};
 
-    const auto conf_file_path_priority{config.get_config_file_path_option().get_priority()};
-    const bool use_installroot_config{!config.get_use_host_config_option().get_value()};
+    const auto conf_file_path_priority{p_impl->config.get_config_file_path_option().get_priority()};
+    const bool use_installroot_config{!p_impl->config.get_use_host_config_option().get_value()};
     const bool user_defined_config_file_name = conf_file_path_priority >= Option::Priority::COMMANDLINE;
     if (use_installroot_config) {
-        fs::path installroot_path{config.get_installroot_option().get_value()};
+        fs::path installroot_path{p_impl->config.get_installroot_option().get_value()};
         if (!user_defined_config_file_name) {
             conf_file_path = installroot_path / conf_file_path.relative_path();
         }
@@ -97,19 +98,19 @@ void Base::load_config() {
     for (const auto & path : paths) {
         ConfigParser parser;
         parser.read(path);
-        config.load_from_parser(parser, "main", vars, *get_logger());
+        p_impl->config.load_from_parser(parser, "main", p_impl->vars, *get_logger());
     }
 
     // Finally, if a user configuration filename is defined or the file exists in the default location,
     // it will be loaded.
     if (user_defined_config_file_name || fs::exists(conf_file_path)) {
         ConfigParser parser;
         parser.read(conf_file_path);
-        config.load_from_parser(parser, "main", vars, *get_logger());
+        p_impl->config.load_from_parser(parser, "main", p_impl->vars, *get_logger());
     }
 }
 
-void Base::with_config_file_path(std::function<void(const std::string &)> func) {
+void Base::Impl::with_config_file_path(std::function<void(const std::string &)> func) {
     std::filesystem::path conf_path{config.get_config_file_path_option().get_value()};
     const auto & conf_path_priority = config.get_config_file_path_option().get_priority();
     const auto & use_host_config = config.get_use_host_config_option().get_value();
@@ -131,16 +132,13 @@ void Base::with_config_file_path(std::function<void(const std::string &)> func)
     }
 }
 
-void Base::load_config_from_file() {
-    load_config();
-}
-
 void Base::load_plugins() {
     const char * plugins_config_dir = std::getenv("LIBDNF_PLUGINS_CONFIG_DIR");
-    if (plugins_config_dir && config.get_pluginconfpath_option().get_priority() < Option::Priority::COMMANDLINE) {
+    if (plugins_config_dir &&
+        p_impl->config.get_pluginconfpath_option().get_priority() < Option::Priority::COMMANDLINE) {
         p_impl->plugins.load_plugins(plugins_config_dir);
     } else {
-        p_impl->plugins.load_plugins(config.get_pluginconfpath_option().get_value());
+        p_impl->plugins.load_plugins(p_impl->config.get_pluginconfpath_option().get_value());
     }
 }
 
@@ -150,43 +148,43 @@ void Base::setup() {
 
     // Resolve installroot configuration
     std::string vars_installroot{"/"};
-    const std::filesystem::path installroot_path{config.get_installroot_option().get_value()};
-    if (!config.get_use_host_config_option().get_value()) {
+    const std::filesystem::path installroot_path{p_impl->config.get_installroot_option().get_value()};
+    if (!p_impl->config.get_use_host_config_option().get_value()) {
         // Prepend installroot to each reposdir and varsdir
         std::vector<std::string> installroot_reposdirs;
-        for (const auto & reposdir : config.get_reposdir_option().get_value()) {
+        for (const auto & reposdir : p_impl->config.get_reposdir_option().get_value()) {
             std::filesystem::path reposdir_path{reposdir};
             installroot_reposdirs.push_back((installroot_path / reposdir_path.relative_path()).string());
         }
-        config.get_reposdir_option().set(Option::Priority::INSTALLROOT, installroot_reposdirs);
+        p_impl->config.get_reposdir_option().set(Option::Priority::INSTALLROOT, installroot_reposdirs);
 
         // Unless varsdir paths are specified on the command line, load vars
         // from the installroot
-        if (config.get_varsdir_option().get_priority() < Option::Priority::COMMANDLINE) {
-            vars_installroot = config.get_installroot_option().get_value();
+        if (p_impl->config.get_varsdir_option().get_priority() < Option::Priority::COMMANDLINE) {
+            vars_installroot = p_impl->config.get_installroot_option().get_value();
         }
     }
     // Unless the cachedir or logdir are specified on the command line, they
     // should be relative to the installroot
-    if (config.get_logdir_option().get_priority() < Option::Priority::COMMANDLINE) {
-        const std::filesystem::path logdir_path{config.get_logdir_option().get_value()};
+    if (p_impl->config.get_logdir_option().get_priority() < Option::Priority::COMMANDLINE) {
+        const std::filesystem::path logdir_path{p_impl->config.get_logdir_option().get_value()};
         const auto full_path = installroot_path / logdir_path.relative_path();
-        config.get_logdir_option().set(Option::Priority::INSTALLROOT, full_path.string());
+        p_impl->config.get_logdir_option().set(Option::Priority::INSTALLROOT, full_path.string());
     }
-    if (config.get_cachedir_option().get_priority() < Option::Priority::COMMANDLINE) {
-        const std::filesystem::path cachedir_path{config.get_cachedir_option().get_value()};
+    if (p_impl->config.get_cachedir_option().get_priority() < Option::Priority::COMMANDLINE) {
+        const std::filesystem::path cachedir_path{p_impl->config.get_cachedir_option().get_value()};
         const auto full_path = installroot_path / cachedir_path.relative_path();
-        config.get_cachedir_option().set(Option::Priority::INSTALLROOT, full_path.string());
+        p_impl->config.get_cachedir_option().set(Option::Priority::INSTALLROOT, full_path.string());
     }
-    if (config.get_system_cachedir_option().get_priority() < Option::Priority::COMMANDLINE) {
-        const std::filesystem::path system_cachedir_path{config.get_system_cachedir_option().get_value()};
+    if (p_impl->config.get_system_cachedir_option().get_priority() < Option::Priority::COMMANDLINE) {
+        const std::filesystem::path system_cachedir_path{p_impl->config.get_system_cachedir_option().get_value()};
         const auto full_path = installroot_path / system_cachedir_path.relative_path();
-        config.get_system_cachedir_option().set(Option::Priority::INSTALLROOT, full_path.string());
+        p_impl->config.get_system_cachedir_option().set(Option::Priority::INSTALLROOT, full_path.string());
     }
 
     // Add protected packages from files from installroot
     {
-        auto & protected_option = config.get_protected_packages_option();
+        auto & protected_option = p_impl->config.get_protected_packages_option();
         auto resolved_protected_packages = resolve_path_globs(protected_option.get_value_string(), installroot_path);
         protected_option.set(protected_option.get_priority(), resolved_protected_packages);
     }
@@ -242,7 +240,7 @@ void Base::setup() {
     // (and force to recompute provides) or locked
     const char * arch = vars->get_value("arch").c_str();
     pool_setarch(**pool, arch);
-    module_sack.p_impl->set_arch(arch);
+    p_impl->module_sack.p_impl->set_arch(arch);
     pool_set_rootdir(**pool, installroot.get_value().c_str());
 
     p_impl->plugins.post_base_setup();
@@ -252,4 +250,39 @@ bool Base::is_initialized() {
     return p_impl->pool.get() != nullptr;
 }
 
+ConfigMain & Base::get_config() {
+    return p_impl->config;
+}
+LogRouterWeakPtr Base::get_logger() {
+    return {&p_impl->log_router, &p_impl->log_router_guard};
+}
+repo::RepoSackWeakPtr Base::get_repo_sack() {
+    return p_impl->repo_sack.get_weak_ptr();
+}
+rpm::PackageSackWeakPtr Base::get_rpm_package_sack() {
+    return p_impl->rpm_package_sack.get_weak_ptr();
+}
+
+transaction::TransactionHistoryWeakPtr Base::get_transaction_history() {
+    return p_impl->transaction_history.get_weak_ptr();
+}
+libdnf5::module::ModuleSackWeakPtr Base::get_module_sack() {
+    return p_impl->module_sack.get_weak_ptr();
+}
+
+VarsWeakPtr Base::get_vars() {
+    return {&p_impl->vars, &p_impl->vars_guard};
+}
+
+libdnf5::BaseWeakPtr Base::get_weak_ptr() {
+    return {this, &base_guard};
+}
+
+void Base::set_download_callbacks(std::unique_ptr<repo::DownloadCallbacks> && download_callbacks) {
+    this->p_impl->download_callbacks = std::move(download_callbacks);
+}
+repo::DownloadCallbacks * Base::get_download_callbacks() {
+    return p_impl->download_callbacks.get();
+}
+
 }  // namespace libdnf5

libdnf5/base/base_impl.hpp

@@ -56,9 +56,12 @@ class Base::Impl {
 
     plugin::Plugins & get_plugins() { return plugins; }
 
+    /// Call a function that loads the config file, catching errors appropriately
+    void with_config_file_path(std::function<void(const std::string &)> func);
+
 private:
     friend class Base;
-    Impl(const libdnf5::BaseWeakPtr & base);
+    Impl(const libdnf5::BaseWeakPtr & base, std::vector<std::unique_ptr<Logger>> && loggers);
 
     // RpmPool as the owner of underlying libsolv data, has to be the first member so that it is destroyed last.
     std::unique_ptr<solv::RpmPool> pool;
@@ -77,6 +80,18 @@ class Base::Impl {
     libdnf5::advisory::AdvisorySack rpm_advisory_sack;
 
     plugin::Plugins plugins;
+    LogRouter log_router;
+    ConfigMain config;
+    repo::RepoSack repo_sack;
+    rpm::PackageSack rpm_package_sack;
+    module::ModuleSack module_sack;
+    std::map<std::string, std::string> variables;
+    transaction::TransactionHistory transaction_history;
+    Vars vars;
+    std::unique_ptr<repo::DownloadCallbacks> download_callbacks;
+
+    WeakPtrGuard<LogRouter, false> log_router_guard;
+    WeakPtrGuard<Vars, false> vars_guard;
 };
 
 

libdnf5/base/goal.cpp

@@ -563,7 +563,7 @@ GoalProblem Goal::Impl::add_specs_to_goal(base::Transaction & transaction) {
 
 GoalProblem Goal::Impl::add_module_specs_to_goal(base::Transaction & transaction) {
     auto ret = GoalProblem::NO_PROBLEM;
-    module::ModuleSack & module_sack = base->module_sack;
+    module::ModuleSack & module_sack = *base->get_module_sack();
 
     std::vector<std::string> missing_module_specs;
     for (auto & [action, spec, settings] : module_specs) {
@@ -2317,7 +2317,7 @@ base::Transaction Goal::resolve() {
 
 
     // TODO(jmracek) Apply modules first
-    module::ModuleSack & module_sack = p_impl->base->module_sack;
+    module::ModuleSack & module_sack = *p_impl->base->get_module_sack();
     ret |= p_impl->add_module_specs_to_goal(transaction);
     // Resolve modules
     auto module_error = module_sack.resolve_active_module_items().second;

libdnf5/module/module_db.cpp

@@ -51,7 +51,7 @@ void ModuleDB::initialize() {
             state.first, RuntimeModuleState({.original = state.second, .changed = state.second}));
     }
 
-    for (const auto & module_item : base->module_sack.get_modules()) {
+    for (const auto & module_item : base->get_module_sack()->get_modules()) {
         auto module_name = module_item->get_name();
         if (runtime_module_states.find(module_name) == runtime_module_states.end()) {
             runtime_module_states.emplace(module_name, RuntimeModuleState());

libdnf5/repo/repo_sack.cpp

@@ -588,7 +588,7 @@ void RepoSack::create_repos_from_file(const std::string & path) {
 }
 
 void RepoSack::create_repos_from_config_file() {
-    p_impl->base->with_config_file_path(
+    p_impl->base->p_impl->with_config_file_path(
         std::function<void(const std::string &)>{[this](const std::string & path) { create_repos_from_file(path); }});
 }