From a732f74400147a9993358b4778335715a0d5e20e Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 22 Nov 2024 17:59:48 +0000 Subject: Collect unprinted Ign errors for display in Err output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of deciding if the first or the last error in a retry chain is the best one to display if the entire chain fails in the end we teach our progress reporting to collect the ignored errors (if it hasn't shown them) and print them all together in the Err line. We de-duplicate the collection which makes it harder to match the Ign line with the error messages, but for these cases its probably better to enable direct display of the text anyhow – and repeating the same message multiple times is boring and probably not that helpful for a user wanting to know why all the downloads failed even they asked apt to retry 429 times… References: 841243df877b39dfcb907a7fab9c4e8b4f65d715 --- apt-private/acqprogress.cc | 58 +++++++++++++++------- apt-private/acqprogress.h | 3 ++ test/integration/test-bug-869859-retry-downloads | 31 +++++++++++- .../test-handle-redirect-as-used-mirror-change | 13 ++++- 4 files changed, 86 insertions(+), 19 deletions(-) diff --git a/apt-private/acqprogress.cc b/apt-private/acqprogress.cc index b4b16e6a9..b8b540e75 100644 --- a/apt-private/acqprogress.cc +++ b/apt-private/acqprogress.cc @@ -123,39 +123,63 @@ void AcqTextStatus::Fail(pkgAcquire::ItemDesc &Itm) AssignItemID(Itm); clearLastLine(); - bool ShowErrorText = true; + bool ShowErrorText = false; if (Itm.Owner->Status == pkgAcquire::Item::StatDone || Itm.Owner->Status == pkgAcquire::Item::StatIdle) { // TRANSLATOR: Very short word to be displayed for files in 'apt-get update' // which failed to download, but the error is ignored (compare "Err:") ioprintf(out, _("Ign:%lu %s"), Itm.Owner->ID, Itm.Description.c_str()); - if (Itm.Owner->ErrorText.empty() || - _config->FindB("Acquire::Progress::Ignore::ShowErrorText", false) == false) - ShowErrorText = false; + if (not Itm.Owner->ErrorText.empty()) + { + if (_config->FindB("Acquire::Progress::Ignore::ShowErrorText", false)) + ShowErrorText = true; + else + { + if (auto const ignored = IgnoredErrorTexts.find(Itm.Owner->ID); ignored != IgnoredErrorTexts.end()) + { + if (std::find(ignored->second.begin(), ignored->second.end(), Itm.Owner->ErrorText) == ignored->second.end()) + ignored->second.emplace_back(Itm.Owner->ErrorText); + } + else + IgnoredErrorTexts.emplace(Itm.Owner->ID, std::vector{Itm.Owner->ErrorText}); + } + } } else { // TRANSLATOR: Very short word to be displayed for files in 'apt-get update' // which failed to download and the error is critical (compare "Ign:") ioprintf(out, _("Err:%lu %s"), Itm.Owner->ID, Itm.Description.c_str()); + ShowErrorText = true; } - if (ShowErrorText) + if (ShowErrorText && not Itm.Owner->ErrorText.empty()) { - std::string::size_type line_start = 0; - std::string::size_type line_end; - while ((line_end = Itm.Owner->ErrorText.find_first_of("\n\r", line_start)) != std::string::npos) { - out << std::endl << " " << Itm.Owner->ErrorText.substr(line_start, line_end - line_start); - line_start = Itm.Owner->ErrorText.find_first_not_of("\n\r", line_end + 1); - if (line_start == std::string::npos) - break; + auto const printErrorText = [](std::ostream &out, std::string_view errortext) { + while (not errortext.empty()) + { + auto const line_end = errortext.find_first_of("\n\r"); + out << "\n " << errortext.substr(0, line_end); + if (line_end == std::string_view::npos) + break; + errortext.remove_prefix(line_end); + auto const line_start = errortext.find_first_not_of("\n\r"); + if (line_start == std::string_view::npos) + break; + errortext.remove_prefix(line_start); + } + }; + if (auto const ignored = IgnoredErrorTexts.find(Itm.Owner->ID); ignored != IgnoredErrorTexts.end()) + { + for (auto const &text : ignored->second) + printErrorText(out, text); + if (std::find(ignored->second.begin(), ignored->second.end(), Itm.Owner->ErrorText) == ignored->second.end()) + printErrorText(out, Itm.Owner->ErrorText); } - if (line_start == 0) - out << std::endl << " " << Itm.Owner->ErrorText; - else if (line_start != std::string::npos) - out << std::endl << " " << Itm.Owner->ErrorText.substr(line_start); + else + printErrorText(out, Itm.Owner->ErrorText); } - out << std::endl; + out << '\n' << std::flush; Update = true; } diff --git a/apt-private/acqprogress.h b/apt-private/acqprogress.h index 87b957e4b..b4015861c 100644 --- a/apt-private/acqprogress.h +++ b/apt-private/acqprogress.h @@ -14,6 +14,8 @@ #include #include +#include +#include class APT_PUBLIC AcqTextStatus : public pkgAcquireStatus { @@ -22,6 +24,7 @@ class APT_PUBLIC AcqTextStatus : public pkgAcquireStatus size_t LastLineLength; unsigned long ID; unsigned long Quiet; + std::unordered_map> IgnoredErrorTexts; APT_HIDDEN void clearLastLine(); APT_HIDDEN void AssignItemID(pkgAcquire::ItemDesc &Itm); diff --git a/test/integration/test-bug-869859-retry-downloads b/test/integration/test-bug-869859-retry-downloads index 333ce816e..821d7bc3e 100755 --- a/test/integration/test-bug-869859-retry-downloads +++ b/test/integration/test-bug-869859-retry-downloads @@ -48,7 +48,17 @@ Ign:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 429 Unknown HTTP code Err:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 429 Unknown HTTP code -E: Failed to fetch http://localhost:${APTHTTPPORT}/pool/testpkg_1_all.deb 429 Unknown HTTP code" apt download testpkg -o acquire::retries=3 -o debug::acquire::retries=true -q +E: Failed to fetch http://localhost:${APTHTTPPORT}/pool/testpkg_1_all.deb 429 Unknown HTTP code" apt download testpkg -o acquire::retries=3 -o debug::acquire::retries=true -q -o Acquire::Progress::Ignore::ShowErrorText=true +testfailure test -f testpkg_1_all.deb +testsecondsgreaterequal 5 testfailureequal "Delaying http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 by 1 seconds +Ign:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 +Delaying http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 by 2 seconds +Ign:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 +Delaying http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 by 4 seconds +Ign:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 +Err:1 http://localhost:${APTHTTPPORT} stable/main all testpkg all 1 + 429 Unknown HTTP code +E: Failed to fetch http://localhost:${APTHTTPPORT}/pool/testpkg_1_all.deb 429 Unknown HTTP code" apt download testpkg -o acquire::retries=3 -o debug::acquire::retries=true -q -o Acquire::Progress::Ignore::ShowErrorText=false testfailure test -f testpkg_1_all.deb msgmsg 'Retry 429 TooManyRequests after Retry-After time' @@ -66,6 +76,25 @@ testsuccess apt download testpkg -o acquire::retries=3 -o acquire::retries::dela testsuccess test -f testpkg_1_all.deb rm -f testpkg_1_all.deb +msgmsg 'Hard failure after retry request' +webserverconfig 'aptwebserver::failrequest::pool/unavailable_1_all.deb' '1' +testfailureequal "Ign:1 http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb + 429 Unknown HTTP code +Err:1 http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb + 404 Not Found +E: Failed to fetch http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb 404 Not Found +E: Download Failed" apthelper download-file "http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb" 'unavailable_1_all.deb' -o acquire::retries=2 -o acquire::retries::delay=false -o Acquire::Progress::Ignore::ShowErrorText=true +testfailure test -f unavailable_1_all.deb +webserverconfig 'aptwebserver::failrequest::pool/unavailable_1_all.deb' '1' +testfailureequal "Ign:1 http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb +Err:1 http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb + 429 Unknown HTTP code + 404 Not Found +E: Failed to fetch http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb 404 Not Found +E: Download Failed" apthelper download-file "http://localhost:${APTHTTPPORT}/pool/unavailable_1_all.deb" 'unavailable_1_all.deb' -o acquire::retries=2 -o acquire::retries::delay=false -o Acquire::Progress::Ignore::ShowErrorText=false +testfailure test -f unavailable_1_all.deb + + msgmsg 'Do not try everything again, hard failures keep hard failures' webserverconfig 'aptwebserver::failrequest' '404' webserverconfig 'aptwebserver::failrequest::pool/testpkg_1_all.deb' '2' diff --git a/test/integration/test-handle-redirect-as-used-mirror-change b/test/integration/test-handle-redirect-as-used-mirror-change index a6f8b788f..1ce1232e2 100755 --- a/test/integration/test-handle-redirect-as-used-mirror-change +++ b/test/integration/test-handle-redirect-as-used-mirror-change @@ -67,7 +67,18 @@ Get:3 http://0.0.0.0:${APTHTTPPORT} unstable/main all Packages [$(stat -c %s apt Get:4 http://0.0.0.0:${APTHTTPPORT} unstable/main Translation-en [$(stat -c %s aptarchive/dists/unstable/main/i18n/Translation-en.gz) B] Reading package lists... Building dependency tree... -All packages are up to date." apt update +All packages are up to date." apt update -o Acquire::Progress::Ignore::ShowErrorText=true +rm -rf rootdir/var/lib/apt/lists +testsuccessequal "Get:1 http://0.0.0.0:${APTHTTPPORT}/storage unstable InRelease [$(stat -c %s aptarchive/storage/dists/unstable/InRelease) B] +Ign:2 http://0.0.0.0:${APTHTTPPORT}/storage unstable/main Sources +Ign:3 http://0.0.0.0:${APTHTTPPORT}/storage unstable/main all Packages +Ign:4 http://0.0.0.0:${APTHTTPPORT}/storage unstable/main Translation-en +Get:2 http://0.0.0.0:${APTHTTPPORT} unstable/main Sources [$(stat -c %s aptarchive/dists/unstable/main/source/Sources.gz) B] +Get:3 http://0.0.0.0:${APTHTTPPORT} unstable/main all Packages [$(stat -c %s aptarchive/dists/unstable/main/binary-all/Packages.gz) B] +Get:4 http://0.0.0.0:${APTHTTPPORT} unstable/main Translation-en [$(stat -c %s aptarchive/dists/unstable/main/i18n/Translation-en.gz) B] +Reading package lists... +Building dependency tree... +All packages are up to date." apt update -o Acquire::Progress::Ignore::ShowErrorText=false find aptarchive -name 'InRelease' -delete rm -rf rootdir/var/lib/apt/lists -- cgit v1.2.3-70-g09d2