Refactor remote package database entries to be more stable around URLs stored in the DB, and storing URLs in repo DBs as well

src/dds/cli/cmd/repoman_add.cpp

@@ -14,13 +14,12 @@
 namespace dds::cli::cmd {
 
 static int _repoman_add(const options& opts) {
-    auto             pkg_id  = dds::pkg_id::parse(opts.repoman.add.pkg_id_str);
-    auto             listing = parse_remote_url(opts.repoman.add.url_str);
+    auto             pkg_id = dds::pkg_id::parse(opts.repoman.add.pkg_id_str);
+    auto             rpkg   = any_remote_pkg::from_url(neo::url::parse(opts.repoman.add.url_str));
     dds::pkg_listing add_info{
         .ident       = pkg_id,
-        .deps        = {},
         .description = opts.repoman.add.description,
-        .remote      = listing,
+        .remote_pkg  = rpkg,
     };
     auto temp_sdist = get_package_sdist(add_info);
 
@@ -71,6 +70,11 @@ int repoman_add(const options& opts) {
                     resp.status_message);
             return 1;
         },
+        [](dds::user_error<errc::invalid_remote_url> e, neo::url url) -> int {
+            dds_log(error, "Invalid URL '{}': {}", url.to_string(), e.what());
+            write_error_marker("repoman-add-invalid-pkg-url");
+            throw;
+        },
         [](dds::e_sqlite3_error_exc e, dds::e_repo_import_targz tgz) {
             dds_log(error, "Database error while importing tar file {}: {}", tgz.path, e.message);
             return 1;

src/dds/cli/cmd/repoman_remove.cpp

@@ -27,22 +27,13 @@ int repoman_remove(const options& opts) {
                 dds::capture_exception();
             }
         },
-        [](dds::e_sqlite3_error_exc,
-           boost::leaf::match<neo::sqlite3::errc, neo::sqlite3::errc::constraint_unique>,
-           dds::e_repo_import_targz tgz,
-           dds::pkg_id              pkid) {
-            dds_log(error,
-                    "Package {} (from {}) is already present in the repository",
-                    pkid.to_string(),
-                    tgz.path);
-            return 1;
-        },
         [](dds::e_system_error_exc e, dds::e_repo_delete_path tgz, dds::pkg_id pkid) {
             dds_log(error,
-                    "Cannot delete requested package '{}' from repository (Path {}): {}",
+                    "Cannot delete requested package '{}' from repository {}: {}",
                     pkid.to_string(),
                     tgz.path,
                     e.message);
+            write_error_marker("repoman-rm-no-such-package");
             return 1;
         },
         [](dds::e_system_error_exc e, dds::e_open_repo_db db) {

src/dds/cli/cmd/sdist_create.cpp

@@ -31,22 +31,13 @@ int sdist_create(const options& opts) {
                     params.project_dir.string());
             dds_log(error, "Error: {}", msg.value);
             dds_log(error, "Missing file: {}", missing.path.string());
+            write_error_marker("no-package-json5");
             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)));
+            write_error_marker("failed-package-json5-scan");
             return 1;
         });
 }

src/dds/cli/error_handler.cpp

@@ -36,6 +36,7 @@ auto handlers = std::tuple(  //
             dds_log(error, "  (While reading from [{}])", maybe_fpath->value);
         }
         dds_log(error, "{}", exc.value().explanation());
