Verify package manifest compatibility with current toolchain

8 files changed, 280 insertions, 112 deletions

diff --git a/bpkg/pkg-build.cxx b/bpkg/pkg-build.cxx
index dd48440..29dd1c9 100644
--- a/bpkg/pkg-build.cxx
+++ b/bpkg/pkg-build.cxx
@@ -391,7 +391,8 @@ namespace bpkg
                     a->absolute () ? *a : db.config_orig / *a,
                     true /* ignore_unknown */,
                     false /* expand_values */)
-      : pkg_verify (sp->effective_src_root (db.config_orig),
+      : pkg_verify (options,
+                    sp->effective_src_root (db.config_orig),
                     true /* ignore_unknown */,
                     // Copy potentially fixed up version from selected package.
                     [&sp] (version& v) {v = sp->version;}));
@@ -1128,15 +1129,19 @@ namespace bpkg
           //
           if (dn == "build2")
           {
-            if (dp.constraint)
-              satisfy_build2 (options, name, dp);
+            if (dp.constraint && !satisfy_build2 (options, dp))
+              fail << "unable to satisfy constraint (" << dp
+                   << ") for package " << name <<
+                info << "available build2 version is " << build2_version;
 
             continue;
           }
           else if (dn == "bpkg")
           {
-            if (dp.constraint)
-              satisfy_bpkg (options, name, dp);
+            if (dp.constraint && !satisfy_bpkg (options, dp))
+              fail << "unable to satisfy constraint (" << dp
+                   << ") for package " << name <<
+                info << "available bpkg version is " << bpkg_version;
 
             continue;
           }
@@ -4592,10 +4597,6 @@ namespace bpkg
 
           const char* package (pa.value.c_str ());
 
-          // Is this a package archive?
-          //
-          bool package_arc (false);
-
           try
           {
             path a (package);
@@ -4611,14 +4612,10 @@ namespace bpkg
                             true /* ignore_unknown */,
                             false /* expand_values */,
                             true /* complete_depends */,
-                            diag));
+                            diag ? 2 : 1));
 
               // This is a package archive.
               //
-              // Note that throwing failed from here on will be fatal.
-              //
-              package_arc = true;
-
               l4 ([&]{trace << "archive '" << a << "': " << arg_string (pa);});
 
               // Supporting this would complicate things a bit, but we may add
@@ -4644,15 +4641,8 @@ namespace bpkg
           {
             // Not a valid path so cannot be an archive.
           }
-          catch (const failed& e)
+          catch (const not_package&)
           {
-            // If this is a valid package archive but something went wrong
-            // afterwards, then we are done.
-            //
-            if (package_arc)
-              throw;
-
-            assert (e.code == 1);
           }
 
           // Is this a package directory?
@@ -4666,8 +4656,6 @@ namespace bpkg
           size_t pn (strlen (package));
           if (pn != 0 && path::traits_type::is_separator (package[pn - 1]))
           {
-            bool package_dir (false);
-
             try
             {
               dir_path d (package);
@@ -4684,6 +4672,7 @@ namespace bpkg
 
                 package_manifest m (
                   pkg_verify (
+                    o,
                     d,
                     true /* ignore_unknown */,
                     [&o, &d, &pvi] (version& v)
@@ -4693,14 +4682,10 @@ namespace bpkg
                       if (pvi.version)
                         v = move (*pvi.version);
                     },
-                    diag));
+                    diag ? 2 : 1));
 
                 // This is a package directory.
                 //
-                // Note that throwing failed from here on will be fatal.
-                //
-                package_dir = true;
-
                 l4 ([&]{trace << "directory '" << d << "': "
                               << arg_string (pa);});
 
@@ -4741,15 +4726,8 @@ namespace bpkg
             {
               // Not a valid path so cannot be a package directory.
             }
-            catch (const failed& e)
+            catch (const not_package&)
             {
-              // If this is a valid package directory but something went wrong
-              // afterwards, then we are done.
-              //
-              if (package_dir)
-                throw;
-
-              assert (e.code == 1);
             }
           }
         }
