Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support repo's Appstream data download and install #1844

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ option(WITH_COMPS "Build with comps groups and environments support" ON)
option(WITH_MODULEMD "Build with modulemd modules support" ON)
option(WITH_ZCHUNK "Build with zchunk delta compression support" ON)
option(WITH_SYSTEMD "Build with systemd and D-Bus features" ON)
option(WITH_APPSTREAM "Build with appstream support" ON)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename it to WITH_PLUGIN_APPSTREAM and move a bit up together with other plugins.

option(ENABLE_SOLV_URPMREORDER "Build with support for URPM-like solution reordering?" OFF)
option(ENABLE_SOLV_FOCUSNEW "Build with SOLVER_FLAG_FOCUS_NEW libsolv flag enabled to ensure new dependencies are installed in latests versions?" ON)

Expand Down Expand Up @@ -129,6 +130,10 @@ if (WITH_MODULEMD)
add_definitions(-DWITH_MODULEMD)
endif()

if (WITH_APPSTREAM)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be dropped now.

add_definitions(-DWITH_APPSTREAM)
endif()

include_directories("${PROJECT_SOURCE_DIR}/include")
include_directories("${PROJECT_SOURCE_DIR}/common")

Expand Down
22 changes: 22 additions & 0 deletions dnf5.spec
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ Provides: dnf5-command(versionlock)

# ========== build options ==========

%bcond_without appstream
%bcond_without dnf5daemon_client
%bcond_without dnf5daemon_server
%bcond_without libdnf_cli
Expand Down Expand Up @@ -143,6 +144,9 @@ BuildRequires: bash-completion
BuildRequires: cmake
BuildRequires: doxygen
BuildRequires: gettext
%if %{with appstream}
BuildRequires: pkgconfig(appstream) >= 0.16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved inside the libdnf5-plugin-appstream package, right? Otherwise the dnf5 itself would depend on appstream.

%endif
BuildRequires: pkgconfig(check)
BuildRequires: pkgconfig(fmt)
BuildRequires: pkgconfig(json-c)
Expand Down Expand Up @@ -605,6 +609,23 @@ Libdnf5 plugin that allows to run actions (external executables) on hooks.
%{_mandir}/man8/libdnf5-actions.8.*
%endif

# ========== libdnf5-plugin-appstream ==========

%if %{with appstream}

%package -n libdnf5-plugin-appstream
Summary: Libdnf5 plugin to install repo Appstream data
License: LGPL-2.1-or-later
Requires: libdnf5%{?_isa} = %{version}-%{release}

%description -n libdnf5-plugin-appstream
Libdnf5 plugin that installs repository's Appstream data, for repositories which provide them.

%files -n libdnf5-plugin-appstream
%{_libdir}/libdnf5/plugins/appstream.so
%config %{_sysconfdir}/dnf/libdnf5-plugins/appstream.conf

%endif

# ========== libdnf5-plugin-plugin_rhsm ==========

Expand Down Expand Up @@ -797,6 +818,7 @@ automatically and regularly from systemd timers, cron jobs or similar.
\
-DENABLE_SOLV_FOCUSNEW=%{?with_focus_new:ON}%{!?with_focus_new:OFF} \
\
-DWITH_APPSTREAM=%{?with_appstream:ON}%{!?with_appstream:OFF} \
-DWITH_DNF5DAEMON_CLIENT=%{?with_dnf5daemon_client:ON}%{!?with_dnf5daemon_client:OFF} \
-DWITH_DNF5DAEMON_SERVER=%{?with_dnf5daemon_server:ON}%{!?with_dnf5daemon_server:OFF} \
-DWITH_LIBDNF5_CLI=%{?with_libdnf_cli:ON}%{!?with_libdnf_cli:OFF} \
Expand Down
8 changes: 7 additions & 1 deletion include/libdnf5/conf/const.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,15 @@ constexpr const char * METADATA_TYPE_OTHER = "other";
constexpr const char * METADATA_TYPE_PRESTO = "presto";
constexpr const char * METADATA_TYPE_UPDATEINFO = "updateinfo";
constexpr const char * METADATA_TYPE_ALL = "all";
constexpr const char * METADATA_TYPE_APPSTREAM = "appstream";

const std::set<std::string> OPTIONAL_METADATA_TYPES{
METADATA_TYPE_COMPS, METADATA_TYPE_FILELISTS, METADATA_TYPE_OTHER, METADATA_TYPE_PRESTO, METADATA_TYPE_UPDATEINFO};
METADATA_TYPE_COMPS,
METADATA_TYPE_FILELISTS,
METADATA_TYPE_OTHER,
METADATA_TYPE_PRESTO,
METADATA_TYPE_UPDATEINFO,
METADATA_TYPE_APPSTREAM};

} // namespace libdnf5