+        dds::write_error_marker("package-json5-parse-error");
         return 1;
     },
     [](boost::leaf::catch_<dds::error_base> exc) {
@@ -48,6 +49,13 @@ auto handlers = std::tuple(  //
         dds_log(critical, "Operation cancelled by the user");
         return 2;
     },
+    [](dds::e_system_error_exc exc, boost::leaf::verbose_diagnostic_info const& diag) {
+        dds_log(critical,
+                "An unhandled std::system_error arose. THIS IS A DDS BUG! Info: {}",
+                diag);
+        dds_log(critical, "Exception message from std::system_error: {}", exc.message);
+        return 42;
+    },
     [](boost::leaf::verbose_diagnostic_info const& diag) {
         dds_log(critical, "An unhandled error arose. THIS IS A DDS BUG! Info: {}", diag);
         return 42;

src/dds/error/errors.hpp

@@ -86,6 +86,16 @@ struct external_error : external_error_base {
 
 using error_invalid_default_toolchain = user_error<errc::invalid_builtin_toolchain>;
 
+template <errc ErrorCode, typename... Args>
+auto make_user_error(std::string_view fmt_str, Args&&... args) {
+    return user_error<ErrorCode>(fmt::format(fmt_str, std::forward<Args>(args)...));
+}
+
+template <errc ErrorCode>
+auto make_user_error() {
+    return user_error<ErrorCode>(std::string(default_error_string(ErrorCode)));
+}
+
 template <errc ErrorCode, typename... Args>
 [[noreturn]] void throw_user_error(std::string_view fmt_str, Args&&... args) {
     throw user_error<ErrorCode>(fmt::format(fmt_str, std::forward<Args>(args)...));
@@ -96,14 +106,24 @@ template <errc ErrorCode>
     throw user_error<ErrorCode>(std::string(default_error_string(ErrorCode)));
 }
 
+template <errc ErrorCode, typename... Args>
+auto make_external_error(std::string_view fmt_str, Args&&... args) {
+    return external_error<ErrorCode>(fmt::format(fmt_str, std::forward<Args>(args)...));
+}
+
+template <errc ErrorCode>
+auto make_external_error() {
+    return external_error<ErrorCode>(std::string(default_error_string(ErrorCode)));
+}
+
 template <errc ErrorCode, typename... Args>
 [[noreturn]] void throw_external_error(std::string_view fmt_str, Args&&... args) {
-    throw external_error<ErrorCode>(fmt::format(fmt_str, std::forward<Args>(args)...));
+    throw make_external_error<ErrorCode>(fmt::format(fmt_str, std::forward<Args>(args)...));
 }
 
 template <errc ErrorCode>
 [[noreturn]] void throw_external_error() {
-    throw external_error<ErrorCode>(std::string(default_error_string(ErrorCode)));
+    throw make_external_error<ErrorCode>(std::string(default_error_string(ErrorCode)));
 }
 
 }  // namespace dds

src/dds/pkg/db.cpp

@@ -134,72 +134,22 @@ void migrate_repodb_3(nsql::database& db) {
     )");
 }
 
-void store_with_remote(const neo::sqlite3::statement_cache&,
-                       const pkg_listing& pkg,
-                       std::monostate) {
-    neo_assert_always(
-        invariant,
-        false,
-        "There was an attempt to insert a package listing into the database where that package "
-        "listing does not have a remote listing. If you see this message, it is a dds bug.",
-        pkg.ident.to_string());
-}
-
-void store_with_remote(neo::sqlite3::statement_cache& stmts,
-                       const pkg_listing&             pkg,
-                       const http_remote_listing&     http) {
-    nsql::exec(  //
-        stmts(R"(
-            INSERT OR REPLACE INTO dds_pkgs (
-                name,
-                version,
-                remote_url,
-                description
-            ) VALUES (?1, ?2, ?3, ?4)
-        )"_sql),
-        pkg.ident.name,
-        pkg.ident.version.to_string(),
-        http.url,
-        pkg.description);
-}
-
-void store_with_remote(neo::sqlite3::statement_cache& stmts,
-                       const pkg_listing&             pkg,
-                       const git_remote_listing&      git) {
-    std::string url = git.url;
-    if (url.starts_with("https://") || url.starts_with("http://")) {
-        url = "git+" + url;
-    }
-    if (git.auto_lib.has_value()) {
-        url += "?lm=" + git.auto_lib->namespace_ + "/" + git.auto_lib->name;
-    }
-    url += "#" + git.ref;
-
-    nsql::exec(  //
-        stmts(R"(
-            INSERT OR REPLACE INTO dds_pkgs (
-                name,
-                version,
-                remote_url,
-                description
-            ) VALUES (
-                ?1,
-                ?2,
-                ?3,
-                ?4
-            )
-        )"_sql),
-        pkg.ident.name,
-        pkg.ident.version.to_string(),
-        url,
-        pkg.description);
-}
-
 void do_store_pkg(neo::sqlite3::database&        db,
                   neo::sqlite3::statement_cache& st_cache,
                   const pkg_listing&             pkg) {
     dds_log(debug, "Recording package {}@{}", pkg.ident.name, pkg.ident.version.to_string());
-    std::visit([&](auto&& remote) { store_with_remote(st_cache, pkg, remote); }, pkg.remote);
+    auto& store_pkg_st = st_cache(R"(
+        INSERT OR REPLACE INTO dds_pkgs
+            (name, version, remote_url, description)
+        VALUES
+            (?, ?, ?, ?)
+    )"_sql);
+    nsql::exec(store_pkg_st,
+               pkg.ident.name,
+               pkg.ident.version.to_string(),
+               pkg.remote_pkg.to_url_string(),
+               pkg.description);
+
     auto  db_pkg_id  = db.last_insert_rowid();
     auto& new_dep_st = st_cache(R"(
         INSERT INTO dds_pkg_deps (
@@ -283,7 +233,6 @@ fs::path pkg_db::default_path() noexcept { return dds_data_dir() / "pkgs.db"; }
 pkg_db pkg_db::open(const std::string& db_path) {
     if (db_path != ":memory:") {
         auto pardir = fs::weakly_canonical(db_path).parent_path();
-        dds_log(trace, "Ensuring parent directory [{}]", pardir.string());
         fs::create_directories(pardir);
     }
     dds_log(debug, "Opening package database [{}]", db_path);
@@ -363,10 +312,10 @@ std::optional<pkg_listing> pkg_db::get(const pkg_id& pk_id) const noexcept {
     auto deps = dependencies_of(pk_id);
 
     auto info = pkg_listing{
-        pk_id,
-        std::move(deps),
-        std::move(description),
-        parse_remote_url(remote_url),
+        .ident       = pk_id,
+        .deps        = std::move(deps),
+        .description = std::move(description),
+        .remote_pkg  = any_remote_pkg::from_url(neo::url::parse(remote_url)),
     };
 
     return info;

src/dds/pkg/db.test.cpp

@@ -27,10 +27,10 @@ public:
 
 TEST_CASE_METHOD(catalog_test_case, "Store a simple package") {
     db.store(dds::pkg_listing{
-        dds::pkg_id("foo", semver::version::parse("1.2.3")),
+        dds::pkg_id{"foo", semver::version::parse("1.2.3")},
         {},
         "example",
-        dds::git_remote_listing{std::nullopt, "git+http://example.com", "master"},
+        dds::any_remote_pkg::from_url(neo::url::parse("git+http://example.com#master")),
     });
 
     auto pkgs = db.by_name("foo");
@@ -41,20 +41,19 @@ TEST_CASE_METHOD(catalog_test_case, "Store a simple package") {
     REQUIRE(info);
     CHECK(info->ident == pkgs[0]);
     CHECK(info->deps.empty());
-    CHECK(std::holds_alternative<dds::git_remote_listing>(info->remote));
-    CHECK(std::get<dds::git_remote_listing>(info->remote).ref == "master");
+    CHECK(info->remote_pkg.to_url_string() == "git+http://example.com#master");
 
     // Update the entry with a new git remote ref
     CHECK_NOTHROW(db.store(dds::pkg_listing{
-        dds::pkg_id("foo", semver::version::parse("1.2.3")),
+        dds::pkg_id{"foo", semver::version::parse("1.2.3")},
         {},
         "example",
-        dds::git_remote_listing{std::nullopt, "git+http://example.com", "develop"},
+        dds::any_remote_pkg::from_url(neo::url::parse("git+http://example.com#develop")),
     }));
     // The previous pkg_id is still a valid lookup key
     info = db.get(pkgs[0]);
     REQUIRE(info);
-    CHECK(std::get<dds::git_remote_listing>(info->remote).ref == "develop");
+    CHECK(info->remote_pkg.to_url_string() == "git+http://example.com#develop");
 }
 
 TEST_CASE_METHOD(catalog_test_case, "Package requirements") {
@@ -65,7 +64,7 @@ TEST_CASE_METHOD(catalog_test_case, "Package requirements") {
             {"baz", {semver::version::parse("5.3.0"), semver::version::parse("6.0.0")}},
         },
         "example",
-        dds::git_remote_listing{std::nullopt, "git+http://example.com", "master"},
+        dds::any_remote_pkg::from_url(neo::url::parse("git+http://example.com#master")),
     });
     auto pkgs = db.by_name("foo");
     REQUIRE(pkgs.size() == 1);

src/dds/pkg/get/base.cpp

@@ -7,20 +7,27 @@
 
 using namespace dds;
 
-void remote_listing_base::generate_auto_lib_files(const pkg_id& pid, path_ref root) const {
-    if (auto_lib.has_value()) {
-        dds_log(info, "Generating library data automatically");
-
-        auto pkg_strm         = open(root / "package.json5", std::ios::binary | std::ios::out);
-        auto man_json         = nlohmann::json::object();
-        man_json["name"]      = pid.name;
-        man_json["version"]   = pid.version.to_string();
-        man_json["namespace"] = auto_lib->namespace_;
-        pkg_strm << nlohmann::to_string(man_json);
-
-        auto lib_strm    = open(root / "library.json5", std::ios::binary | std::ios::out);
-        auto lib_json    = nlohmann::json::object();
-        lib_json["name"] = auto_lib->name;
-        lib_strm << nlohmann::to_string(lib_json);
-    }
-}
+// void remote_pkg_base::generate_auto_lib_files(const pkg_id& pid, path_ref root) const {
+//     if (auto_lib.has_value()) {
+//         dds_log(info, "Generating library data automatically");
+
+//         auto pkg_strm         = open(root / "package.json5", std::ios::binary | std::ios::out);
+//         auto man_json         = nlohmann::json::object();
+//         man_json["name"]      = pid.name;
+//         man_json["version"]   = pid.version.to_string();
+//         man_json["namespace"] = auto_lib->namespace_;
+//         pkg_strm << nlohmann::to_string(man_json);
+
+//         auto lib_strm    = open(root / "library.json5", std::ios::binary | std::ios::out);
+//         auto lib_json    = nlohmann::json::object();
+//         lib_json["name"] = auto_lib->name;
+//         lib_strm << nlohmann::to_string(lib_json);
+//     }
+// }
+
+void remote_pkg_base::get_sdist(path_ref dest) const { get_raw_directory(dest); }
+void remote_pkg_base::get_raw_directory(path_ref dest) const { do_get_raw(dest); }
+
+neo::url remote_pkg_base::to_url() const { return do_to_url(); }
+
+std::string remote_pkg_base::to_url_string() const { return to_url().to_string(); }

src/dds/pkg/get/base.hpp

@@ -2,6 +2,7 @@
 
 #include <libman/package.hpp>
 #include <neo/concepts.hpp>
+#include <neo/url.hpp>
 
 #include <optional>
 #include <vector>
@@ -10,13 +11,16 @@ namespace dds {
 
 struct pkg_id;
 
-struct remote_listing_base {
-    std::optional<lm::usage> auto_lib{};
+class remote_pkg_base {
+    virtual void     do_get_raw(path_ref dest) const = 0;
+    virtual neo::url do_to_url() const               = 0;
 
-    void generate_auto_lib_files(const pkg_id& pid, path_ref root) const;
-};
+public:
+    void get_sdist(path_ref dest) const;
+    void get_raw_directory(path_ref dest) const;
 
-template <typename T>
-concept remote_listing = neo::derived_from<std::remove_cvref_t<T>, remote_listing_base>;
+    neo::url    to_url() const;
+    std::string to_url_string() const;
+};
 
 }  // namespace dds

src/dds/pkg/get/dds_http.cpp

@@ -0,0 +1,41 @@
+#include "./dds_http.hpp"
+
+#include "./http.hpp"
+
+#include <fmt/core.h>
+
+using namespace dds;
+
+neo::url dds_http_remote_pkg::do_to_url() const {
+    auto ret   = repo_url;
+    ret.scheme = "dds+" + ret.scheme;
+    ret.path   = fmt::format("{}/{}", ret.path, pkg_id.to_string());
+    return ret;
+}
+
+dds_http_remote_pkg dds_http_remote_pkg::from_url(const neo::url& url) {
+    auto repo_url = url;
+    if (repo_url.scheme.starts_with("dds+")) {
+        repo_url.scheme = repo_url.scheme.substr(4);
+    } else if (repo_url.scheme.ends_with("+dds")) {
+        repo_url.scheme = repo_url.scheme.substr(0, repo_url.scheme.size() - 4);
+    } else {
+        // Nothing to trim
+    }
+
+    fs::path full_path = repo_url.path;
+    repo_url.path      = full_path.parent_path().generic_string();
+    auto pkg_id        = dds::pkg_id::parse(full_path.filename().string());
+
+    return {repo_url, pkg_id};
+}
+
+void dds_http_remote_pkg::do_get_raw(path_ref dest) const {
+    auto     http_url = repo_url;
+    fs::path path     = fs::path(repo_url.path) / "pkg" / pkg_id.name / pkg_id.version.to_string()
+        / "sdist.tar.gz";
+    http_url.path = path.lexically_normal().generic_string();
+    http_remote_pkg http;
+    http.url = http_url;
+    http.get_raw_directory(dest);
+}

src/dds/pkg/get/dds_http.hpp

@@ -0,0 +1,31 @@
+#pragma once
+
+#include "./base.hpp"
+
+#include <dds/pkg/id.hpp>
+
+#include <neo/url.hpp>
+
+#include <string>
+#include <string_view>
+
+namespace dds {
+
+class dds_http_remote_pkg : public remote_pkg_base {
+    void     do_get_raw(path_ref) const override;
+    neo::url do_to_url() const override;
+
+public:
+    neo::url    repo_url;
+    dds::pkg_id pkg_id;
+
+    dds_http_remote_pkg() = default;
+
+    dds_http_remote_pkg(neo::url u, dds::pkg_id pid)
+        : repo_url(u)
+        , pkg_id(pid) {}
+
+    static dds_http_remote_pkg from_url(const neo::url& url);
+};
+
+}  // namespace dds

src/dds/pkg/get/dds_http.test.cpp

@@ -0,0 +1,12 @@
+#include "./dds_http.hpp"
+
+#include <catch2/catch.hpp>
+
+TEST_CASE("Parse a URL") {
+    auto pkg = dds::dds_http_remote_pkg::from_url(
+        neo::url::parse("dds+http://foo.bar/repo-dir/egg@1.2.3"));
+    CHECK(pkg.repo_url.to_string() == "http://foo.bar/repo-dir");
+    CHECK(pkg.pkg_id.name == "egg");
+    CHECK(pkg.pkg_id.version.to_string() == "1.2.3");
+    CHECK(pkg.to_url_string() == "dds+http://foo.bar/repo-dir/egg@1.2.3");
+}

src/dds/pkg/get/get.cpp

@@ -14,37 +14,25 @@ using namespace dds;
 
 namespace {
 
-temporary_sdist do_pull_sdist(const pkg_listing& listing, std::monostate) {
-    neo_assert_always(
-        invariant,
-        false,
-        "A package listing in the database has no defined remote from which to pull. This "
-        "shouldn't happen in normal usage. This will occur if the database has been "
-        "manually altered, or if DDS has a bug.",
-        listing.ident.to_string());
-}
-
-template <remote_listing R>
-temporary_sdist do_pull_sdist(const pkg_listing& listing, const R& remote) {
+temporary_sdist do_pull_sdist(const any_remote_pkg& rpkg) {
     auto tmpdir = dds::temporary_dir::create();
 
-    remote.pull_source(tmpdir.path());
-    remote.generate_auto_lib_files(listing.ident, tmpdir.path());
+    rpkg.get_sdist(tmpdir.path());
 
-    dds_log(info, "Create sdist ...");
-    sdist_params params;
-    params.project_dir = tmpdir.path();
-    auto sd_tmp_dir    = dds::temporary_dir::create();
-    params.dest_path   = sd_tmp_dir.path();
-    params.force       = true;
-    auto sd            = create_sdist(params);
+    auto         sd_tmp_dir = dds::temporary_dir::create();
+    sdist_params params{
+        .project_dir = tmpdir.path(),
+        .dest_path   = sd_tmp_dir.path(),
+        .force       = true,
+    };
+    auto sd = create_sdist(params);
     return {sd_tmp_dir, sd};
 }
 
 }  // namespace
 
 temporary_sdist dds::get_package_sdist(const pkg_listing& pkg) {
-    auto tsd = std::visit([&](auto&& remote) { return do_pull_sdist(pkg, remote); }, pkg.remote);
+    auto tsd = do_pull_sdist(pkg.remote_pkg);
     if (!(tsd.sdist.manifest.id == pkg.ident)) {
         throw_external_error<errc::sdist_ident_mismatch>(
             "The package name@version in the generated source distribution does not match the name "

src/dds/pkg/get/git.cpp

@@ -3,64 +3,57 @@
 #include <dds/error/errors.hpp>
 #include <dds/proc.hpp>
 #include <dds/util/log.hpp>
+#include <dds/util/result.hpp>
 
 #include <neo/url.hpp>
 #include <neo/url/query.hpp>
 
 using namespace dds;
-
-void git_remote_listing::pull_source(path_ref dest) const {
-    fs::remove_all(dest);
-    using namespace std::literals;
-    dds_log(info, "Clone Git repository [{}] (at {}) to [{}]", url, ref, dest.string());
-    auto command = {"git"s, "clone"s, "--depth=1"s, "--branch"s, ref, url, dest.generic_string()};
-    auto git_res = run_proc(command);
-    if (!git_res.okay()) {
-        throw_external_error<errc::git_clone_failure>(
-            "Git clone operation failed [Git command: {}] [Exitted {}]:\n{}",
-            quote_command(command),
-            git_res.retc,
-            git_res.output);
+using namespace std::literals;
+
+git_remote_pkg git_remote_pkg::from_url(const neo::url& url) {
+    if (!url.fragment) {
+        BOOST_LEAF_THROW_EXCEPTION(
+            user_error<errc::invalid_remote_url>(
+                "Git URL requires a fragment specified the Git ref to clone"),
+            DDS_E_ARG(e_url_string{url.to_string()}));
     }
-}
-
-git_remote_listing git_remote_listing::from_url(std::string_view sv) {
-    auto url = neo::url::parse(sv);
-    dds_log(trace, "Create Git remote listing from URL '{}'", sv);
-
-    auto ref     = url.fragment;
-    url.fragment = {};
-    auto q       = url.query;
-    url.query    = {};
-
-    std::optional<lm::usage> auto_lib;
-
+    git_remote_pkg ret;
+    ret.url = url;
     if (url.scheme.starts_with("git+")) {
-        url.scheme = url.scheme.substr(4);
+        ret.url.scheme = url.scheme.substr(4);
     } else if (url.scheme.ends_with("+git")) {
-        url.scheme = url.scheme.substr(0, url.scheme.size() - 4);
+        ret.url.scheme = url.scheme.substr(0, url.scheme.size() - 4);
     } else {
         // Leave the URL as-is
     }
+    ret.ref = *url.fragment;
+    ret.url.fragment.reset();
+    return ret;
+}
 
-    if (q) {
-        neo::basic_query_string_view qsv{*q};
-        for (auto qstr : qsv) {
-            if (qstr.key_raw() != "lm") {
-                dds_log(warn, "Unknown query string parameter in package url: '{}'", qstr.string());
-            } else {
-                auto_lib = lm::split_usage_string(qstr.value_decoded());
-            }
-        }
+neo::url git_remote_pkg::do_to_url() const {
+    neo::url ret = url;
+    ret.fragment = ref;
+    if (ret.scheme != "git") {
+        ret.scheme = "git+" + ret.scheme;
     }
+    return ret;
+}
 
-    if (!ref) {
-        throw_user_error<errc::invalid_remote_url>(
-            "Git URL requires a fragment specifying the Git ref to clone");
+void git_remote_pkg::do_get_raw(path_ref dest) const {
+    fs::remove(dest);
+    dds_log(info, "Clone Git repository [{}] (at {}) to [{}]", url.to_string(), ref, dest.string());
+    auto command
+        = {"git"s, "clone"s, "--depth=1"s, "--branch"s, ref, url.to_string(), dest.string()};
+    auto git_res = run_proc(command);
+    if (!git_res.okay()) {
+        BOOST_LEAF_THROW_EXCEPTION(
+            make_external_error<errc::git_clone_failure>(
+                "Git clone operation failed [Git command: {}] [Exitted {}]:\n{}",
+                quote_command(command),
+                git_res.retc,
+                git_res.output),
+            url);
     }
-    return git_remote_listing{
-        {.auto_lib = auto_lib},
-        url.to_string(),
-        *ref,
-    };
 }

src/dds/pkg/get/git.hpp

@@ -2,18 +2,21 @@
 
 #include "./base.hpp"
 
+#include <neo/url.hpp>
+
 #include <string>
-#include <string_view>
 
 namespace dds {
 
-struct git_remote_listing : remote_listing_base {
-    std::string url;
-    std::string ref;
+class git_remote_pkg : public remote_pkg_base {
+    void     do_get_raw(path_ref) const override;
+    neo::url do_to_url() const override;
 
-    void pull_source(path_ref path) const;
+public:
+    neo::url    url;
+    std::string ref;
 
-    static git_remote_listing from_url(std::string_view sv);
+    static git_remote_pkg from_url(const neo::url&);
 };
 
 }  // namespace dds

src/dds/pkg/get/git.test.cpp

@@ -0,0 +1,9 @@
+#include "./git.hpp"
+
+#include <catch2/catch.hpp>
+
+TEST_CASE("Round-trip a URL") {
+    auto git = dds::git_remote_pkg::from_url(
+        neo::url::parse("http://github.com/vector-of-bool/neo-fun.git#0.4.0"));
+    CHECK(git.to_url_string() == "git+http://github.com/vector-of-bool/neo-fun.git#0.4.0");
+}

src/dds/pkg/get/github.cpp

@@ -0,0 +1,42 @@
+#include "./github.hpp"
+
+#include "./http.hpp"
+
+#include <dds/error/errors.hpp>
+#include <dds/util/result.hpp>
+
+#include <fmt/format.h>
+#include <range/v3/iterator/operations.hpp>
+
+using namespace dds;
+
+neo::url github_remote_pkg::do_to_url() const {
+    neo::url ret;
+    ret.scheme = "github";
+    ret.path   = fmt::format("{}/{}/{}", owner, reponame, ref);
+    return ret;
+}
+
+void github_remote_pkg::do_get_raw(path_ref dest) const {
+    http_remote_pkg http;
+    auto new_url = fmt::format("https://github.com/{}/{}/archive/{}.tar.gz", owner, reponame, ref);
+    http.url     = neo::url::parse(new_url);
+    http.strip_n_components = 1;
+    http.get_raw_directory(dest);
+}
+
+github_remote_pkg github_remote_pkg::from_url(const neo::url& url) {
+    fs::path path = url.path;
+    if (ranges::distance(path) != 3) {
+        BOOST_LEAF_THROW_EXCEPTION(make_user_error<errc::invalid_remote_url>(
+                                       "'github:' URLs should have a path with three segments"),
+                                   url);
+    }
+    github_remote_pkg ret;
+    // Split the three path elements as {owner}/{reponame}/{git-ref}
+    auto elem_iter = path.begin();
+    ret.owner      = (*elem_iter++).generic_string();
+    ret.reponame   = (*elem_iter++).generic_string();
+    ret.ref        = (*elem_iter).generic_string();
+    return ret;
+}

src/dds/pkg/get/github.hpp

@@ -0,0 +1,24 @@
+#pragma once
+
+#include "./base.hpp"
+
+#include <neo/url.hpp>
+
+#include <string>
+#include <string_view>
+
+namespace dds {
+
+class github_remote_pkg : public remote_pkg_base {
+    void     do_get_raw(path_ref) const override;
+    neo::url do_to_url() const override;
+
+public:
+    std::string owner;
+    std::string reponame;
+    std::string ref;
+
+    static github_remote_pkg from_url(const neo::url&);
+};
+
+}  // namespace dds

src/dds/pkg/get/github.test.cpp

@@ -0,0 +1,11 @@
+#include "./github.hpp"
+
+#include <catch2/catch.hpp>
+
+TEST_CASE("Parse a github: URL") {
+    auto gh_pkg
+        = dds::github_remote_pkg::from_url(neo::url::parse("github:vector-of-bool/neo-fun/0.6.0"));
+    CHECK(gh_pkg.owner == "vector-of-bool");
+    CHECK(gh_pkg.reponame == "neo-fun");
+    CHECK(gh_pkg.ref == "0.6.0");
+}

src/dds/pkg/get/http.cpp

@@ -4,6 +4,7 @@
 #include <dds/temp.hpp>
 #include <dds/util/http/pool.hpp>
 #include <dds/util/log.hpp>
+#include <dds/util/result.hpp>
 
 #include <neo/io/stream/buffers.hpp>
 #include <neo/io/stream/file.hpp>
@@ -13,107 +14,110 @@
 
 using namespace dds;
 
-void http_remote_listing::pull_source(path_ref dest) const {
-    neo::url url;
-    try {
-        url = neo::url::parse(this->url);
-    } catch (const neo::url_validation_error& e) {
-        throw_user_error<errc::invalid_remote_url>("Failed to parse the string '{}' as a URL: {}",
-                                                   this->url,
-                                                   e.what());
-    }
-    dds_log(trace, "Downloading HTTP remote from [{}]", url.to_string());
+void http_remote_pkg::do_get_raw(path_ref dest) const {
+    dds_log(trace, "Downloading remote package via HTTP from [{}]", url.to_string());
 
     if (url.scheme != "http" && url.scheme != "https") {
         dds_log(error, "Unsupported URL scheme '{}' (in [{}])", url.scheme, url.to_string());
-        throw_user_error<errc::invalid_remote_url>(
-            "The given URL download is not supported. (Only 'http' URLs are supported, "
-            "got '{}')",
-            this->url);
+        BOOST_LEAF_THROW_EXCEPTION(user_error<errc::invalid_remote_url>(
+                                       "The given URL download is not supported. (Only 'http' and "
+                                       "'https' URLs are supported)"),
+                                   DDS_E_ARG(e_url_string{url.to_string()}));
     }
 
     neo_assert(invariant,
                !!url.host,
                "The given URL did not have a host part. This shouldn't be possible... Please file "
                "a bug report.",
-               this->url);
+               url.to_string());
 
-    auto tdir     = dds::temporary_dir::create();
-    auto url_path = fs::path(url.path);
-    auto fname    = url_path.filename();
+    // Create a temporary directory in which to download the archive
+    auto tdir = dds::temporary_dir::create();
+    // For ease of debugging, use the filename from the URL, if possible
+    auto fname = fs::path(url.path).filename();
     if (fname.empty()) {
         fname = "dds-download.tmp";
     }
     auto dl_path = tdir.path() / fname;
-    fs::create_directory(dl_path.parent_path());
+    fs::create_directories(tdir.path());
 
-    http_pool pool;
-    auto [client, resp] = pool.request(url);
-    auto dl_file        = neo::file_stream::open(dl_path, neo::open_mode::write);
-    client.recv_body_into(resp, neo::stream_io_buffers{dl_file});
-
-    neo_assert(invariant,
-               fs::is_regular_file(dl_path),
-               "HTTP client did not properly download the file??",
-               this->url,
-               dl_path);
+    // Download the file!
+    {
+        auto& pool          = http_pool::thread_local_pool();
+        auto [client, resp] = pool.request(url);
+        auto dl_file        = neo::file_stream::open(dl_path, neo::open_mode::write);
+        client.recv_body_into(resp, neo::stream_io_buffers{dl_file});
+    }
 
-    fs::create_directories(dest);
-    dds_log(debug, "Expanding downloaded source distribution into {}", dest.string());
+    fs::create_directories(fs::absolute(dest));
+    dds_log(debug, "Expanding downloaded package archive into [{}]", dest.string());
     std::ifstream infile{dl_path, std::ios::binary};
     try {
         neo::expand_directory_targz(
             neo::expand_options{
                 .destination_directory = dest,
                 .input_name            = dl_path.string(),
-                .strip_components      = this->strip_components,
+                .strip_components      = this->strip_n_components,
             },
             infile);
     } catch (const std::runtime_error& err) {
         throw_external_error<errc::invalid_remote_url>(
             "The file downloaded from [{}] failed to extract (Inner error: {})",
-            this->url,
+            url.to_string(),
             err.what());
     }
 }
 
-http_remote_listing http_remote_listing::from_url(std::string_view sv) {
-    auto url = neo::url::parse(sv);
-    dds_log(trace, "Create HTTP remote listing from URL [{}]", sv);
-
-    // Because archives most often have one top-level directory, the default strip-components
-    // setting is 'one'
-    unsigned int             strip_components = 1;
-    std::optional<lm::usage> auto_lib;
-
-    // IF we are a dds+ URL, strip_components should be zero, and give the url a plain
-    // HTTP/HTTPS scheme
-    if (url.scheme.starts_with("dds+")) {
-        url.scheme       = url.scheme.substr(4);
-        strip_components = 0;
-    } else if (url.scheme.ends_with("+dds")) {
-        url.scheme.erase(url.scheme.end() - 3);
-        strip_components = 0;
-    } else {
-        // Leave the URL as-is
+http_remote_pkg http_remote_pkg::from_url(const neo::url& url) {
+    neo_assert(expects,
+               url.scheme == neo::oper::any_of("http", "https"),
+               "Invalid URL for an HTTP remote",
+               url.to_string());
+
+    neo::url ret_url = url;
+    if (url.fragment) {
+        dds_log(warn,
+                "Fragment '{}' in URL [{}] will have no effect",
+                *url.fragment,
+                url.to_string());
+        ret_url.fragment.reset();
     }
 
+    ret_url.query = {};
+
+    unsigned n_strpcmp = 0;
+
     if (url.query) {
+        std::string query_acc;
+
         neo::basic_query_string_view qsv{*url.query};
         for (auto qstr : qsv) {
-            if (qstr.key_raw() == "dds_lm") {
-                auto_lib = lm::split_usage_string(qstr.value_decoded());
-            } else if (qstr.key_raw() == "dds_strpcmp") {
-                strip_components = static_cast<unsigned>(std::stoul(qstr.value_decoded()));
+            if (qstr.key_raw() == "__dds_strpcmp") {
+                n_strpcmp = static_cast<unsigned>(std::stoul(qstr.value_decoded()));
             } else {
-                dds_log(warn, "Unknown query string parameter in package url: '{}'", qstr.string());
+                if (!query_acc.empty()) {
+                    query_acc.push_back(';');
+                }
+                query_acc.append(qstr.string());
             }
         }
+        if (!query_acc.empty()) {
+            ret_url.query = query_acc;
+        }
     }
 
-    return http_remote_listing{
-        {.auto_lib = auto_lib},
-        url.to_string(),
-        strip_components,
-    };
+    return {ret_url, n_strpcmp};
+}
+
+neo::url http_remote_pkg::do_to_url() const {
+    auto ret_url = url;
+    if (strip_n_components != 0) {
+        auto strpcmp_param = fmt::format("__dds_strpcmp={}", strip_n_components);
+        if (ret_url.query) {
+            *ret_url.query += ";" + strpcmp_param;
+        } else {
+            ret_url.query = strpcmp_param;
+        }
+    }
+    return ret_url;
 }

src/dds/pkg/get/http.hpp

@@ -2,18 +2,28 @@
 
 #include "./base.hpp"
 
+#include <neo/url.hpp>
+
 #include <string>
 #include <string_view>
 
 namespace dds {
 
-struct http_remote_listing : remote_listing_base {
-    std::string url;
-    unsigned    strip_components = 0;
+class http_remote_pkg : public remote_pkg_base {
+    void     do_get_raw(path_ref) const override;
+    neo::url do_to_url() const override;
+
+public:
+    neo::url url;
+    unsigned strip_n_components = 0;
+
+    http_remote_pkg() = default;
 
-    void pull_source(path_ref path) const;
+    http_remote_pkg(neo::url u, unsigned strpcmp)
+        : url(u)
+        , strip_n_components(strpcmp) {}
 
-    static http_remote_listing from_url(std::string_view sv);
+    static http_remote_pkg from_url(const neo::url& url);
 };
 
 }  // namespace dds

src/dds/pkg/get/http.test.cpp

@@ -6,7 +6,14 @@
 
 #include <catch2/catch.hpp>
 
-TEST_CASE("Convert URL to an HTTP remote listing") {
-    auto remote = dds::http_remote_listing::from_url(
-        "http://localhost:8000/neo-buffer-0.4.2.tar.gz?dds_strpcmp=1");
+TEST_CASE("Convert from URL") {
+    auto listing = dds::http_remote_pkg::from_url(neo::url::parse("http://example.org/foo"));
+    CHECK(listing.to_url_string() == "http://example.org/foo");
+    listing.strip_n_components = 4;
+    CHECK(listing.to_url_string() == "http://example.org/foo?__dds_strpcmp=4");
+
+    listing = dds::http_remote_pkg::from_url(
+        neo::url::parse("http://example.org/foo?bar=baz;__dds_strpcmp=7;thing=foo#fragment"));
+    CHECK(listing.strip_n_components == 7);
+    CHECK(listing.to_url_string() == "http://example.org/foo?bar=baz;thing=foo;__dds_strpcmp=7");
 }

src/dds/pkg/id.cpp

@@ -13,7 +13,8 @@ pkg_id pkg_id::parse(const std::string_view s) {
     DDS_E_SCOPE(e_invalid_pkg_id_str{std::string(s)});
     auto at_pos = s.find('@');
     if (at_pos == s.npos) {
-        throw_user_error<errc::invalid_pkg_id>("Invalid package ID '{}'", s);
+        BOOST_LEAF_THROW_EXCEPTION(
+            make_user_error<errc::invalid_pkg_id>("Package ID must contain an '@' symbol"));
     }
 
     auto name    = s.substr(0, at_pos);
@@ -22,18 +23,8 @@ pkg_id pkg_id::parse(const std::string_view s) {
     try {
         return {std::string(name), semver::version::parse(ver_str)};
     } catch (const semver::invalid_version& err) {
-        BOOST_LEAF_THROW_EXCEPTION(user_error<errc::invalid_pkg_id>("Package ID string is invalid"),
-                                   err);
+        BOOST_LEAF_THROW_EXCEPTION(make_user_error<errc::invalid_pkg_id>(), err);
     }
 }
 
-pkg_id::pkg_id(std::string_view n, semver::version v)
-    : name(n)
-    , version(std::move(v)) {
-    if (name.find('@') != name.npos) {
-        throw_user_error<errc::invalid_pkg_name>(
-            "Invalid package name '{}' (The '@' character is not allowed)");
-    }
-}
-
-std::string pkg_id::to_string() const noexcept { return name + "@" + version.to_string(); }
+std::string pkg_id::to_string() const noexcept { return name + "@" + version.to_string(); }

src/dds/pkg/id.hpp

@@ -25,11 +25,6 @@ struct pkg_id {
     /// The version of the package
     semver::version version;
 
-    /// Default-initialize a pkg_id with a blank name and a default version
-    pkg_id() = default;
-    /// Construct a package ID from a name-version pair
-    pkg_id(std::string_view s, semver::version v);
-
     /**
      * Parse the given string into a pkg_id object.
      */
@@ -49,4 +44,4 @@ struct pkg_id {
     }
 };
 
-}  // namespace dds
+}  // namespace dds

src/dds/pkg/listing.cpp

@@ -1,6 +1,12 @@
 #include "./listing.hpp"
 
+#include "./get/dds_http.hpp"
+#include "./get/git.hpp"
+#include "./get/github.hpp"
+#include "./get/http.hpp"
+
 #include <dds/error/errors.hpp>
+#include <dds/util/result.hpp>
 #include <dds/util/string.hpp>
 
 #include <neo/url.hpp>
@@ -9,36 +15,40 @@
 
 using namespace dds;
 
-dds::remote_listing_var dds::parse_remote_url(std::string_view sv) {
-    neo_assertion_breadcrumbs("Loading package remote from URI string", sv);
-    auto url = neo::url::parse(sv);
-    if (url.scheme == neo::oper::any_of("git+https", "git+http", "http+git", "https+git", "git")) {
-        return git_remote_listing::from_url(sv);
-    } else if (url.scheme == neo::oper::any_of("http", "https")) {
-        return http_remote_listing::from_url(sv);
-    } else if (url.scheme == neo::oper::any_of("dds+http", "dds+https", "http+dds", "https+dds")) {
-        fs::path path         = url.path;
-        auto     leaf         = path.filename().string();
-        auto     namever_path = replace(leaf, "@", "/");
-        url.path = (path.parent_path() / "pkg" / namever_path / "sdist.tar.gz").generic_string();
-        return http_remote_listing::from_url(url.to_string());
+any_remote_pkg::~any_remote_pkg() = default;
+any_remote_pkg::any_remote_pkg() {}
+
+static std::shared_ptr<remote_pkg_base> do_parse_url(const neo::url& url) {
+    if (url.scheme == neo::oper::any_of("http", "https")) {
+        return std::make_shared<http_remote_pkg>(http_remote_pkg::from_url(url));
+    } else if (url.scheme
+               == neo::oper::any_of("git", "git+https", "git+http", "https+git", "http+git")) {
+        return std::make_shared<git_remote_pkg>(git_remote_pkg::from_url(url));
     } else if (url.scheme == "github") {
-        fs::path path = url.path;
-        if (ranges::distance(path) != 2) {
-            throw_user_error<errc::invalid_remote_url>(
-                "github: URLs should have a path with two segments");
-        }
-        auto fragment = url.fragment;
-        if (!fragment) {
-            throw_user_error<errc::invalid_remote_url>(
-                "github: URLs should have a fragment naming a Git ref to pull from");
-        }
-        auto new_url = fmt::format("https://github.com/{}/archive/{}.tar.gz", url.path, *fragment);
-        return parse_remote_url(new_url);
+        return std::make_shared<github_remote_pkg>(github_remote_pkg::from_url(url));
+    } else if (url.scheme == neo::oper::any_of("dds+http", "http+dds", "dds+https", "https+dds")) {
+        return std::make_shared<dds_http_remote_pkg>(dds_http_remote_pkg::from_url(url));
     } else {
-        throw_user_error<
-            errc::invalid_remote_url>("Unknown scheme '{}' for remote package URL '{}'",
-                                      url.scheme,
-                                      sv);
+        BOOST_LEAF_THROW_EXCEPTION(make_user_error<errc::invalid_remote_url>(
+                                       "Unknown scheme '{}' for remote package listing URL",
+                                       url.scheme),
+                                   url);
     }
 }
+
+any_remote_pkg any_remote_pkg::from_url(const neo::url& url) {
+    auto ptr = do_parse_url(url);
+    return any_remote_pkg(ptr);
+}
+
+neo::url any_remote_pkg::to_url() const {
+    neo_assert(expects, !!_impl, "Accessing an inactive any_remote_pkg");
+    return _impl->to_url();
+}
+
+std::string any_remote_pkg::to_url_string() const { return to_url().to_string(); }
+
+void any_remote_pkg::get_sdist(path_ref dest) const {
+    neo_assert(expects, !!_impl, "Accessing an inactive any_remote_pkg");
+    _impl->get_sdist(dest);
+}

src/dds/pkg/listing.hpp

@@ -1,29 +1,42 @@
 #pragma once
 
-#include "./get/git.hpp"
-#include "./get/http.hpp"
-
 #include <dds/deps.hpp>
 #include <dds/pkg/id.hpp>
-#include <dds/util/glob.hpp>
 
-#include <optional>
+#include <neo/url.hpp>
+
+#include <memory>
 #include <string>
-#include <variant>
+#include <string_view>
 #include <vector>
 
 namespace dds {
 
-using remote_listing_var = std::variant<std::monostate, git_remote_listing, http_remote_listing>;
+class remote_pkg_base;
+
+class any_remote_pkg {
+    std::shared_ptr<const remote_pkg_base> _impl;
+
+    explicit any_remote_pkg(std::shared_ptr<const remote_pkg_base> p)
+        : _impl(p) {}
 
-remote_listing_var parse_remote_url(std::string_view url);
+public:
+    any_remote_pkg();
+    ~any_remote_pkg();
+
+    static any_remote_pkg from_url(const neo::url& url);
+
+    neo::url    to_url() const;
+    std::string to_url_string() const;
+    void        get_sdist(path_ref dest) const;
+};
 
 struct pkg_listing {
     pkg_id                  ident;
-    std::vector<dependency> deps;
-    std::string             description;
+    std::vector<dependency> deps{};
+    std::string             description{};
 
-    remote_listing_var remote;
+    any_remote_pkg remote_pkg{};
 };
 
 }  // namespace dds

src/dds/pkg/listing.test.cpp

@@ -0,0 +1,12 @@
+#include "./listing.hpp"
+
+#include <catch2/catch.hpp>
+
+TEST_CASE("Round trip a URL") {
+    auto listing
+        = dds::any_remote_pkg::from_url(neo::url::parse("http://example.org/package.tar.gz"));
+    CHECK(listing.to_url_string() == "http://example.org/package.tar.gz");
+
+    listing = dds::any_remote_pkg::from_url(neo::url::parse("git://example.org/repo#wat"));
+    CHECK(listing.to_url_string() == "git://example.org/repo#wat");
+}

src/dds/repoman/repoman.cpp

@@ -154,7 +154,7 @@ void repo_manager::import_targz(path_ref tgz_file) {
     dds::pkg_listing info{.ident       = man->id,
                           .deps        = man->dependencies,
                           .description = "[No description]",
-                          .remote      = {}};
+                          .remote_pkg  = {}};
     auto             rel_url = fmt::format("dds:{}", man->id.to_string());
     add_pkg(info, rel_url);
 

src/dds/repoman/repoman.test.cpp

@@ -39,7 +39,7 @@ TEST_CASE_METHOD(tmp_repo, "Add a package directly") {
         .ident       = dds::pkg_id::parse("foo@1.2.3"),
         .deps        = {},
         .description = "Something",
-        .remote      = {},
+        .remote_pkg  = {},
     };
     repo.add_pkg(info, "http://example.com");
     CHECK_THROWS_AS(repo.add_pkg(info, "https://example.com"),

src/dds/sdist/dist.cpp

@@ -151,8 +151,8 @@ temporary_sdist dds::expand_sdist_from_istream(std::istream& is, std::string_vie
 }
 
 temporary_sdist dds::download_expand_sdist_targz(std::string_view url_str) {
-    auto remote  = http_remote_listing::from_url(url_str);
+    auto remote  = http_remote_pkg::from_url(neo::url::parse(url_str));
     auto tempdir = temporary_dir::create();
-    remote.pull_source(tempdir.path());
+    remote.get_raw_directory(tempdir.path());
     return {tempdir, sdist::from_directory(tempdir.path())};
 }

src/dds/sdist/package.cpp

@@ -119,8 +119,7 @@ package_manifest package_manifest::load_from_json5_str(std::string_view content,
         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"));
+                                   boost::leaf::e_file_name{std::string(input_name)});
     }
 }
 
@@ -141,7 +140,6 @@ result<fs::path> package_manifest::find_in_directory(path_ref dirpath) {
                 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()}));
         }
     }
@@ -149,8 +147,7 @@ result<fs::path> package_manifest::find_in_directory(path_ref dirpath) {
     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"));
+                                  DDS_E_ARG(e_missing_file{dirpath / "package.json5"}));
 }
 
 result<package_manifest> package_manifest::load_from_directory(path_ref dirpath) {

src/dds/util/http/pool.cpp

@@ -187,6 +187,11 @@ http_client::~http_client() {
         // We are moved-from
         return;
     }
+    if (_impl->_state != detail::http_client_impl::_state_t::ready
+        && _n_exceptions != std::uncaught_exceptions()) {
+        dds_log(debug, "NOTE: An http_client was dropped due to an exception");
+        return;
+    }
     neo_assert(expects,
                _impl->_state == detail::http_client_impl::_state_t::ready,
                "An http_client object was dropped while in a partial-request state. Did you read "

src/dds/util/http/pool.hpp

@@ -51,6 +51,7 @@ class http_client {
 
     std::weak_ptr<detail::http_pool_impl>     _pool;
     std::shared_ptr<detail::http_client_impl> _impl;
+    int                                       _n_exceptions;
 
     http_client() = default;
 
@@ -62,7 +63,8 @@ class http_client {
 public:
     http_client(http_client&& o)
         : _pool(neo::take(o._pool))
-        , _impl(neo::take(o._impl)) {}
+        , _impl(neo::take(o._impl))
+        , _n_exceptions(std::uncaught_exceptions()) {}
     ~http_client();
 
     void send_head(http_request_params const& params);

src/dds/util/result.hpp

@@ -67,8 +67,6 @@ struct e_parse_error {
 
 #define DDS_E_ARG(...) ([&] { return __VA_ARGS__; })
 
-#define DDS_ERROR_MARKER(Value) DDS_E_ARG(::dds::e_error_marker{Value})
-
 void write_error_marker(std::string_view error) noexcept;
 
 /**

tests/test_repoman.py

@@ -1,7 +1,8 @@
 import pytest
 
-from dds_ci import dds
-from dds_ci.testing.fixtures import DDSWrapper, Project
+from dds_ci.dds import DDSWrapper
+from dds_ci.testing.fixtures import Project
+from dds_ci.testing.http import RepoFixture
 from dds_ci.testing.error import expect_error_marker
 from pathlib import Path
 
@@ -12,7 +13,7 @@ def tmp_repo(tmp_path: Path, dds: DDSWrapper) -> Path:
     return tmp_path
 
 
-def test_bad_pkg_id(dds: DDSWrapper, tmp_repo: Path) -> None:
+def test_error_bad_pkg_id(dds: DDSWrapper, tmp_repo: Path) -> None:
     with expect_error_marker('invalid-pkg-id-str-version'):
         dds.run(['repoman', 'add', tmp_repo, 'foo@bar', 'http://example.com'])
 
@@ -28,6 +29,37 @@ def test_add_simple(dds: DDSWrapper, tmp_repo: Path) -> None:
 
 
 def test_add_github(dds: DDSWrapper, tmp_repo: Path) -> None:
-    dds.run(['repoman', 'add', tmp_repo, 'neo-fun@0.6.0', 'github:vector-of-bool/neo-fun#0.6.0'])
+    dds.run(['repoman', 'add', tmp_repo, 'neo-fun@0.6.0', 'github:vector-of-bool/neo-fun/0.6.0'])
     with expect_error_marker('dup-pkg-add'):
-        dds.run(['repoman', 'add', tmp_repo, 'neo-fun@0.6.0', 'github:vector-of-bool/neo-fun#0.6.0'])
+        dds.run(['repoman', 'add', tmp_repo, 'neo-fun@0.6.0', 'github:vector-of-bool/neo-fun/0.6.0'])
+
+
+def test_add_invalid(dds: DDSWrapper, tmp_repo: Path) -> None:
+    with expect_error_marker('repoman-add-invalid-pkg-url'):
+        dds.run(['repoman', 'add', tmp_repo, 'foo@1.2.3', 'invalid://google.com/lolwut'])
+
+
+def test_error_double_remove(tmp_repo: Path, dds: DDSWrapper) -> None:
+    dds.run([
+        'repoman', '-ltrace', 'add', tmp_repo, 'neo-fun@0.4.0',
+        'https://github.com/vector-of-bool/neo-fun/archive/0.4.0.tar.gz?__dds_strpcmp=1'
+    ])
+    dds.run(['repoman', 'remove', tmp_repo, 'neo-fun@0.4.0'])
+
+    with expect_error_marker('repoman-rm-no-such-package'):
+        dds.run(['repoman', 'remove', tmp_repo, 'neo-fun@0.4.0'])
+
+
+def test_pkg_http(http_repo: RepoFixture, tmp_project: Project) -> None:
+    tmp_project.dds.run([
+        'repoman', '-ltrace', 'add', http_repo.server.root, 'neo-fun@0.4.0',
+        'https://github.com/vector-of-bool/neo-fun/archive/0.4.0.tar.gz?__dds_strpcmp=1'
+    ])
+    tmp_project.dds.repo_add(http_repo.url)
+    tmp_project.package_json = {
+        'name': 'test',
+        'version': '1.2.3',
+        'depends': ['neo-fun@0.4.0'],
+        'namespace': 'test',
+    }
+    tmp_project.build()

tools/dds_ci/proc.py

@@ -1,5 +1,5 @@
 from pathlib import PurePath
-from typing import Iterable, Union, Optional, Iterator
+from typing import Iterable, Union, Optional, Iterator, NoReturn, Sequence
 from typing_extensions import Protocol
 import subprocess
 
@@ -21,8 +21,10 @@ class CommandLine(Protocol):
 
 
 class ProcessResult(Protocol):
+    args: Sequence[str]
     returncode: int
     stdout: bytes
+    stderr: bytes
 
 
 def flatten_cmd(cmd: CommandLine) -> Iterable[str]:
@@ -40,7 +42,14 @@ def flatten_cmd(cmd: CommandLine) -> Iterable[str]:
 
 def run(*cmd: CommandLine, cwd: Optional[Pathish] = None, check: bool = False) -> ProcessResult:
     command = list(flatten_cmd(cmd))
-    return subprocess.run(command, cwd=cwd, check=check)
+    res = subprocess.run(command, cwd=cwd, check=False)
+    if res.returncode and check:
+        raise_error(res)
+    return res
+
+
+def raise_error(proc: ProcessResult) -> NoReturn:
+    raise subprocess.CalledProcessError(proc.returncode, proc.args, output=proc.stdout, stderr=proc.stderr)
 
 
 def check_run(*cmd: CommandLine, cwd: Optional[Pathish] = None) -> ProcessResult:

tools/dds_ci/testing/error.py

@@ -17,9 +17,10 @@ def expect_error_marker(expect: str) -> Iterator[None]:
     try:
         os.environ['DDS_WRITE_ERROR_MARKER'] = str(err_file)
         yield
-        assert False, 'dds subprocess did not raise CallProcessError!'
+        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'
+        assert err_file.exists(), \
+            f'No error marker file was generated, but dds exited with an error (Expected "{expect}")'
         marker = err_file.read_text().strip()
         assert marker == expect, \
             f'dds did not produce the expected error (Expected {expect}, got {marker})'