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.

Sign up for GitHub

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

During package donwload setup first add all downloads then handle local #1989

Merged
merged 4 commits into from
Jan 21, 2025
Merged

During package donwload setup first add all downloads then handle local #1989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39093c6
Add couple explaining comments to `MultiProgressBar::operator<<`
kontura Jan 7, 2025
9bd6f52
Enhance `perform_control_sequences()` to handle colors
kontura Jan 7, 2025
23107e1
Test MultiProgressBar with already finished downloads
kontura Jan 7, 2025
3ebf1c1
During package download setup first add all downloads then handle local
kontura Jan 7, 2025
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
2 changes: 2 additions & 0 deletions libdnf5-cli/progressbar/multi_progress_bar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ std::ostream & operator<<(std::ostream & stream, MultiProgressBar & mbar) {
text_buffer << "\r";
}

// Number of lines that need to be cleared, including the last line even if it is empty
mbar.num_of_lines_to_clear = 0;
mbar.line_printed = false;

Expand Down Expand Up @@ -209,6 +210,7 @@ std::ostream & operator<<(std::ostream & stream, MultiProgressBar & mbar) {
}

text_buffer << mbar.total;
// +1 for the divider line, +1 for the total bar line
mbar.num_of_lines_to_clear += 2;
if (mbar.total.is_finished()) {
text_buffer << std::endl;
Expand Down
59 changes: 34 additions & 25 deletions libdnf5/repo/package_downloader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ void PackageDownloader::download() try {

std::vector<std::unique_ptr<LrPackageTarget>> lr_targets;
lr_targets.reserve(p_impl->targets.size());
std::vector<PackageTarget *> local_targets;
for (auto & pkg_target : p_impl->targets) {
if (use_cache_only && !pkg_target.package.is_available_locally()) {
throw RepoCacheonlyError(
Expand All @@ -164,31 +165,11 @@ void PackageDownloader::download() try {
}

if (pkg_target.package.is_available_locally()) {
// Copy local packages to their destination directories
std::filesystem::path source = pkg_target.package.get_package_path();
std::filesystem::path destination = pkg_target.destination / source.filename();
std::error_code ec;
const bool same_file = std::filesystem::equivalent(source, destination, ec);
if (!same_file) {
std::filesystem::copy(source, destination, std::filesystem::copy_options::overwrite_existing, ec);
}
if (auto * download_callbacks = pkg_target.package.get_base()->get_download_callbacks()) {
std::string msg;
DownloadCallbacks::TransferStatus status;
if (ec) {
status = DownloadCallbacks::TransferStatus::ERROR;
msg = ec.message();
} else {
status = DownloadCallbacks::TransferStatus::ALREADYEXISTS;
if (same_file) {
msg = "Already downloaded";
} else {
msg = fmt::format("Copied from {}", source.string());
}
}
pkg_target.need_call_end_callback = false;
download_callbacks->end(pkg_target.user_cb_data, status, msg.c_str());
}
// First only gather all local package targets, we cannot handle them right away because
// the download callbacks are not fully initialized (they don't contain all required downloads).
// This could cause problems for example when the first added pkg_target is local and we called
// the `end` callback right here the callbacks might think that they are finished.
local_targets.push_back(&pkg_target);
continue;
}

Expand Down Expand Up @@ -216,6 +197,34 @@ void PackageDownloader::download() try {
lr_targets.emplace_back(lr_target);
}

for (auto * local_pkg_target : local_targets) {
// Copy local packages to their destination directories
std::filesystem::path source = local_pkg_target->package.get_package_path();
std::filesystem::path destination = local_pkg_target->destination / source.filename();
std::error_code ec;
const bool same_file = std::filesystem::equivalent(source, destination, ec);
if (!same_file) {
std::filesystem::copy(source, destination, std::filesystem::copy_options::overwrite_existing, ec);
}
if (auto * download_callbacks = local_pkg_target->package.get_base()->get_download_callbacks()) {
std::string msg;
DownloadCallbacks::TransferStatus status;
if (ec) {
status = DownloadCallbacks::TransferStatus::ERROR;
msg = ec.message();
} else {
status = DownloadCallbacks::TransferStatus::ALREADYEXISTS;
if (same_file) {
msg = "Already downloaded";
} else {
msg = fmt::format("Copied from {}", source.string());
}
}
local_pkg_target->need_call_end_callback = false;
download_callbacks->end(local_pkg_target->user_cb_data, status, msg.c_str());
}
}

// Adding items to the end of GSList is slow. We go from the back and add items to the beginning.
GSList * list{nullptr};
for (auto it = lr_targets.rbegin(); it != lr_targets.rend(); ++it) {
Expand Down
76 changes: 74 additions & 2 deletions test/libdnf5-cli/test_progressbar_interactive.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ namespace {
std::string perform_control_sequences(std::string target) {
const char * raw_string = target.c_str();

std::string amount_str = "";
size_t amount = 1;
enum ControlSequence {
EMPTY,
Expand All @@ -63,9 +64,11 @@ std::string perform_control_sequences(std::string target) {
} else if (current_value == '[' && state == ESC) {
state = CONTROL_SEQUENCE_INTRO;
amount = 1;
} else if (isdigit(current_value) && state == CONTROL_SEQUENCE_INTRO) {
amount_str.clear();
} else if ((state == CONTROL_SEQUENCE_INTRO || state == CONTROL_SEQUENCE_AMOUNT) && isdigit(current_value)) {
// current_value has to be one of: 0123456789
amount = static_cast<size_t>(current_value - '0');
amount_str += current_value;
amount = (size_t)stoi(amount_str);
state = CONTROL_SEQUENCE_AMOUNT;
} else if ((state == CONTROL_SEQUENCE_INTRO || state == CONTROL_SEQUENCE_AMOUNT) && current_value == 'A') {
if (amount > current_row) {
Expand All @@ -82,6 +85,12 @@ std::string perform_control_sequences(std::string target) {
output[current_row].clear();
}
state = EMPTY;
} else if (state == CONTROL_SEQUENCE_AMOUNT && current_value == 'm') {
// This is a color control sequence, just skip it
state = EMPTY;
} else if (state == CONTROL_SEQUENCE_INTRO || state == CONTROL_SEQUENCE_AMOUNT) {
CPPUNIT_FAIL(fmt::format(
"Unknown control sequence: \\x1b[{}{} at position: {}", amount_str, current_value, input_pos));
} else {
while (current_row >= output.size()) {
output.push_back({});
Expand Down Expand Up @@ -517,3 +526,66 @@ void ProgressbarInteractiveTest::test_multi_progress_bar_with_short_messages() {

ASSERT_MATCHES(expected, perform_control_sequences(oss.str()));
}

void ProgressbarInteractiveTest::test_multi_progress_bars_with_already_downloaded() {
auto download_progress_bar1 = std::make_unique<libdnf5::cli::progressbar::DownloadProgressBar>(10, "test");
auto download_progress_bar2 = std::make_unique<libdnf5::cli::progressbar::DownloadProgressBar>(10, "test");
auto download_progress_bar3 = std::make_unique<libdnf5::cli::progressbar::DownloadProgressBar>(10, "test");
auto download_progress_bar1_raw = download_progress_bar1.get();
auto download_progress_bar2_raw = download_progress_bar2.get();
auto download_progress_bar3_raw = download_progress_bar3.get();

libdnf5::cli::progressbar::MultiProgressBar multi_progress_bar;
multi_progress_bar.add_bar(std::move(download_progress_bar1));
multi_progress_bar.add_bar(std::move(download_progress_bar2));
multi_progress_bar.add_bar(std::move(download_progress_bar3));

// First download progress bar represents already downloaded package
download_progress_bar1_raw->set_ticks(0);
download_progress_bar1_raw->set_total_ticks(0);
download_progress_bar1_raw->add_message(libdnf5::cli::progressbar::MessageType::SUCCESS, "Already Downloaded");
download_progress_bar1_raw->start();
download_progress_bar1_raw->set_state(libdnf5::cli::progressbar::ProgressBarState::SUCCESS);

std::ostringstream oss;
oss << multi_progress_bar;
Pattern expected =
"\\[1/3\\] test 100% | ????? ??B\\/s | 0.0 B | ???????\n"
">>> Already Downloaded \n"
"----------------------------------------------------------------------\n"
"\\[1/3\\] Total ???% | ????? ??B\\/s | -2.0 B | ???????";

ASSERT_MATCHES(expected, perform_control_sequences(oss.str()));

// Add another already downloaded package
download_progress_bar2_raw->set_ticks(0);
download_progress_bar2_raw->set_total_ticks(0);
download_progress_bar2_raw->add_message(libdnf5::cli::progressbar::MessageType::SUCCESS, "Already Downloaded");
download_progress_bar2_raw->start();
download_progress_bar2_raw->set_state(libdnf5::cli::progressbar::ProgressBarState::SUCCESS);

oss << multi_progress_bar;
expected =
"\\[1/3\\] test 100% | ????? ??B\\/s | 0.0 B | ???????\n"
">>> Already Downloaded \n"
"\\[2/3\\] test 100% | ????? ??B\\/s | 0.0 B | ???????\n"
">>> Already Downloaded \n"
"----------------------------------------------------------------------\n"
"\\[2/3\\] Total ???% | ????? ??B\\/s | -1.0 B | ???????";
ASSERT_MATCHES(expected, perform_control_sequences(oss.str()));

// In progress download
download_progress_bar3_raw->set_ticks(4);
download_progress_bar3_raw->set_state(libdnf5::cli::progressbar::ProgressBarState::STARTED);

oss << multi_progress_bar;
expected =
"\\[1/3\\] test 100% | ????? ??B\\/s | 0.0 B | ???????\n"
">>> Already Downloaded \n"
"\\[2/3\\] test 100% | ????? ??B\\/s | 0.0 B | ???????\n"
">>> Already Downloaded \n"
"\\[3/3\\] test 40% | ????? ??B\\/s | 4.0 B | ???????\n"
"----------------------------------------------------------------------\n"
"\\[2/3\\] Total 40% | ????? ??B\\/s | 4.0 B | ???????";
ASSERT_MATCHES(expected, perform_control_sequences(oss.str()));
}
2 changes: 2 additions & 0 deletions test/libdnf5-cli/test_progressbar_interactive.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class ProgressbarInteractiveTest : public CppUnit::TestCase {
CPPUNIT_TEST(test_multi_progress_bar_with_messages);
CPPUNIT_TEST(test_multi_progress_bar_with_short_messages);
CPPUNIT_TEST(test_multi_progress_bars_with_messages);
CPPUNIT_TEST(test_multi_progress_bars_with_already_downloaded);

CPPUNIT_TEST_SUITE_END();

Expand All @@ -52,6 +53,7 @@ class ProgressbarInteractiveTest : public CppUnit::TestCase {
void test_multi_progress_bar_with_messages();
void test_multi_progress_bar_with_short_messages();
void test_multi_progress_bars_with_messages();
void test_multi_progress_bars_with_already_downloaded();
};


Expand Down
Loading