@@ -7261,7 +7239,8 @@ namespace bpkg
         assert (sp->state == package_state::unpacked);
 
         package_manifest m (
-          pkg_verify (sp->effective_src_root (pdb.config_orig),
+          pkg_verify (o,
+                      sp->effective_src_root (pdb.config_orig),
                       true /* ignore_unknown */,
                       [&sp] (version& v) {v = sp->version;}));
 
diff --git a/bpkg/pkg-configure.cxx b/bpkg/pkg-configure.cxx
index 393ed68..65718ca 100644
--- a/bpkg/pkg-configure.cxx
+++ b/bpkg/pkg-configure.cxx
@@ -43,16 +43,20 @@ namespace bpkg
           //
           if (n == "build2")
           {
-            if (d.constraint)
-              satisfy_build2 (o, package, d);
+            if (d.constraint && !satisfy_build2 (o, d))
+              fail << "unable to satisfy constraint (" << d
+                   << ") for package " << package <<
+                info << "available build2 version is " << build2_version;
 
             satisfied = true;
             break;
           }
           else if (n == "bpkg")
           {
-            if (d.constraint)
-              satisfy_bpkg (o, package, d);
+            if (d.constraint && !satisfy_bpkg (o, d))
+              fail << "unable to satisfy constraint (" << d
+                   << ") for package " << package <<
+                info << "available bpkg version is " << bpkg_version;
 
             satisfied = true;
             break;
@@ -371,7 +375,8 @@ namespace bpkg
 
       l4 ([&]{trace << *p;});
 
-      package_manifest m (pkg_verify (p->effective_src_root (c),
+      package_manifest m (pkg_verify (o,
+                                      p->effective_src_root (c),
                                       true /* ignore_unknown */,
                                       [&p] (version& v) {v = p->version;}));
 
diff --git a/bpkg/pkg-unpack.cxx b/bpkg/pkg-unpack.cxx
index 862feac..0087086 100644
--- a/bpkg/pkg-unpack.cxx
+++ b/bpkg/pkg-unpack.cxx
@@ -169,7 +169,8 @@ namespace bpkg
     // Verify the directory is a package and get its manifest.
     //
     package_manifest m (
-      pkg_verify (d,
+      pkg_verify (o,
+                  d,
                   true /* ignore_unknown */,
                   [&o, &d, &pvi] (version& v)
                   {
diff --git a/bpkg/pkg-verify.cxx b/bpkg/pkg-verify.cxx
index aae1b43..4cfaa85 100644
--- a/bpkg/pkg-verify.cxx
+++ b/bpkg/pkg-verify.cxx
@@ -10,6 +10,7 @@
 
 #include <bpkg/archive.hxx>
 #include <bpkg/diagnostics.hxx>
+#include <bpkg/satisfaction.hxx>
 #include <bpkg/manifest-utility.hxx>
 
 using namespace std;
@@ -17,28 +18,144 @@ using namespace butl;
 
 namespace bpkg
 {
+  vector<manifest_name_value>
+  pkg_verify (const common_options& co,
+              manifest_parser& p,
+              const path& what,
+              int diag_level)
+  {
+    manifest_name_value nv (p.next ());
+
+    // Make sure this is the start and we support the version.
+    //
+    if (!nv.name.empty ())
+      throw manifest_parsing (p.name (), nv.name_line, nv.name_column,
+                              "start of package manifest expected");
+
+    if (nv.value != "1")
+      throw manifest_parsing (p.name (), nv.value_line, nv.value_column,
+                              "unsupported format version");
+
+    vector<manifest_name_value> r;
+
+    // For the depends name, parse the value and if it contains the build2 or
+    // bpkg constraints, verify that they are satisfied.
+    //
+    // Note that if the semantics of the depends value changes we may be
+    // unable to parse some of them before we get to build2 or bpkg and issue
+    // the user-friendly diagnostics. So we are going to ignore such depends
+    // values. But that means that if the user made a mistake in build2/bpkg
+    // then we will skip them as well. This, however, is not a problem since
+    // the pre-parsed result will then be re-parsed (e.g., by the
+    // package_manifest() constructor) which will diagnose any mistakes.
+    //
+    for (nv = p.next (); !nv.empty (); nv = p.next ())
+    {
+      if (nv.name == "depends")
+      try
+      {
+        dependency_alternatives da (nv.value);
+
+        for (dependency& d: da)
+        {
+          const package_name& dn (d.name);
+
+          if (dn != "build2" && dn != "bpkg")
+            continue;
+
+          if (!da.buildtime)
+          {
+            if (diag_level != 0)
+              error (p.name (), nv.value_line, nv.value_column)
+                << dn << " dependency must be build-time";
+
+            throw failed ();
+          }
+
+          if (da.size () != 1)
+          {
+            if (diag_level != 0)
+              error (p.name (), nv.value_line, nv.value_column)
+                << "alternatives in " << dn << " dependency";
+
+            throw failed ();
+          }
+
+          if (dn == "build2")
+          {
+            if (d.constraint && !satisfy_build2 (co, d))
+            {
+              if (diag_level != 0)
+              {
+                diag_record dr (error);
+                dr << "unable to satisfy constraint (" << d << ")";
+
+                if (!what.empty ())
+                  dr << " for package " << what;
+
+                dr << info << "available build2 version is " << build2_version;
+              }
+
+              throw failed ();
+            }
+          }
+          else
+          {
+            if (d.constraint && !satisfy_bpkg (co, d))
+            {
+              if (diag_level != 0)
+              {
+                diag_record dr (error);
+                dr << "unable to satisfy constraint (" << d << ")";
+
+                if (!what.empty ())
+                  dr << " for package " << what;
+
+                dr << "available bpkg version is " << bpkg_version;
+              }
+
+              throw failed ();
+            }
+          }
+        }
+      }
+      catch (const invalid_argument&) {} // Ignore
+
+      r.push_back (move (nv));
+    }
+
+    // Make sure this is the end.
+    //
+    nv = p.next ();
+    if (!nv.empty ())
+      throw manifest_parsing (p.name (), nv.name_line, nv.name_column,
+                              "single package manifest expected");
+
+    return r;
+  }
+
   package_manifest
   pkg_verify (const common_options& co,
               const path& af,
               bool iu,
               bool ev,
               bool cd,
-              bool diag)
+              int diag_level)
   try
   {
     dir_path pd (package_dir (af));
     path mf (pd / manifest_file);
 
-    // 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.
+    // If the diag level is less than 2, 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.
     //
     // 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<process, process> pr (start_extract (co, af, mf, diag));
+    pair<process, process> pr (start_extract (co, af, mf, diag_level == 2));
 
     auto wait = [&pr] () {return pr.second.wait () && pr.first.wait ();};
 
@@ -46,7 +163,11 @@ namespace bpkg
     {
       ifdstream is (move (pr.second.in_ofd), fdstream_mode::skip);
       manifest_parser mp (is, mf.string ());
-      package_manifest m (mp, iu, cd);
+
+      package_manifest m (mp.name (),
+                          pkg_verify (co, mp, af, diag_level),
+                          iu,
+                          cd);
       is.close ();
 
       if (wait ())
@@ -57,7 +178,7 @@ namespace bpkg
 
         if (pd != ed)
         {
-          if (diag)
+          if (diag_level != 0)
             error << "package archive/directory name mismatch in " << af <<
               info << "extracted from archive '" << pd << "'" <<
               info << "expected from manifest '" << ed << "'";
@@ -70,14 +191,14 @@ namespace bpkg
         if (ev)
         {
           m.load_files (
-            [&pd, &co, &af, diag] (const string& n, const path& p)
+            [&pd, &co, &af, diag_level] (const string& n, const path& p)
             {
               path f (pd / p);
-              string s (extract (co, af, f, diag));
+              string s (extract (co, af, f, diag_level != 0));
 
               if (s.empty ())
               {
-                if (diag)
+                if (diag_level != 0)
                   error << n << " manifest value in package archive "
                         << af << " references empty file " << f;
 
@@ -101,7 +222,7 @@ namespace bpkg
     {
       if (wait ())
       {
-        if (diag)
+        if (diag_level != 0)
           error (e.name, e.line, e.column) << e.description <<
             info << "package archive " << af;
 
@@ -112,7 +233,7 @@ namespace bpkg
     {
       if (wait ())
       {
-        if (diag)
+        if (diag_level != 0)
           error << "unable to extract " << mf << " from " << af;
 
         throw failed ();
@@ -128,10 +249,10 @@ namespace bpkg
     // diagnostics, tar, specifically, doesn't mention the archive
     // name.
     //
-    if (diag)
+    if (diag_level == 2)
       error << af << " does not appear to be a bpkg package";
 
-    throw failed ();
+    throw not_package ();
   }
   catch (const process_error& e)
   {
@@ -142,10 +263,11 @@ namespace bpkg
   }
 
   package_manifest
-  pkg_verify (const dir_path& d,
+  pkg_verify (const common_options& co,
+              const dir_path& d,
               bool iu,
               const function<package_manifest::translate_function>& tf,
-              bool diag)
+              int diag_level)
   {
     // Parse the manifest.
     //
@@ -153,17 +275,21 @@ namespace bpkg
 
     if (!exists (mf))
     {
-      if (diag)
+      if (diag_level == 2)
         error << "no manifest file in package directory " << d;
 
-      throw failed ();
+      throw not_package ();
     }
 
     try
     {
       ifdstream ifs (mf);
       manifest_parser mp (ifs, mf.string ());
-      package_manifest m (mp, tf, iu);
+
+      package_manifest m (mp.name (),
+                          pkg_verify (co, mp, d, diag_level),
+                          tf,
+                          iu);
 
       // We used to verify package directory is <name>-<version> but it is
       // not clear why we should enforce it in this case (i.e., the user
@@ -173,25 +299,25 @@ namespace bpkg
       //
       // if (d.leaf () != ed)
       // {
-      //  if (diag)
-      //    error << "invalid package directory name '" << d.leaf () << "'" <<
-      //      info << "expected from manifest '" << ed << "'";
+      //   if (diag_level != 0)
+      //     error << "invalid package directory name '" << d.leaf () << "'" <<
+      //       info << "expected from manifest '" << ed << "'";
       //
-      //  throw failed ();
+      //   throw failed ();
       // }
 
       return m;
     }
     catch (const manifest_parsing& e)
     {
-      if (diag)
+      if (diag_level != 0)
         error (e.name, e.line, e.column) << e.description;
 
       throw failed ();
     }
     catch (const io_error& e)
     {
-      if (diag)
+      if (diag_level != 0)
         error << "unable to read from " << mf << ": " << e;
 
       throw failed ();
@@ -224,7 +350,7 @@ namespace bpkg
                                       o.ignore_unknown (),
                                       o.deep () /* expand_values */,
                                       o.deep () /* complete_depends */,
-                                      !o.silent ()));
+                                      o.silent () ? 0 : 2));
 
       if (o.manifest ())
       {
diff --git a/bpkg/pkg-verify.hxx b/bpkg/pkg-verify.hxx
index 5643692..ac231ec 100644
--- a/bpkg/pkg-verify.hxx
+++ b/bpkg/pkg-verify.hxx
@@ -4,6 +4,8 @@
 #ifndef BPKG_PKG_VERIFY_HXX
 #define BPKG_PKG_VERIFY_HXX
 
+#include <libbutl/manifest-forward.hxx>
+
 #include <libbpkg/manifest.hxx>
 
 #include <bpkg/types.hxx>
@@ -20,17 +22,27 @@ namespace bpkg
   // expand the file-referencing manifest values (description, changes, etc),
   // setting them to the contents of files they refer to, set the potentially
   // absent description-type value to the effective description type (see
-  // libbpkg/manifest.hxx), and complete the dependency constraints. Throw
-  // failed if invalid or if something goes wrong. If diag is false, then
-  // don't issue diagnostics about the reason why the package is invalid.
+  // libbpkg/manifest.hxx), and complete the dependency constraints.
+  //
+  // Throw not_package (derived from failed) if this doesn't look like a
+  // package. Throw plain failed if this does looks like a package but
+  // something about it is invalid or if something else goes wrong.
+  //
+  // Issue diagnostics according the diag_level as follows:
   //
+  // 0 - Suppress all errors messages except for underlying system errors.
+  // 1 - Suppress error messages about the reason why this is not a package.
+  // 2 - Suppress no error messages.
+  //
+  class not_package: public failed {};
+
   package_manifest
   pkg_verify (const common_options&,
               const path& archive,
               bool ignore_unknown,
               bool expand_values,
               bool complete_depends = true,
-              bool diag = true);
+              int diag_level = 2);
 
   // Similar to the above but verifies that a source directory is a valid
   // package. Always translates the package version and completes dependency
@@ -39,10 +51,28 @@ namespace bpkg
   // itself.
   //
   package_manifest
-  pkg_verify (const dir_path& source,
+  pkg_verify (const common_options&,
+              const dir_path& source,
               bool ignore_unknown,
               const function<package_manifest::translate_function>&,
-              bool diag = true);
+              int diag_level = 2);
+
+  // Pre-parse the package manifest and return the name value pairs list,
+  // stripping the format version and the end-of-manifest/stream pairs. Also
+  // verify that the package is compatible with the current toolchain and
+  // issue diagnostics and throw failed if it is not. Pass through the
+  // manifest_parsing and io_error exceptions, so that the caller can decide
+  // how to handle them (for example, ignore them if the manifest-printing
+  // process has failed, etc).
+  //
+  // To omit the package location from the diagnostics, pass an empty path as
+  // the what argument.
+  //
+  vector<butl::manifest_name_value>
+  pkg_verify (const common_options&,
+              butl::manifest_parser&,
+              const path& what,
+              int diag_level = 2);
 }
 
 #endif // BPKG_PKG_VERIFY_HXX
diff --git a/bpkg/rep-fetch.cxx b/bpkg/rep-fetch.cxx
index cefa799..4d1a670 100644
--- a/bpkg/rep-fetch.cxx
+++ b/bpkg/rep-fetch.cxx
@@ -15,6 +15,7 @@
 #include <bpkg/package-odb.hxx>
 #include <bpkg/database.hxx>
 #include <bpkg/rep-remove.hxx>
+#include <bpkg/pkg-verify.hxx>
 #include <bpkg/diagnostics.hxx>
 #include <bpkg/manifest-utility.hxx>
 
@@ -219,10 +220,20 @@ namespace bpkg
     // files and retrieve the package versions via the single `b info` call.
     // While at it cache the manifest paths for the future use.
     //
-    paths                 mfs;
-    package_version_infos pvs;
+    // Note that if the package is not compatible with the toolchain, not to
+    // end up with an unfriendly build2 error message (referring a line in the
+    // bootstrap file issued by the version module), we need to verify the
+    // compatibility of the package manifests prior to calling `b info`. Also
+    // note that we cannot create the package manifest objects at this stage,
+    // since we need the package versions for that. Thus, we cache the
+    // respective name value lists instead.
+    //
+    package_version_infos               pvs;
+    paths                               mfs;
+    vector<vector<manifest_name_value>> nvs;
     {
       mfs.reserve (pms.size ());
+      nvs.reserve (pms.size ());
 
       dir_paths pds;
       pds.reserve (pms.size ());
@@ -242,6 +253,38 @@ namespace bpkg
           add_package_info (pm, dr);
         }
 
+        // Provide the context if the package compatibility verification fails.
+        //
+        auto g (
+          make_exception_guard (
+            [&pm, &add_package_info] ()
+            {
+              diag_record dr (info);
+
+              dr << "while retrieving information for ";
+              add_package_info (pm, dr);
+            }));
+
+        try
+        {
+          ifdstream ifs (f);
+          manifest_parser mp (ifs, f.string ());
+
+          // Note that the package directory points to something temporary
+          // (e.g., .bpkg/tmp/6f746365314d/) and it's probably better to omit
+          // it entirely (the above exception guard will print all we've got).
+          //
+          nvs.push_back (pkg_verify (co, mp, dir_path ()));
+        }
+        catch (const manifest_parsing& e)
+        {
+          fail (e.name, e.line, e.column) << e.description;
+        }
+        catch (const io_error& e)
+        {
+          fail << "unable to read from " << f << ": " << e;
+        }
+
         mfs.push_back (move (f));
         pds.push_back (move (d));
       }
@@ -261,17 +304,13 @@ namespace bpkg
 
       assert (pm.location);
 
-      const path& f (mfs[i]);
-
       try
       {
-        ifdstream ifs (f);
-        manifest_parser mp (ifs, f.string ());
-
         optional<version>& pv (pvs[i].version);
 
         package_manifest m (
-          mp,
+          mfs[i].string (),
+          move (nvs[i]),
           [&pv] (version& v)
           {
             if (pv)
@@ -290,10 +329,6 @@ namespace bpkg
         dr << e.description << info;
         add_package_info (pm, dr);
       }
-      catch (const io_error& e)
-      {
-        fail << "unable to read from " << f << ": " << e;
-      }
 
       r.first.push_back  (move (pm));
       r.second.push_back (move (pvs[i].info));
diff --git a/bpkg/satisfaction.cxx b/bpkg/satisfaction.cxx
index 52def32..cbcb5a0 100644
--- a/bpkg/satisfaction.cxx
+++ b/bpkg/satisfaction.cxx
@@ -128,12 +128,10 @@ namespace bpkg
     return s;
   }
 
-  static version build2_version;
+  version build2_version;
 
-  void
-  satisfy_build2 (const common_options& o,
-                  const package_name& pkg,
-                  const dependency& d)
+  bool
+  satisfy_build2 (const common_options& o, const dependency& d)
   {
     assert (d.name == "build2");
 
@@ -180,18 +178,13 @@ namespace bpkg
         fail << "unable to determine build2 version of " << name_b (o);
     }
 
-    if (!satisfies (build2_version, d.constraint))
-      fail << "unable to satisfy constraint (" << d << ") for package "
-           << pkg <<
-        info << "available build2 version is " << build2_version;
+    return satisfies (build2_version, d.constraint);
   }
 
-  static version bpkg_version;
+  version bpkg_version;
 
-  void
-  satisfy_bpkg (const common_options&,
-                const package_name& pkg,
-                const dependency& d)
+  bool
+  satisfy_bpkg (const common_options&, const dependency& d)
   {
     assert (d.name == "bpkg");
 
@@ -200,9 +193,6 @@ namespace bpkg
     if (bpkg_version.empty ())
       bpkg_version = version (BPKG_VERSION_STR);
 
-    if (!satisfies (bpkg_version, d.constraint))
-      fail << "unable to satisfy constraint (" << d << ") for package "
-           << pkg <<
-        info << "available bpkg version is " << bpkg_version;
+    return satisfies (bpkg_version, d.constraint);
   }
 }
diff --git a/bpkg/satisfaction.hxx b/bpkg/satisfaction.hxx
index 7046a92..8df4580 100644
--- a/bpkg/satisfaction.hxx
+++ b/bpkg/satisfaction.hxx
@@ -42,13 +42,15 @@ namespace bpkg
 
   // Special build-time dependencies.
   //
-  void
-  satisfy_build2 (const common_options&,
-                  const package_name&,
-                  const dependency&);
+  extern version build2_version; // Set on the first satisfy_build2() call.
 
-  void
-  satisfy_bpkg (const common_options&, const package_name&, const dependency&);
+  bool
+  satisfy_build2 (const common_options&, const dependency&);
+
+  extern version bpkg_version;  // Set on the first satisfy_bpkg() call.
+
+  bool
+  satisfy_bpkg (const common_options&, const dependency&);
 }
 
 #endif // BPKG_SATISFACTION_HXX