From 5fb0df6f63e02c141e8a0e5ad4543dea525df3fc Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Mon, 1 May 2017 11:02:36 +0200 Subject: Reimplement tar invocations to do manual decompression This is needed to prevent tar from forking, which doesn't work reliably on MSYS2. --- bpkg/archive | 6 +++-- bpkg/archive.cxx | 64 +++++++++++++++++++++++++++++++++++++++++++-------- bpkg/pkg-unpack.cxx | 56 +++++++++++++++++++++++++++++++++++++------- bpkg/pkg-verify.cxx | 31 +++++++++++++------------ tests/rep-create.test | 5 ++-- 5 files changed, 124 insertions(+), 38 deletions(-) diff --git a/bpkg/archive b/bpkg/archive index 918734c..1f33538 100644 --- a/bpkg/archive +++ b/bpkg/archive @@ -20,10 +20,12 @@ namespace bpkg package_dir (const path& archive); // Start the process of extracting the specified file from the archive. If - // error is false, then redirect STDERR to STDOUT (this can be used, for + // error is false, then redirect STDERR to /dev/null (this can be used, for // example, to suppress diagnostics). // - butl::process + // Return a pair of processes that form a pipe. Wait on the second first. + // + pair start_extract (const common_options&, const path& archive, const path& file, diff --git a/bpkg/archive.cxx b/bpkg/archive.cxx index 4e12e28..54ab8ea 100644 --- a/bpkg/archive.cxx +++ b/bpkg/archive.cxx @@ -26,7 +26,7 @@ namespace bpkg return path_cast (d); } - process + pair start_extract (const common_options& co, const path& a, const path& f, @@ -34,7 +34,30 @@ namespace bpkg { assert (!f.empty () && f.relative ()); - cstrings args {co.tar ().string ().c_str ()}; + cstrings args; + + // See if we need to decompress. + // + { + string e (a.extension ()); + + if (e == "gz") args.push_back ("gzip"); + else if (e == "bzip2") args.push_back ("bzip2"); + else if (e == "xz") args.push_back ("xz"); + else if (e != "tar") + fail << "unknown compression method in " << a; + } + + size_t i (0); // The tar command line start. + if (!args.empty ()) + { + args.push_back ("-dc"); + args.push_back (a.string ().c_str ()); + args.push_back (nullptr); + i = args.size (); + } + + args.push_back (co.tar ().string ().c_str ()); // Add extra options. // @@ -55,7 +78,7 @@ namespace bpkg #endif args.push_back ("-xf"); - args.push_back (a.string ().c_str ()); + args.push_back (i == 0 ? a.string ().c_str () : "-"); // MSYS tar doesn't find archived file if it's path is provided in Windows // notation. @@ -64,21 +87,42 @@ namespace bpkg args.push_back (fs.c_str ()); args.push_back (nullptr); + args.push_back (nullptr); // Pipe end. + size_t what; try { - process_path pp (process::path_search (args[0])); + process_path dpp; + process_path tpp; + + process dpr; + process tpr; + + if (i != 0) + dpp = process::path_search (args[what = 0]); + + tpp = process::path_search (args[what = i]); if (verb >= 2) print_process (args); // If err is false, then redirect STDERR to STDOUT. // - return process (pp, args.data (), 0, -1, (err ? 2 : 1)); + auto_fd nfd (err ? nullfd : fdnull ()); + + if (i != 0) + { + dpr = process (dpp, &args[what = 0], 0, -1, (err ? 2 : nfd.get ())); + tpr = process (tpp, &args[what = i], dpr, -1, (err ? 2 : nfd.get ())); + } + else + tpr = process (tpp, &args[what = 0], 0, -1, (err ? 2 : nfd.get ())); + + return make_pair (move (dpr), move (tpr)); } catch (const process_error& e) { - error << "unable to execute " << args[0] << ": " << e; + error << "unable to execute " << args[what] << ": " << e; if (e.child) exit (1); @@ -91,20 +135,20 @@ namespace bpkg extract (const common_options& o, const path& a, const path& f) try { - process pr (start_extract (o, a, f)); + pair pr (start_extract (o, a, f)); try { // Do not throw when eofbit is set (end of stream reached), and // when failbit is set (getline() failed to extract any character). // - ifdstream is (move (pr.in_ofd), ifdstream::badbit); + ifdstream is (move (pr.second.in_ofd), ifdstream::badbit); string s; getline (is, s, '\0'); is.close (); - if (pr.wait ()) + if (pr.second.wait () && pr.first.wait ()) return s; // Fall through. @@ -114,7 +158,7 @@ namespace bpkg // Child exit status doesn't matter. Just wait for the process // completion and fall through. // - pr.wait (); // Check throw. + pr.second.wait (); pr.first.wait (); // Check throw. } // While it is reasonable to assuming the child process issued diagnostics diff --git a/bpkg/pkg-unpack.cxx b/bpkg/pkg-unpack.cxx index d4a255c..9aead38 100644 --- a/bpkg/pkg-unpack.cxx +++ b/bpkg/pkg-unpack.cxx @@ -163,7 +163,30 @@ namespace bpkg // auto_rm_r arm (d); - cstrings args {co.tar ().string ().c_str ()}; + cstrings args; + + // See if we need to decompress. + // + { + string e (a.extension ()); + + if (e == "gz") args.push_back ("gzip"); + else if (e == "bzip2") args.push_back ("bzip2"); + else if (e == "xz") args.push_back ("xz"); + else if (e != "tar") + fail << "unknown compression method in package " << a; + } + + size_t i (0); // The tar command line start. + if (!args.empty ()) + { + args.push_back ("-dc"); + args.push_back (a.string ().c_str ()); + args.push_back (nullptr); + i = args.size (); + } + + args.push_back (co.tar ().string ().c_str ()); // Add extra options. // @@ -185,28 +208,45 @@ namespace bpkg #endif args.push_back ("-xf"); - args.push_back (a.string ().c_str ()); + args.push_back (i == 0 ? a.string ().c_str () : "-"); args.push_back (nullptr); + args.push_back (nullptr); // Pipe end. + size_t what; try { - process_path pp (process::path_search (args[0])); + process_path dpp; + process_path tpp; + + process dpr; + process tpr; + + if (i != 0) + dpp = process::path_search (args[what = 0]); + + tpp = process::path_search (args[what = i]); if (verb >= 2) print_process (args); - process pr (pp, args.data ()); + if (i != 0) + { + dpr = process (dpp, &args[what = 0], 0, -1); + tpr = process (tpp, &args[what = i], dpr); + } + else + tpr = process (tpp, &args[what = 0]); // While it is reasonable to assuming the child process issued - // diagnostics, tar, specifically, doesn't mention the archive - // name. + // diagnostics, tar, specifically, doesn't mention the archive name. // - if (!pr.wait ()) + if (!(what = i, tpr.wait ()) || + !(what = 0, dpr.wait ())) fail << "unable to extract package archive " << a; } catch (const process_error& e) { - error << "unable to execute " << args[0] << ": " << e; + error << "unable to execute " << args[what] << ": " << e; if (e.child) exit (1); diff --git a/bpkg/pkg-verify.cxx b/bpkg/pkg-verify.cxx index cd51d54..275c710 100644 --- a/bpkg/pkg-verify.cxx +++ b/bpkg/pkg-verify.cxx @@ -23,26 +23,27 @@ namespace bpkg dir_path pd (package_dir (af)); path mf (pd / path ("manifest")); - // If diag is false, we need to make tar not print any diagnostics. - // There doesn't seem to be an option to suppress this and the only - // way is to redirect STDERR to something like /dev/null. To keep - // things simple, we are going to redirect it to STDOUT, which we - // in turn redirect to a pipe and use to parse the manifest data. - // If things go badly for tar and it starts spitting errors instead - // of the manifest, the manifest parser will fail. But that's ok - // since we assume that the child error is always the reason for - // the manifest parsing failure. + // If diag is false, we need to make tar not print any diagnostics. There + // doesn't seem to be an option to suppress this and the only way is to + // redirect STDERR to something like /dev/null. // - process pr (start_extract (co, af, mf, diag)); + // If things go badly for tar and it starts spitting errors instead of the + // manifest, the manifest parser will fail. But that's ok since we assume + // that the child error is always the reason for the manifest parsing + // failure. + // + pair pr (start_extract (co, af, mf, diag)); + + auto wait = [&pr] () {return pr.second.wait () && pr.first.wait ();}; try { - ifdstream is (move (pr.in_ofd), fdstream_mode::skip); + ifdstream is (move (pr.second.in_ofd), fdstream_mode::skip); manifest_parser mp (is, mf.string ()); package_manifest m (mp, iu); is.close (); - if (pr.wait ()) + if (wait ()) { // Verify package archive/directory is -. // @@ -68,7 +69,7 @@ namespace bpkg // catch (const manifest_parsing& e) { - if (pr.wait ()) + if (wait ()) { if (diag) error (e.name, e.line, e.column) << e.description << @@ -79,7 +80,7 @@ namespace bpkg } catch (const io_error&) { - if (pr.wait ()) + if (wait ()) { if (diag) error << "unable to extract " << mf << " from " << af; @@ -91,7 +92,7 @@ namespace bpkg // We should only get here if the child exited with an error // status. // - assert (!pr.wait ()); + assert (!wait ()); // While it is reasonable to assuming the child process issued // diagnostics, tar, specifically, doesn't mention the archive diff --git a/tests/rep-create.test b/tests/rep-create.test index acc6957..01ab7f8 100644 --- a/tests/rep-create.test +++ b/tests/rep-create.test @@ -176,9 +176,8 @@ $clone_rep; touch stable/foo; - $* stable/ 2>>/~%EOE% != 0 - %.+ - error: stable/foo does not appear to be a bpkg package + $* stable/ 2>>/EOE != 0 + error: unknown compression method in stable/foo EOE } } -- cgit v1.1