More Boost.LEAF error handling, and starting some error handling tests

src/dds/cli/cmd/sdist_create.cpp

@@ -3,6 +3,8 @@
 #include <dds/error/errors.hpp>
 #include <dds/sdist/dist.hpp>
 
+#include <boost/leaf/common.hpp>
+#include <boost/leaf/handle_exception.hpp>
 #include <fmt/core.h>
 
 namespace dds::cli::cmd {
@@ -15,16 +17,38 @@ int sdist_create(const options& opts) {
         .include_apps  = true,
         .include_tests = true,
     };
-    auto pkg_man = package_manifest::load_from_directory(params.project_dir);
-    if (!pkg_man) {
-        dds::throw_user_error<
-            errc::invalid_pkg_filesystem>("The source root at [{}] is not a valid dds source root",
-                                          params.project_dir.string());
-    }
-    auto default_filename = fmt::format("{}.tar.gz", pkg_man->id.to_string());
-    auto filepath         = opts.out_path.value_or(fs::current_path() / default_filename);
-    create_sdist_targz(filepath, params);
-    return 0;
+    return boost::leaf::try_catch(
+        [&] {
+            auto pkg_man = package_manifest::load_from_directory(params.project_dir).value();
+            auto default_filename = fmt::format("{}.tar.gz", pkg_man.id.to_string());
+            auto filepath         = opts.out_path.value_or(fs::current_path() / default_filename);
+            create_sdist_targz(filepath, params);
+            return 0;
+        },
+        [&](boost::leaf::bad_result, e_missing_file missing, e_human_message msg) {
+            dds_log(error,
+                    "A required file is missing for creating a source distribution for [{}]",
+                    params.project_dir.string());
+            dds_log(error, "Error: {}", msg.value);
+            dds_log(error, "Missing file: {}", missing.path.string());
+            return 1;
+        },
+        [&](std::error_code ec, e_human_message msg, boost::leaf::e_file_name file) {
+            dds_log(error, "Error: {}", msg.value);
+            dds_log(error, "Failed to access file [{}]: {}", file.value, ec.message());
+            return 1;
+        },
+        [&](std::error_code ec, e_human_message msg) {
+            dds_log(error, "Unexpected error: {}: {}", msg.value, ec.message());
+            return 1;
+        },
+        [&](boost::leaf::bad_result, std::errc ec) {
+            dds_log(error,
+                    "Failed to create source distribution from directory [{}]: {}",
+                    params.project_dir.string(),
+                    std::generic_category().message(int(ec)));
+            return 1;
+        });
 }
 
 }  // namespace dds::cli::cmd

src/dds/cli/error_handler.cpp

@@ -6,12 +6,17 @@
 #include <dds/util/result.hpp>
 #include <dds/util/signal.hpp>
 
+#include <boost/leaf/common.hpp>
 #include <boost/leaf/handle_error.hpp>
 #include <boost/leaf/handle_exception.hpp>
 #include <boost/leaf/result.hpp>
 #include <fmt/ostream.h>
+#include <json5/parse_data.hpp>
+#include <neo/scope.hpp>
 #include <neo/url/parse.hpp>
 
