Implement reposync plugin #1903
Conversation
| :ref:`reposync <reposync_plugin_ref-label>` | ||
| | Synchronize packages and metadata of a remote DNF repository to a local directory. | ||
|
|
There was a problem hiding this comment.
For consistency we could also add a link to the See Also section at the bottom of this file.
|
|
||
| if (norepopath_option->get_value() && repos_query.size() > 1) { | ||
| throw libdnf5::cli::ArgumentParserConflictingArgumentsError( | ||
| M_("Can't use --norepopath with multiple repositories enabled")); |
There was a problem hiding this comment.
According to the docs the --safe-write-path can also be used only with a single repository should we also check it here?
| if (urls.empty()) { | ||
| std::cerr << libdnf5::utils::sformat(_("Failed to get mirror for package: \"{}\""), pkg.get_name()) | ||
| << std::endl; | ||
| continue; |
There was a problem hiding this comment.
The continue doesn't seem to be needed?
| if (!optional_metadata_option.get_value().contains(libdnf5::METADATA_TYPE_ALL)) { | ||
| optional_metadata_option.set(libdnf5::METADATA_TYPE_ALL); | ||
| } | ||
| repo.download_metadata(repo_path); |
There was a problem hiding this comment.
If I understand correctly we download the same metadata for the second time here. All enabled repositories were expired in the configure step so we could just copy those and download only the missing parts.
I think this would be a nice optimization but I wouldn't block this PR on that.
| void run() override; | ||
|
|
||
| private: | ||
| using download_list_type = std::vector<std::pair<std::filesystem::path, libdnf5::rpm::Package>>; |
There was a problem hiding this comment.
I may be missing something but would std::map<std::filesystem::path, libdnf5::rpm::Package> be a better fit here?
The paths from the vector are converted to an unordered_set twice in the code, using the map we could avoid that.
Though the order would be different.
There was a problem hiding this comment.
Thank you! The use of vector here is most likely the result of gradual development. Let me check whether the std::map would be a better fit.
There was a problem hiding this comment.
I've replaced the std::vector with std::map, thanks again.
|
I have also noticed in the tests that there is an exit code change: https://github.com/rpm-software-management/ci-dnf-stack/pull/1599/files#diff-ea74dda041309ef66140b8a3856ddc3f3ace60e6e93733efcdc762410324fc39R390 Is this on purpose? |
Thanks for noticing! I did not take a note and forgot about it. The thing is, that currently PackageDownloader::download() does not indicate recoverable errors related to individual targets. It also looks like download() method does not honor Edit: filed as #1926 and added TODO to the reposync code |
Implements command `dnf5 reposync` for creating local copies of remote repositories. The syntax is mostly compatible with the dnf4 version of the command.
To unify the DNF behavior across different commands use the same option to operate on source packages everywhere.
Using std::map<path, Package> instead of std::vector<std::pair<path, Package>> eliminates the need to construct auxiliary sets.
m-blaha
force-pushed
the
mblaha/reposync
branch
from
a09c577 to
d4ccb40
Compare
Implements command
dnf5 reposyncfor creating local copies of remoterepositories.
The syntax is mostly compatible with the dnf4 version of the command.
Resolves: #931