-
Notifications
You must be signed in to change notification settings - Fork 91
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
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) | ||
|
||
|
@@ -129,6 +130,10 @@ if (WITH_MODULEMD) | |
add_definitions(-DWITH_MODULEMD) | ||
endif() | ||
|
||
if (WITH_APPSTREAM) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -143,6 +144,9 @@ BuildRequires: bash-completion | |
BuildRequires: cmake | ||
BuildRequires: doxygen | ||
BuildRequires: gettext | ||
%if %{with appstream} | ||
BuildRequires: pkgconfig(appstream) >= 0.16 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be moved inside the |
||
%endif | ||
BuildRequires: pkgconfig(check) | ||
BuildRequires: pkgconfig(fmt) | ||
BuildRequires: pkgconfig(json-c) | ||
|
@@ -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 ========== | ||
|
||
|
@@ -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} \ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,3 +4,7 @@ set(CMAKE_C_VISIBILITY_PRESET hidden) | |
add_subdirectory("actions") | ||
add_subdirectory("python_plugins_loader") | ||
add_subdirectory("rhsm") | ||
|
||
if(WITH_APPSTREAM) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
endif() |
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") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[main] | ||
name = appstream | ||
enabled = 1 |
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()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is minor, but I would rather do:
|
||
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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
} | ||
|
There was a problem hiding this comment.
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.