+#include <fstream>
+
 namespace {
 
 template <dds::cli::subcommand Val>
@@ -22,6 +27,17 @@ auto handlers = std::tuple(  //
         dds_log(error, "Invalid URL '{}': {}", bad_url.value, exc.what());
         return 1;
     },
+    [](boost::leaf::catch_<dds::error_base> exc,
+       json5::parse_error                   parse_err,
+       boost::leaf::e_file_name*            maybe_fpath) {
+        dds_log(error, "{}", exc.value().what());
+        dds_log(error, "Invalid JSON5 was found: {}", parse_err.what());
+        if (maybe_fpath) {
+            dds_log(error, "  (While reading from [{}])", maybe_fpath->value);
+        }
+        dds_log(error, "{}", exc.value().explanation());
+        return 1;
+    },
     [](boost::leaf::catch_<dds::error_base> exc) {
         dds_log(error, "{}", exc.value().what());
         dds_log(error, "{}", exc.value().explanation());
@@ -39,6 +55,25 @@ auto handlers = std::tuple(  //
 }  // namespace
 
 int dds::handle_cli_errors(std::function<int()> fn) noexcept {
-    return boost::leaf::try_handle_all([&]() -> boost::leaf::result<int> { return fn(); },
-                                       handlers);
+    return boost::leaf::try_catch(
+        [&] {
+            boost::leaf::context<dds::e_error_marker> marker_ctx;
+            marker_ctx.activate();
+            neo_defer {
+                marker_ctx.deactivate();
+                marker_ctx.handle_error<void>(
+                    boost::leaf::current_error(),
+                    [](dds::e_error_marker mark) {
+                        dds_log(trace, "[error marker {}]", mark.value);
+                        auto efile_path = std::getenv("DDS_WRITE_ERROR_MARKER");
+                        if (efile_path) {
+                            std::ofstream outfile{efile_path, std::ios::binary};
+                            fmt::print(outfile, "{}", mark.value);
+                        }
+                    },
+                    [] {});
+            };
+            return fn();
+        },
+        handlers);
 }

src/dds/sdist/package.cpp

@@ -3,8 +3,10 @@
 #include <dds/dym.hpp>
 #include <dds/error/errors.hpp>
 #include <dds/util/log.hpp>
+#include <dds/util/result.hpp>
 #include <dds/util/string.hpp>
 
+#include <boost/leaf/common.hpp>
 #include <range/v3/view/split.hpp>
 #include <range/v3/view/split_when.hpp>
 #include <range/v3/view/transform.hpp>
@@ -108,34 +110,50 @@ package_manifest package_manifest::load_from_file(const fs::path& fpath) {
 
 package_manifest package_manifest::load_from_json5_str(std::string_view content,
                                                        std::string_view input_name) {
-    auto data = json5::parse_data(content);
     try {
+        auto data = json5::parse_data(content);
         return parse_json(data, input_name);
     } catch (const semester::walk_error& e) {
         throw_user_error<errc::invalid_pkg_manifest>(e.what());
+    } catch (const json5::parse_error& err) {
+        BOOST_LEAF_THROW_EXCEPTION(user_error<errc::invalid_pkg_manifest>(
+                                       "Invalid package manifest JSON5 document"),
+                                   err,
+                                   boost::leaf::e_file_name{std::string(input_name)},
+                                   DDS_ERROR_MARKER("package-json5-parse-error"));
     }
 }
 
-std::optional<fs::path> package_manifest::find_in_directory(path_ref dirpath) {
+result<fs::path> package_manifest::find_in_directory(path_ref dirpath) {
     auto cands = {
         "package.json5",
         "package.jsonc",
         "package.json",
     };
     for (auto c : cands) {
-        auto cand = dirpath / c;
-        if (fs::is_regular_file(cand)) {
+        auto            cand = dirpath / c;
+        std::error_code ec;
+        if (fs::is_regular_file(cand, ec)) {
             return cand;
         }
+        if (ec != std::errc::no_such_file_or_directory) {
+            return boost::leaf::
+                new_error(ec,
+                          DDS_E_ARG(e_human_message{
+                              "Failed to check for package manifest in project directory"}),
+                          DDS_ERROR_MARKER("failed-package-json5-scan"),
+                          DDS_E_ARG(boost::leaf::e_file_name{cand.string()}));
+        }
     }
 
-    return std::nullopt;
+    return boost::leaf::new_error(std::errc::no_such_file_or_directory,
+                                  DDS_E_ARG(
+                                      e_human_message{"Expected to find a package manifest file"}),
+                                  DDS_E_ARG(e_missing_file{dirpath / "package.json5"}),
+                                  DDS_ERROR_MARKER("no-package-json5"));
 }
 
-std::optional<package_manifest> package_manifest::load_from_directory(path_ref dirpath) {
-    auto found = find_in_directory(dirpath);
-    if (!found.has_value()) {
-        return std::nullopt;
-    }
-    return load_from_file(*found);
+result<package_manifest> package_manifest::load_from_directory(path_ref dirpath) {
+    BOOST_LEAF_AUTO(found, find_in_directory(dirpath));
+    return load_from_file(found);
 }

src/dds/sdist/package.hpp

@@ -3,6 +3,7 @@
 #include <dds/deps.hpp>
 #include <dds/pkg/id.hpp>
 #include <dds/util/fs.hpp>
+#include <dds/util/result.hpp>
 
 #include <optional>
 #include <string>
@@ -45,8 +46,8 @@ struct package_manifest {
      * for a few file candidates and return the result from the first matching.
      * If none match, it will return nullopt.
      */
-    static std::optional<fs::path>         find_in_directory(path_ref);
-    static std::optional<package_manifest> load_from_directory(path_ref);
+    static result<fs::path>         find_in_directory(path_ref);
+    static result<package_manifest> load_from_directory(path_ref);
 };
 
 }  // namespace dds

src/dds/util/result.hpp

@@ -2,9 +2,12 @@
 
 #include <boost/leaf/on_error.hpp>
 #include <boost/leaf/result.hpp>
+#include <neo/concepts.hpp>
+#include <neo/fwd.hpp>
 #include <neo/pp.hpp>
 
 #include <exception>
+#include <filesystem>
 #include <string>
 
 namespace dds {
@@ -12,7 +15,28 @@ namespace dds {
 using boost::leaf::current_error;
 using boost::leaf::error_id;
 using boost::leaf::new_error;
-using boost::leaf::result;
+
+template <typename T>
+class result : public boost::leaf::result<T> {
+public:
+    using result_base = boost::leaf::result<T>;
+
+    using result_base::result;
+
+    template <neo::convertible_to<result_base> Other>
+    result(Other&& oth) noexcept
+        : result_base(static_cast<result_base>(NEO_FWD(oth))) {}
+
+    constexpr bool has_value() const noexcept { return !!*this; }
+    template <typename U>
+    constexpr T value_or(U&& u) const noexcept {
+        if (has_value()) {
+            return **this;
+        } else {
+            return T(NEO_FWD(u));
+        }
+    }
+};
 
 /**
  * @brief Error object representing a captured system_error exception
@@ -34,17 +58,40 @@ struct e_url_string {
     std::string value;
 };
 
+struct e_human_message {
+    std::string value;
+};
+
+struct e_missing_file {
+    std::filesystem::path path;
+};
+
+struct e_error_marker {
+    std::string_view value;
+};
+
+struct e_parse_error {
+    std::string value;
+};
+
 /**
  * @brief Capture currently in-flight special exceptions as new error object. Works around a bug in
  * Boost.LEAF when catching std::system error.
  */
 [[noreturn]] void capture_exception();
 
+#define DDS_E_ARG(...) ([&] { return __VA_ARGS__; })
+
+#define DDS_ERROR_MARKER(Value) DDS_E_ARG(::dds::e_error_marker{Value})
+
 /**
  * @brief Generate a leaf::on_error object that loads the given expression into the currently
  * in-flight error if the current scope is exitted via exception or a bad result<>
  */
 #define DDS_E_SCOPE(...)                                                                           \
-    auto NEO_CONCAT(_err_info_, __LINE__) = boost::leaf::on_error([&] { return __VA_ARGS__; })
+    auto NEO_CONCAT(_err_info_, __LINE__) = boost::leaf::on_error(DDS_E_ARG(__VA_ARGS__))
 
 }  // namespace dds
+
+template <typename T>
+struct boost::leaf::is_result_type<dds::result<T>> : std::true_type {};

tests/test_sdist.py

@@ -2,9 +2,11 @@ import pytest
 from pathlib import Path
 from typing import Tuple
 import subprocess
+import platform
 
 from dds_ci import proc
-from dds_ci.testing import ProjectOpener, Project
+from dds_ci.dds import DDSWrapper
+from dds_ci.testing import ProjectOpener, Project, error
 
 
 @pytest.fixture()
@@ -76,3 +78,20 @@ def test_import_sdist_stdin(test_sdist: Tuple[Path, Project]) -> None:
     # Excluded file will not be in the sdist:
     assert not repo_content_path.joinpath('other-file.txt').is_file(), \
         'Non-package content appeared in the package cache'
+
+
+def test_sdist_invalid_project(tmp_project: Project) -> None:
+    with error.expect_error_marker('no-package-json5'):
+        tmp_project.sdist_create()
+
+
+@pytest.mark.skipif(platform.system() != 'Linux', reason='We know this fails on Linux')
+def test_sdist_unreadable_dir(dds: DDSWrapper) -> None:
+    with error.expect_error_marker('failed-package-json5-scan'):
+        dds.run(['sdist', 'create', '--project=/root'])
+
+
+def test_sdist_invalid_json5(tmp_project: Project) -> None:
+    tmp_project.write('package.json5', 'bogus json5')
+    with error.expect_error_marker('package-json5-parse-error'):
+        tmp_project.sdist_create()

tools/dds_ci/proc.py

@@ -44,5 +44,4 @@ def run(*cmd: CommandLine, cwd: Optional[Pathish] = None, check: bool = False) -
 
 
 def check_run(*cmd: CommandLine, cwd: Optional[Pathish] = None) -> ProcessResult:
-    command = list(flatten_cmd(cmd))
-    return subprocess.run(command, cwd=cwd, check=True)
+    return run(cmd, cwd=cwd, check=True)

tools/dds_ci/testing/error.py

@@ -0,0 +1,27 @@
+"""
+Test utility for error checking
+"""
+
+from contextlib import contextmanager
+from typing import Iterator
+import subprocess
+from pathlib import Path
+import tempfile
+import os
+
+
+@contextmanager
+def expect_error_marker(expect: str) -> Iterator[None]:
+    tdir = Path(tempfile.mkdtemp())
+    err_file = tdir / 'error'
+    try:
+        os.environ['DDS_WRITE_ERROR_MARKER'] = str(err_file)
+        yield
+        assert False, 'dds subprocess did not raise CallProcessError!'
+    except subprocess.CalledProcessError:
+        assert err_file.exists(), 'No error marker file was generated, but dds exited with an error'
+        marker = err_file.read_text().strip()
+        assert marker == expect, \
+            f'dds did not produce the expected error (Expected {expect}, got {marker})'
+    finally:
+        os.environ.pop('DDS_WRITE_ERROR_MARKER')

tools/dds_ci/testing/fixtures.py

@@ -157,9 +157,9 @@ class ProjectOpener():
 
 
 @pytest.fixture()
-def project_opener(request: FixtureRequest, worker_id: str, dds_2: DDSWrapper,
+def project_opener(request: FixtureRequest, worker_id: str, dds: DDSWrapper,
                    tmp_path_factory: TempPathFactory) -> ProjectOpener:
-    opener = ProjectOpener(dds_2, request, worker_id, tmp_path_factory)
+    opener = ProjectOpener(dds, request, worker_id, tmp_path_factory)
     return opener
 
 
@@ -179,7 +179,7 @@ def tmp_project(request: FixtureRequest, worker_id: str, project_opener: Project
 
 
 @pytest.fixture(scope='session')
-def dds_2(dds_exe: Path) -> NewDDSWrapper:
+def dds(dds_exe: Path) -> NewDDSWrapper:
     wr = NewDDSWrapper(dds_exe)
     return wr