From f40f28b12046cc993712956497dfb9d9baa452f9 Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Sat, 19 Jan 2019 21:50:43 +0300 Subject: Add support for --no-progress option --- bpkg/common.cli | 33 ++++++-- bpkg/fetch-git.cxx | 226 +++++++++++++++++++++++++++++++++----------------- bpkg/fetch.cxx | 71 ++++++++++++++-- bpkg/pkg-checkout.cxx | 4 +- bpkg/utility.txx | 16 +++- 5 files changed, 256 insertions(+), 94 deletions(-) (limited to 'bpkg') diff --git a/bpkg/common.cli b/bpkg/common.cli index 31580c0..4090ad3 100644 --- a/bpkg/common.cli +++ b/bpkg/common.cli @@ -100,6 +100,23 @@ namespace bpkg a command." } + // When it comes to external programs (such as curl, git, etc), if stderr + // is not a terminal, the logic is actually tri-state: With --no-progress + // we suppress any progress. With --progress (which we may add in the + // future), we request full progress. Finally, without any --*progress + // options we let the external program decide what to do: it may do + // something intelligent (like curl) and produce non-terminal-friendly + // progress (such as status lines printed periodically) or it may disable + // progress all together (like git). Of course, it may also do no + // detection and dump non-terminal-unfriendly progress in which case we + // should probably do the detection ourselves and suppress it. + // + bool --no-progress + { + "Suppress progress indicators for long-lasting operations, such as + network transfers, building, etc." + } + path --build { "", @@ -138,6 +155,14 @@ namespace bpkg \cb{wget}, and \cb{fetch}." } + strings --fetch-option + { + "", + "Additional option to be passed to the fetch program. See \cb{--fetch} + for more information on the fetch program. Repeat this option to + specify multiple fetch options." + } + size_t --fetch-timeout { "", @@ -157,14 +182,6 @@ namespace bpkg programs." } - strings --fetch-option - { - "", - "Additional option to be passed to the fetch program. See \cb{--fetch} - for more information on the fetch program. Repeat this option to - specify multiple fetch options." - } - path --git = "git" { "", diff --git a/bpkg/fetch-git.cxx b/bpkg/fetch-git.cxx index 1695bcf..ab3255c 100644 --- a/bpkg/fetch-git.cxx +++ b/bpkg/fetch-git.cxx @@ -35,8 +35,6 @@ namespace bpkg static const diag_noreturn_end endg; - using opt = optional; // Program option. - static strings timeout_opts (const common_options& co, repository_protocol proto) { @@ -209,19 +207,77 @@ namespace bpkg } } - // Run git process. + // Run git process, optionally suppressing progress. // template static process_exit - run_git (const common_options& co, A&&... args) + run_git (const common_options& co, bool progress, A&&... args) { + // Unfortunately git doesn't have any kind of a no-progress option. The + // only way to suppress progress is to run quiet (-q) which also + // suppresses some potentially useful information. However, git suppresses + // progress automatically if its stderr is not a terminal. So we use this + // feature for the progress suppression by redirecting git's stderr to our + // own diagnostics stream via a proxy pipe. + // + fdpipe pipe; + + if (!progress) + pipe = open_pipe (); + process pr (start_git (co, - 1 /* stdout */, 2 /* stderr */, + 1 /* stdout */, + !progress ? pipe.out.get () : 2 /* stderr */, forward (args)...)); + + if (!progress) + { + // Shouldn't throw, unless something is severely damaged. + // + pipe.out.close (); + + try + { + ifdstream is (move (pipe.in), fdstream_mode::skip, ifdstream::badbit); + + // We could probably write something like this, instead: + // + // *diag_stream << is.rdbuf () << flush; + // + // However, it would never throw and we could potentially miss the + // reading failure, unless we decide to additionally mess with the + // diagnostics stream exception mask. + // + for (string l; !eof (getline (is, l)); ) + *diag_stream << l << endl; + + is.close (); + + // Fall through. + } + catch (const io_error& e) + { + // Fail if git exited normally with zero code, so the issue won't go + // unnoticed. Otherwise, let the caller handle git's failure. + // + if (pr.wait ()) + fail << "unable to read git diagnostics: " << e; + + // Fall through. + } + } + pr.wait (); return *pr.exit; } + template + static process_exit + run_git (const common_options& co, A&&... args) + { + return run_git (co, true /* progress */, forward (args)...); + } + // Run git process and return it's output as a string. Fail if the output // doesn't contain a single line. // @@ -695,12 +751,15 @@ namespace bpkg if (i != repository_refs.end ()) return i->second; - if (verb) + if (verb && !co.no_progress ()) text << "querying " << url; refs rs; fdpipe pipe (open_pipe ()); + // Note: ls-remote doesn't print anything to stderr, so no progress + // suppression is required. + // process pr (start_git (co, pipe, 2 /* stderr */, timeout_opts (co, url.scheme), @@ -823,7 +882,7 @@ namespace bpkg ? strings ({"--separate-git-dir=" + git_dir.string ()}) : strings (), - verb < 2 ? opt ("-q") : nullopt, + verb < 2 ? "-q" : nullptr, dir)) fail << "unable to init " << dir << endg; @@ -1262,27 +1321,44 @@ namespace bpkg } }; - // Note that we suppress the (too detailed) fetch command output if the - // verbosity level is 1. However, we still want to see the progress in - // this case, unless stderr is not directed to a terminal. + // Map verbosity level. Suppress the (too detailed) fetch command output + // if the verbosity level is 1. However, we still want to see the + // progress in this case, unless we were asked to suppress it (git also + // suppress progress for a non-terminal stderr). // + cstrings vos; + bool progress (!co.no_progress ()); + + if (verb < 2) + { + vos.push_back ("-q"); + + if (progress) + { + if (verb == 1 && stderr_term) + vos.push_back ("--progress"); + } + else + progress = true; // Already suppressed with -q. + } + else if (verb > 3) + vos.push_back ("-v"); + // Also note that we don't need to specify --refmap option since we can // rely on the init() function that properly sets the // remote.origin.fetch configuration option. // if (!run_git (co, + progress, timeout_opts (co, url ().scheme), co.git_option (), "-C", dir, "fetch", "--no-recurse-submodules", - - shallow ? cstrings ({"--depth", "1"}) : - shallow_repo () ? cstrings ({"--unshallow"}) : - cstrings (), - - verb == 1 && fdterm (2) ? opt ("--progress") : nullopt, - verb < 2 ? opt ("-q") : verb > 3 ? opt ("-v") : nullopt, + (shallow ? cstrings ({"--depth", "1"}) : + shallow_repo () ? cstrings ({"--unshallow"}) : + cstrings ()), + vos, "origin", remapped_refspecs ? *remapped_refspecs : refspecs)) fail << "unable to fetch " << dir << endg; @@ -1294,72 +1370,74 @@ namespace bpkg fail << "unable to test if " << dir << " is shallow" << endg; }; - // Print the progress indicator. - // - // Note that the clone command prints the following line prior to the - // progress lines: - // - // Cloning into ''... - // - // The fetch command doesn't print anything similar, for some reason. - // This makes it hard to understand which superproject/submodule is - // currently being fetched. Let's fix that. + // Print progress. // - // Also note that we have "fixed" that capital letter nonsense and stripped - // the trailing '...'. - // - if (verb) + if (verb && !co.no_progress ()) { - diag_record dr (text); - dr << "fetching "; + // Note that the clone command prints the following line prior to the + // progress lines: + // + // Cloning into ''... + // + // The fetch command doesn't print anything similar, for some reason. + // This makes it hard to understand which superproject/submodule is + // currently being fetched. Let's fix that. + // + // Also note that we have "fixed" that capital letter nonsense and + // stripped the trailing '...'. + // + { + diag_record dr (text); + dr << "fetching "; - if (!submodule.empty ()) - dr << "submodule '" << submodule.posix_string () << "' "; + if (!submodule.empty ()) + dr << "submodule '" << submodule.posix_string () << "' "; - dr << "from " << url (); + dr << "from " << url (); - if (verb >= 2) - dr << " in '" << dir.posix_string () << "'"; // Is used by tests. - } + if (verb >= 2) + dr << " in '" << dir.posix_string () << "'"; // Is used by tests. + } - // First, we perform the deep fetching. - // - if (fetch_repo || !dcs.empty ()) - { // Print warnings prior to the deep fetching. // + if (fetch_repo || !dcs.empty ()) { - diag_record dr (warn); - dr << "fetching whole " << (fetch_repo ? "repository" : "reference") - << " history"; - - if (!submodule.empty ()) - dr << " for submodule '" << submodule.posix_string () << "'"; + { + diag_record dr (warn); + dr << "fetching whole " << (fetch_repo ? "repository" : "reference") + << " history"; + + if (!submodule.empty ()) + dr << " for submodule '" << submodule.posix_string () << "'"; + + dr << " (" + << (caps () == capabilities::dumb + ? "dumb HTTP" + : "unadvertised commit") // There are no other reasons so far. + << ')'; + } - dr << " (" - << (caps () == capabilities::dumb - ? "dumb HTTP" - : "unadvertised commit") // There are no other reasons so far. - << ')'; + if (caps () == capabilities::dumb) + warn << "no progress will be shown (dumb HTTP)"; } + } - if (caps () == capabilities::dumb) - warn << "no progress will be shown (dumb HTTP)"; - - // Fetch. - // - fetch (fetch_repo ? strings () : dcs, false); + // Fetch. + // + // First, we perform the deep fetching. + // + fetch (fetch_repo ? strings () : dcs, false); - // After the deep fetching some of the shallow commits might also be - // fetched, so we drop them from the fetch list. - // - for (auto i (scs.begin ()); i != scs.end (); ) - { - if (commit_fetched (co, dir, *i)) - i = scs.erase (i); - else - ++i; - } + // After the deep fetching some of the shallow commits might also be + // fetched, so we drop them from the fetch list. + // + for (auto i (scs.begin ()); i != scs.end (); ) + { + if (commit_fetched (co, dir, *i)) + i = scs.erase (i); + else + ++i; } // Finally, we perform the shallow fetching. @@ -1425,7 +1503,7 @@ namespace bpkg : strings (), "submodule--helper", "init", - verb < 2 ? opt ("-q") : nullopt)) + verb < 2 ? "-q" : nullptr)) failure ("unable to initialize submodules"); repository_url orig_url (origin_url (co, dir)); @@ -1589,7 +1667,7 @@ namespace bpkg // Let's make the message match the git-submodule script output // (again, except for capitalization). // - if (verb) + if (verb && !co.no_progress ()) text << "submodule path '" << psd << "': checked out '" << commit << "'"; @@ -1716,7 +1794,7 @@ namespace bpkg "-C", dir, "reset", "--hard", - verb < 2 ? opt ("-q") : nullopt, + verb < 2 ? "-q" : nullptr, commit)); if (!pr.wait ()) fail << "unable to reset to " << commit << endg; @@ -1729,7 +1807,7 @@ namespace bpkg "-d", "-x", "-ff", - verb < 2 ? opt ("-q") : nullopt)) + verb < 2 ? "-q" : nullptr)) fail << "unable to clean " << dir << endg; } diff --git a/bpkg/fetch.cxx b/bpkg/fetch.cxx index 1aaff37..aaabffe 100644 --- a/bpkg/fetch.cxx +++ b/bpkg/fetch.cxx @@ -91,6 +91,7 @@ namespace bpkg static process start_wget (const path& prog, const optional& timeout, + bool no_progress, const strings& ops, const string& url, const path& out) @@ -119,7 +120,10 @@ namespace bpkg // user re-runs the command with -v to see all the gory details. // if (verb < (fo ? 1 : 2)) + { args.push_back ("-q"); + no_progress = false; // Already suppressed with -q. + } else if (fo && verb == 1) { // Wget 1.16 introduced the --show-progress option which in the @@ -129,12 +133,24 @@ namespace bpkg if (wget_major > 1 || (wget_major == 1 && wget_minor >= 16)) { args.push_back ("-q"); - args.push_back ("--show-progress"); + + if (!no_progress) + args.push_back ("--show-progress"); + else + no_progress = false; // Already suppressed with -q. } } else if (verb > 3) args.push_back ("-d"); + // Suppress progress. + // + // Note: the `--no-verbose -d` options combination is valid and results in + // debug messages with the progress meter suppressed. + // + if (no_progress) + args.push_back ("--no-verbose"); + // Set download timeout if requested. // string tm; @@ -224,6 +240,7 @@ namespace bpkg static process start_curl (const path& prog, const optional& timeout, + bool no_progress, const strings& ops, const string& url, const path& out) @@ -237,6 +254,12 @@ namespace bpkg "-A", (BPKG_USER_AGENT " curl") }; + auto suppress_progress = [&args] () + { + args.push_back ("-s"); + args.push_back ("-S"); // But show errors. + }; + // Map verbosity level. If we are running quiet or at level 1 // and the output is stdout, then run curl quiet. If at level // 1 and the output is a file, then show the progress bar. At @@ -246,14 +269,25 @@ namespace bpkg // if (verb < (fo ? 1 : 2)) { - args.push_back ("-s"); - args.push_back ("-S"); // But show errors. + suppress_progress (); + no_progress = false; // Already suppressed. } else if (fo && verb == 1) - args.push_back ("--progress-bar"); + { + if (!no_progress) + args.push_back ("--progress-bar"); + } else if (verb > 3) args.push_back ("-v"); + // Suppress progress. + // + // Note: the `-v -s` options combination is valid and results in a verbose + // output without progress. + // + if (no_progress) + suppress_progress (); + // Set download timeout if requested. // string tm; @@ -285,7 +319,7 @@ namespace bpkg if (verb >= 2) print_process (args); - else if (verb == 1 && fo) + else if (verb == 1 && fo && !no_progress) // // Unfortunately curl doesn't print the filename being fetched // next to the progress bar. So the best we can do is print it @@ -350,6 +384,7 @@ namespace bpkg static process start_fetch (const path& prog, const optional& timeout, + bool no_progress, const strings& ops, const string& url, const path& out) @@ -369,10 +404,28 @@ namespace bpkg // level 2 or 3, then run it at the default level (so it will display // the progress). Higher than that -- run it verbose. // + // Note that the only way to suppress progress for the fetch program is to + // run it quiet (-q). However, it prints nothing but the progress by + // default and some additional information in the verbose mode (-v). + // Therefore, if the progress suppression is requested we will run quiet + // unless the verbosity level is greater than three, in which case we will + // run verbose (and with progress). That's the best we can do. + // if (verb < (fo ? 1 : 2)) + { args.push_back ("-q"); + no_progress = false; // Already suppressed with -q. + } else if (verb > 3) + { args.push_back ("-v"); + no_progress = false; // Don't be quiet in the verbose mode (see above). + } + + // Suppress progress. + // + if (no_progress) + args.push_back ("-q"); // Set download timeout if requested. // @@ -417,8 +470,8 @@ namespace bpkg // The dispatcher. // - // Cache the result of finding/testing the fetch program. Sometimes - // a simple global variable is really the right solution... + // Cache the result of finding/testing the fetch program. Sometimes a simple + // global variable is really the right solution... // enum kind {wget, curl, fetch}; @@ -509,6 +562,7 @@ namespace bpkg { process (*f) (const path&, const optional&, + bool, const strings&, const string&, const path&) = nullptr; @@ -526,7 +580,8 @@ namespace bpkg try { - return f (fetch_path, timeout, o.fetch_option (), url, out); + return f ( + fetch_path, timeout, o.no_progress (), o.fetch_option (), url, out); } catch (const process_error& e) { diff --git a/bpkg/pkg-checkout.cxx b/bpkg/pkg-checkout.cxx index 6e0f92a..a91cb92 100644 --- a/bpkg/pkg-checkout.cxx +++ b/bpkg/pkg-checkout.cxx @@ -42,7 +42,7 @@ namespace bpkg // Print the progress indicator to attribute the possible fetching // progress. // - if (verb) + if (verb && !o.no_progress ()) text << "checking out " << package_string (ap->id.name, ap->version); @@ -194,7 +194,7 @@ namespace bpkg // At verbosity level 1 we want our (nicer) progress header but the // build system's actual progress. // - if (verb == 1) + if (verb == 1 && !o.no_progress ()) text << "distributing " << n << '/' << v; run_b (o, diff --git a/bpkg/utility.txx b/bpkg/utility.txx index 93699ab..71701bc 100644 --- a/bpkg/utility.txx +++ b/bpkg/utility.txx @@ -36,17 +36,26 @@ namespace bpkg // as us. // string vl; + bool no_progress (co.no_progress ()); if (verb == 0) + { ops.push_back ("-q"); + no_progress = false; // Already suppressed with -q. + } else if (verb == 1) { if (v != verb_b::normal) { ops.push_back ("-q"); - if (v == verb_b::progress && stderr_term) - ops.push_back ("--progress"); + if (!no_progress) + { + if (v == verb_b::progress && stderr_term) + ops.push_back ("--progress"); + } + else + no_progress = false; // Already suppressed with -q. } } else if (verb == 2) @@ -58,6 +67,9 @@ namespace bpkg ops.push_back (vl.c_str ()); } + if (no_progress) + ops.push_back ("--no-progress"); + return process_start_callback ( [] (const char* const args[], size_t n) { -- cgit v1.1