Expand Down
3 changes: 3 additions & 0 deletions include/libdnf5/repo/repo.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,9 @@ class LIBDNF_API Repo {
// @replaces libdnf:repo/Repo.hpp:method:Repo.downloadMetadata(const std::string & destdir)
void download_metadata(const std::string & destdir);

/// Returns a list of pairs of the rpmmd type and filename of the Appstream data of the repo
std::vector<std::pair<std::string, std::string>> get_appstream_metadata() const;

private:
class LIBDNF_LOCAL Impl;
friend class RepoSack;
Expand Down
4 changes: 4 additions & 0 deletions libdnf5-plugins/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,7 @@ set(CMAKE_C_VISIBILITY_PRESET hidden)
add_subdirectory("actions")
add_subdirectory("python_plugins_loader")
add_subdirectory("rhsm")

if(WITH_APPSTREAM)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the subdirectory unconditionally and move the condition within the appstream/CMakeLists.txt like it is done e.g. for actions plugin.

add_subdirectory("appstream")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works, the Appstream data is installed as expected, with "one little caveat": when a session ends (on the dnf5daemon-server side), you also destroy the plugin class (which is fine), but you also unload the module (.so file). It breaks glib internals, because when another session opens and loads the plugin and calls the appstream library function, the glib aborts the dnf5daemon-server with:

(process:49835): GLib-GObject-CRITICAL **: 15:47:52.469: cannot register existing type 'AsMetadata'

(process:49835): GLib-GObject-CRITICAL **: 15:47:52.469: cannot add private field to invalid (non-instantiatable) type '<invalid>'

(process:49835): GLib-CRITICAL **: 15:47:52.469: g_once_init_leave_pointer: assertion 'result != 0' failed

(process:49835): GLib-GObject-CRITICAL **: 15:47:52.469: g_object_new_with_properties: assertion 'G_TYPE_IS_OBJECT (object_type)' failed
Segmentation fault (core dumped)

This does not exhibit when there's at least one session always loaded, which keeps the plugin module loaded in the memory, thus the glib type system is left in tact.

Is the plugin module unload really needed? I guess it can break more than just glib, it can break anything what stores some global data.

Copy link
Member

@jan-kolarik jan-kolarik Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So this issue is related to the code that loads and unloads shared libraries here.

Could we simply solve this by passing RTLD_NODELETE during the library's dlopen to prevent unloading the libraries for the duration of the daemon's lifetime? Alternatively, should this be conditional for specific plugins only? Another approach could be to implement custom logic to unload libraries when the daemon terminates. @m-blaha @jrohel, could you please share your thoughts on this?

endif()
13 changes: 13 additions & 0 deletions libdnf5-plugins/appstream/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
add_library(appstream_plugin MODULE appstream.cpp)

# disable the 'lib' prefix in order to create appstream.so
set_target_properties(appstream_plugin PROPERTIES PREFIX "")
set_target_properties(appstream_plugin PROPERTIES OUTPUT_NAME "appstream")

pkg_check_modules(APPSTREAM REQUIRED appstream>=0.16)
include_directories(${APPSTREAM_INCLUDE_DIRS})
target_link_libraries(appstream_plugin PRIVATE ${APPSTREAM_LIBRARIES})
target_link_libraries(appstream_plugin PRIVATE libdnf5)

install(TARGETS appstream_plugin LIBRARY DESTINATION "${CMAKE_INSTALL_FULL_LIBDIR}/libdnf5/plugins/")
install(FILES "appstream.conf" DESTINATION "${CMAKE_INSTALL_FULL_SYSCONFDIR}/dnf/libdnf5-plugins")
3 changes: 3 additions & 0 deletions libdnf5-plugins/appstream/appstream.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[main]
name = appstream
enabled = 1
131 changes: 131 additions & 0 deletions libdnf5-plugins/appstream/appstream.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/*
Copyright Contributors to the libdnf project.

This file is part of libdnf: https://github.com/rpm-software-management/libdnf/

Libdnf is free software: you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation, either version 2 of the License, or
(at your option) any later version.

Libdnf is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
GNU General Public License for more details.

You should have received a copy of the GNU General Public License
along with libdnf. If not, see <https://www.gnu.org/licenses/>.
*/

#include <appstream.h>
#include <libdnf5/base/base.hpp>
#include <libdnf5/conf/const.hpp>
#include <libdnf5/plugin/iplugin.hpp>
#include <libdnf5/repo/repo.hpp>
#include <libdnf5/repo/repo_query.hpp>

#include <iostream>

using namespace libdnf5;

namespace {

constexpr const char * PLUGIN_NAME{"appstream"};
constexpr libdnf5::plugin::Version PLUGIN_VERSION{.major = 1, .minor = 0, .micro = 0};

constexpr const char * attrs[]{"author.name", "author.email", "description", nullptr};
constexpr const char * attrs_value[]{"Milan Crha", "[email protected]", "install repo Appstream data."};

class AppstreamPlugin : public plugin::IPlugin {
public:
AppstreamPlugin(libdnf5::plugin::IPluginData & data, libdnf5::ConfigParser &) : IPlugin(data) {}
virtual ~AppstreamPlugin() = default;

PluginAPIVersion get_api_version() const noexcept override { return PLUGIN_API_VERSION; }

const char * get_name() const noexcept override { return PLUGIN_NAME; }

plugin::Version get_version() const noexcept override { return PLUGIN_VERSION; }

const char * const * get_attributes() const noexcept override { return attrs; }

const char * get_attribute(const char * attribute) const noexcept override {
for (size_t i = 0; attrs[i]; ++i) {
if (std::strcmp(attribute, attrs[i]) == 0) {
return attrs_value[i];
}
}
return nullptr;
}

void repos_loaded() override {
Base & base = get_base();
repo::RepoQuery repos(base);
repos.filter_enabled(true);
for (const auto & repo : repos) {
switch (repo->get_type()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is minor, but I would rather do:

auto type = repo->get_type();

if (type == repo::Repo::Type::AVAILABLE || type == repo::Repo::Type::SYSTEM) {
    install_appstream(repo.get());
}

case repo::Repo::Type::AVAILABLE:
case repo::Repo::Type::SYSTEM:
install_appstream(repo.get());
break;
case repo::Repo::Type::COMMANDLINE:;
}
}
}

private:
void install_appstream(libdnf5::repo::Repo * repo);
};

void AppstreamPlugin::install_appstream(libdnf5::repo::Repo * repo) {
libdnf5::Base & base = get_base();
if (!repo->get_config().get_main_config().get_optional_metadata_types_option().get_value().contains(
libdnf5::METADATA_TYPE_APPSTREAM))
return;

std::string repo_id = repo->get_config().get_id();
auto appstream_metadata = repo->get_appstream_metadata();
for (auto & item : appstream_metadata) {
const std::string path = item.second;
GError * local_error = NULL;

if (!as_utils_install_metadata_file(
AS_METADATA_LOCATION_CACHE, path.c_str(), repo_id.c_str(), NULL, &local_error)) {
base.get_logger()->debug(
"Failed to install Appstream metadata file '{}' for repo '{}': {}",
path,
repo_id,
local_error ? local_error->message : "Unknown error");
}

g_clear_error(&local_error);
}
}

} // namespace


PluginAPIVersion libdnf_plugin_get_api_version(void) {
return PLUGIN_API_VERSION;
}

const char * libdnf_plugin_get_name(void) {
return PLUGIN_NAME;
}

plugin::Version libdnf_plugin_get_version(void) {
return PLUGIN_VERSION;
}

plugin::IPlugin * libdnf_plugin_new_instance(
[[maybe_unused]] LibraryVersion library_version,
libdnf5::plugin::IPluginData & data,
libdnf5::ConfigParser & parser) try {
return new AppstreamPlugin(data, parser);
} catch (...) {
return nullptr;
}

void libdnf_plugin_delete_instance(plugin::IPlugin * plugin_object) {
delete plugin_object;
}
11 changes: 10 additions & 1 deletion libdnf5/repo/repo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ extern "C" {
#include <filesystem>
#include <set>


namespace libdnf5::repo {

static void is_readable_rpm(const std::string & fn) {
Expand Down Expand Up @@ -203,6 +202,9 @@ void Repo::read_metadata_cache() {
p_impl->downloader->load_local();
}

std::vector<std::pair<std::string, std::string>> Repo::get_appstream_metadata() const {
return get_downloader().get_appstream_metadata();
}

bool Repo::is_in_sync() {
if (!p_impl->config.get_metalink_option().empty() && !p_impl->config.get_metalink_option().get_value().empty()) {
Expand Down Expand Up @@ -413,6 +415,13 @@ void Repo::load_available_repo() {
auto optional_metadata = p_impl->config.get_main_config().get_optional_metadata_types_option().get_value();
const bool all_metadata = optional_metadata.contains(libdnf5::METADATA_TYPE_ALL);

if (all_metadata || optional_metadata.contains(libdnf5::METADATA_TYPE_APPSTREAM)) {
auto appstream_metadata = p_impl->downloader->get_appstream_metadata();
for (auto & item : appstream_metadata) {
p_impl->solv_repo->load_repo_ext(RepodataType::APPSTREAM, item.first, *p_impl->downloader.get());
}
}

if (all_metadata || optional_metadata.contains(libdnf5::METADATA_TYPE_FILELISTS)) {
p_impl->solv_repo->load_repo_ext(RepodataType::FILELISTS, *p_impl->downloader.get());
}
Expand Down
48 changes: 47 additions & 1 deletion libdnf5/repo/repo_downloader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,25 @@ const std::string & RepoDownloader::get_metadata_path(const std::string & metada
return it != metadata_paths.end() ? it->second : empty;
}

bool RepoDownloader::is_appstream_metadata_type(const std::string & type) const {
/* TODO: make the list configurable with this default */
return utils::string::starts_with(type, "appstream") || utils::string::starts_with(type, "appdata");
}

std::vector<std::pair<std::string, std::string>> RepoDownloader::get_appstream_metadata() const {
std::vector<std::pair<std::string, std::string>> appstream_metadata;
/* The RepoDownloader::common_handle_setup() sets the expected names,
check for the starts_with() only here, to avoid copying the list here. */

for (auto it = metadata_paths.begin(); it != metadata_paths.end(); it++) {
const std::string type = it->first;
const std::string path = it->second;

if (is_appstream_metadata_type(type))
appstream_metadata.push_back(std::pair<std::string, std::string>(type, path));
}
return appstream_metadata;
}

LibrepoHandle RepoDownloader::init_local_handle() {
LibrepoHandle h;
Expand Down Expand Up @@ -498,8 +517,35 @@ void RepoDownloader::common_handle_setup(LibrepoHandle & h) {

// download the rest metadata added by 3rd parties
for (auto & item : optional_metadata) {
dlist.push_back(item.c_str());
// the appstream metadata is a "virtual" type, the list
// of the types is read from the repomd file
if (item.compare(libdnf5::METADATA_TYPE_APPSTREAM) != 0)
dlist.push_back(item.c_str());
}
#ifdef WITH_APPSTREAM
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we can probably drop this conditional in the end as it is now in a separate plugin. There is not anyway anything dependent on the appstream library.

if (optional_metadata.extract(libdnf5::METADATA_TYPE_APPSTREAM)) {
// ideally, the repomd.xml file should be read and every type matching is_appstream_metadata_type()
// would be added from it, but the content is not known at this point and when it is known, then
// it's too late, thus declare some "expected" types to be downloaded here
dlist.push_back("appstream");
dlist.push_back("appstream-icons");
dlist.push_back("appstream-icons-48x48");
dlist.push_back("appstream-icons-48x48@2");
dlist.push_back("appstream-icons-64x64");
dlist.push_back("appstream-icons-64x64@2");
dlist.push_back("appstream-icons-128x128");
dlist.push_back("appstream-icons-128x128@2");
// consult the prefixes with the is_appstream_metadata_type()
dlist.push_back("appdata");
dlist.push_back("appdata-icons");
dlist.push_back("appdata-icons-48x48");
dlist.push_back("appdata-icons-48x48@2");
dlist.push_back("appdata-icons-64x64");
dlist.push_back("appdata-icons-64x64@2");
dlist.push_back("appdata-icons-128x128");
dlist.push_back("appdata-icons-128x128@2");
}
#endif
dlist.push_back(nullptr);
h.set_opt(LRO_YUMDLIST, dlist.data());
}
Expand Down
3 changes: 3 additions & 0 deletions libdnf5/repo/repo_downloader.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class RepoDownloader {
static constexpr const char * MD_FILENAME_GROUP_GZ = "group_gz";
static constexpr const char * MD_FILENAME_GROUP = "group";
static constexpr const char * MD_FILENAME_MODULES = "modules";
static constexpr const char * MD_FILENAME_APPSTREAM = "appstream";

RepoDownloader(const libdnf5::BaseWeakPtr & base, const ConfigRepo & config, Repo::Type repo_type);

Expand All @@ -74,6 +75,7 @@ class RepoDownloader {
void * get_user_data() const noexcept;

const std::string & get_metadata_path(const std::string & metadata_type) const;
std::vector<std::pair<std::string, std::string>> get_appstream_metadata() const;


private:
Expand All @@ -99,6 +101,7 @@ class RepoDownloader {
time_t get_system_epoch() const;

std::set<std::string> get_optional_metadata() const;
bool is_appstream_metadata_type(const std::string & type) const;

libdnf5::BaseWeakPtr base;
const ConfigRepo & config;
Expand Down
Loading
Loading