Add note and fix downloaded url

mamba-org · Feb 14, 2025 · 1146b9c · 1146b9c
1 parent 868a059
commit 1146b9c
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 34 deletions.
diff --git a/libmamba/include/mamba/core/package_fetcher.hpp b/libmamba/include/mamba/core/package_fetcher.hpp
@@ -101,8 +101,7 @@ namespace mamba
 
         struct CheckSumParams;
 
-        bool is_local_package() const;
-        bool use_explicit_https_url() const;
+        bool use_oci() const;
         const std::string& filename() const;
         std::string channel() const;
         std::string url_path() const;

diff --git a/libmamba/src/core/package_fetcher.cpp b/libmamba/src/core/package_fetcher.cpp
@@ -152,7 +152,7 @@ namespace mamba
                 cb.value()(success.transfer.downloaded_size);
             }
             m_needs_download = false;
-            m_downloaded_url = success.transfer.effective_url;
+            m_downloaded_url = m_package_info.package_url;
             return expected_t<void>();
         };
 
@@ -317,53 +317,43 @@ namespace mamba
     /*******************
      * Private methods *
      *******************/
-    // TODO to be removed if not used
-    bool PackageFetcher::is_local_package() const
-    {
-        return util::starts_with(m_package_info.package_url, "file://");
-    }
 
-    bool PackageFetcher::use_explicit_https_url() const
+    const std::string& PackageFetcher::filename() const
     {
-        // This excludes OCI case, which uses explicitly a "oci://" scheme,
-        // but is resolved later to something starting with `oci_base_url`
-        constexpr std::string_view oci_base_url = "https://pkg-containers.githubusercontent.com/";
-        return util::starts_with(m_package_info.package_url, "https://")
-               && !util::starts_with(m_package_info.package_url, oci_base_url);
+        return m_package_info.filename;
     }
 
-    const std::string& PackageFetcher::filename() const
+    bool PackageFetcher::use_oci() const
     {
-        return m_package_info.filename;
+        constexpr std::string_view oci_scheme = "oci://";
+        return util::starts_with(m_package_info.package_url, oci_scheme);
     }
 
+    // NOTE
+    // In the general case (not fetching from an oci registry),
+    // `channel()` and `url_path()` are concatenated when passed to `HTTPMirror`
+    // and the channel is resolved if needed (using the channel alias).
+    // Therefore, `util::url_concat("", m_package_info.package_url)`
+    // and `util::url_concat(m_package_info.channel, m_package_info.platform,
+    // m_package_info.filename)` should be equivalent, except when an explicit url is used as a spec
+    // with `--override-channels` option.
+    // Hence, we are favoring the first option (returning "" and `m_package_info.package_url`
+    // to be concatenated), valid for all the mentioned cases used with `HTTPMirror`.
+    // In the case of fetching from oci registries (using `OCIMirror`),the actual url
+    // used is built differently, and returning `m_package_info.package_url` is not relevant
+    // (i.e oci://ghcr.io/<mirror>/<channel>/<platform>/<filename>).
     std::string PackageFetcher::channel() const
     {
-        //         if (is_local_package() || use_explicit_https_url())
-        //         {
-        //             // Use explicit url or local package path
-        //             // to fetch package, leaving the channel empty.
-        //             return "";
-        //         }
-        //         return m_package_info.channel;
-        if (util::starts_with(m_package_info.package_url, "oci://"))
+        if (use_oci())
         {
             return m_package_info.channel;
         }
         return "";
     }
 
-    // TODO to rename, second_part_url?
     std::string PackageFetcher::url_path() const
     {
-        //         if (is_local_package() || use_explicit_https_url())
-        //         {
-        //             // Use explicit url or local package path
-        //             // to fetch package.
-        //             return m_package_info.package_url;
-        //         }
-        //         return util::concat(m_package_info.platform, '/', m_package_info.filename);
-        if (util::starts_with(m_package_info.package_url, "oci://"))
+        if (use_oci())
         {
             return util::concat(m_package_info.platform, '/', m_package_info.filename);
         }

diff --git a/micromamba/tests/test_create.py b/micromamba/tests/test_create.py
@@ -1241,7 +1241,7 @@ def test_create_with_explicit_url(tmp_home, tmp_root_prefix, tmp_path, spec):
         assert pkgs[0]["channel"] == "https://conda.anaconda.org/conda-forge"
 
 
-def test_create_with_from_mirror(tmp_home, tmp_root_prefix, tmp_path):
+def test_create_from_mirror(tmp_home, tmp_root_prefix, tmp_path):
     """Attempts to install a package using an explicit channel/mirror."""
     empty_root_prefix = tmp_path / "empty-root-create-from-mirror"
     env_name = "env-create-from-mirror"