From 8824be110587374b7e4ac6744f7f8921bab26961 Mon Sep 17 00:00:00 2001 From: Hind-M <70631848+Hind-M@users.noreply.github.com> Date: Fri, 21 Feb 2025 10:48:45 +0100 Subject: [PATCH] Use correct `url` in metadata and mirrors (#3816) --- docs/source/developer_zone/changes-2.0.rst | 4 +- .../include/mamba/core/package_fetcher.hpp | 3 +- libmamba/include/mamba/core/subdirdata.hpp | 3 + libmamba/src/core/package_fetcher.cpp | 50 ++++++----- libmamba/src/core/subdirdata.cpp | 8 +- libmamba/tests/CMakeLists.txt | 1 + .../tests/src/core/test_package_fetcher.cpp | 89 +++++++++++++++++++ micromamba/tests/test_create.py | 48 +++++++--- 8 files changed, 165 insertions(+), 41 deletions(-) create mode 100644 libmamba/tests/src/core/test_package_fetcher.cpp diff --git a/docs/source/developer_zone/changes-2.0.rst b/docs/source/developer_zone/changes-2.0.rst index 610e7c646..93739714c 100644 --- a/docs/source/developer_zone/changes-2.0.rst +++ b/docs/source/developer_zone/changes-2.0.rst @@ -198,5 +198,5 @@ Listing packages in the created ``pandoc_from_oci`` environment: $ micromamba list -n pandoc_from_oci Name Version Build Channel - ───────────────────────────────────────────────────────────────────────────────────── - pandoc 3.2 ha770c72_0 https://pkg-containers.githubusercontent.com/ghcr1/blobs + ────────────────────────────────────────── + pandoc 3.2 ha770c72_0 conda-forge diff --git a/libmamba/include/mamba/core/package_fetcher.hpp b/libmamba/include/mamba/core/package_fetcher.hpp index faaec08f2..21c3e0cf9 100644 --- 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/include/mamba/core/subdirdata.hpp b/libmamba/include/mamba/core/subdirdata.hpp index 91ea6b092..fd748923d 100644 --- a/libmamba/include/mamba/core/subdirdata.hpp +++ b/libmamba/include/mamba/core/subdirdata.hpp @@ -155,6 +155,7 @@ namespace mamba ); std::string repodata_url_path() const; + const std::string& repodata_full_url() const; void load(MultiPackageCache& caches, ChannelContext& channel_context, const specs::Channel& channel); @@ -185,6 +186,8 @@ namespace mamba std::string m_solv_fn; bool m_is_noarch; + std::string m_full_url; + SubdirMetadata m_metadata; std::unique_ptr m_temp_file; const Context* p_context; diff --git a/libmamba/src/core/package_fetcher.cpp b/libmamba/src/core/package_fetcher.cpp index 80f21d61f..154c96601 100644 --- 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(); }; @@ -317,45 +317,47 @@ namespace mamba /******************* * Private methods * *******************/ - bool PackageFetcher::is_local_package() const - { - return util::starts_with(m_package_info.package_url, "file://"); - } - - bool PackageFetcher::use_explicit_https_url() 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); - } const std::string& PackageFetcher::filename() const { return m_package_info.filename; } + bool PackageFetcher::use_oci() const + { + 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////). std::string PackageFetcher::channel() const { - if (is_local_package() || use_explicit_https_url()) + if (use_oci()) { - // Use explicit url or local package path - // to fetch package, leaving the channel empty. - return ""; + return m_package_info.channel; } - return m_package_info.channel; + return ""; } std::string PackageFetcher::url_path() const { - if (is_local_package() || use_explicit_https_url()) + if (use_oci()) { - // 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); } - return util::concat(m_package_info.platform, '/', m_package_info.filename); + return m_package_info.package_url; } const std::string& PackageFetcher::url() const diff --git a/libmamba/src/core/subdirdata.cpp b/libmamba/src/core/subdirdata.cpp index 67d71211a..17281cecf 100644 --- a/libmamba/src/core/subdirdata.cpp +++ b/libmamba/src/core/subdirdata.cpp @@ -580,6 +580,7 @@ namespace mamba , m_is_noarch(platform == "noarch") , p_context(&(ctx)) { + m_full_url = util::url_concat(channel.url().str(), "/", repodata_url_path()); assert(!channel.is_package()); m_forbid_cache = (channel.mirror_urls().size() == 1u) && util::starts_with(channel.url().str(), "file://"); @@ -591,6 +592,11 @@ namespace mamba return util::concat(m_platform, "/", m_repodata_fn); } + const std::string& SubdirData::repodata_full_url() const + { + return m_full_url; + } + void SubdirData::load(MultiPackageCache& caches, ChannelContext& channel_context, const specs::Channel& channel) { @@ -773,7 +779,7 @@ namespace mamba } else { - return finalize_transfer(SubdirMetadata::HttpMetadata{ success.transfer.effective_url, + return finalize_transfer(SubdirMetadata::HttpMetadata{ repodata_full_url(), success.etag, success.last_modified, success.cache_control }); diff --git a/libmamba/tests/CMakeLists.txt b/libmamba/tests/CMakeLists.txt index 841d9b2fb..371b1b410 100644 --- a/libmamba/tests/CMakeLists.txt +++ b/libmamba/tests/CMakeLists.txt @@ -85,6 +85,7 @@ set( src/core/test_invoke.cpp src/core/test_lockfile.cpp src/core/test_output.cpp + src/core/test_package_fetcher.cpp src/core/test_pinning.cpp src/core/test_progress_bar.cpp src/core/test_shell_init.cpp diff --git a/libmamba/tests/src/core/test_package_fetcher.cpp b/libmamba/tests/src/core/test_package_fetcher.cpp new file mode 100644 index 000000000..e98f17f88 --- /dev/null +++ b/libmamba/tests/src/core/test_package_fetcher.cpp @@ -0,0 +1,89 @@ +// Copyright (c) 2025, QuantStack and Mamba Contributors +// +// Distributed under the terms of the BSD 3-Clause License. +// +// The full license is in the file LICENSE, distributed with this software. + +#include + +#include "mamba/core/context.hpp" +#include "mamba/core/package_fetcher.hpp" + +namespace +{ + using namespace mamba; + + TEST_CASE("build_download_request") + { + auto ctx = Context(); + MultiPackageCache package_caches{ ctx.pkgs_dirs, ctx.validation_params }; + + SECTION("From conda-forge") + { + static constexpr std::string_view url = "https://conda.anaconda.org/conda-forge/linux-64/pkg-6.4-bld.conda"; + auto pkg_info = specs::PackageInfo::from_url(url).value(); + + PackageFetcher pkg_fetcher{ pkg_info, package_caches }; + REQUIRE(pkg_fetcher.name() == pkg_info.name); + + auto req = pkg_fetcher.build_download_request(); + // Should correspond to package name + REQUIRE(req.name == pkg_info.name); + // Should correspond to PackageFetcher::channel() + REQUIRE(req.mirror_name == ""); + // Should correspond to PackageFetcher::url_path() + REQUIRE(req.url_path == url); + } + + SECTION("From some mirror") + { + static constexpr std::string_view url = "https://repo.prefix.dev/emscripten-forge-dev/emscripten-wasm32/cpp-tabulate-1.5.0-h7223423_2.tar.bz2"; + auto pkg_info = specs::PackageInfo::from_url(url).value(); + + PackageFetcher pkg_fetcher{ pkg_info, package_caches }; + REQUIRE(pkg_fetcher.name() == pkg_info.name); + + auto req = pkg_fetcher.build_download_request(); + // Should correspond to package name + REQUIRE(req.name == pkg_info.name); + // Should correspond to PackageFetcher::channel() + REQUIRE(req.mirror_name == ""); + // Should correspond to PackageFetcher::url_path() + REQUIRE(req.url_path == url); + } + + SECTION("From local file") + { + static constexpr std::string_view url = "file:///home/wolfv/Downloads/xtensor-0.21.4-hc9558a2_0.tar.bz2"; + auto pkg_info = specs::PackageInfo::from_url(url).value(); + + PackageFetcher pkg_fetcher{ pkg_info, package_caches }; + REQUIRE(pkg_fetcher.name() == pkg_info.name); + + auto req = pkg_fetcher.build_download_request(); + // Should correspond to package name + REQUIRE(req.name == pkg_info.name); + // Should correspond to PackageFetcher::channel() + REQUIRE(req.mirror_name == ""); + // Should correspond to PackageFetcher::url_path() + REQUIRE(req.url_path == url); + } + + SECTION("From oci") + { + static constexpr std::string_view url = "oci://ghcr.io/channel-mirrors/conda-forge/linux-64/xtensor-0.25.0-h00ab1b0_0.conda"; + auto pkg_info = specs::PackageInfo::from_url(url).value(); + + PackageFetcher pkg_fetcher{ pkg_info, package_caches }; + REQUIRE(pkg_fetcher.name() == pkg_info.name); + + auto req = pkg_fetcher.build_download_request(); + // Should correspond to package name + REQUIRE(req.name == pkg_info.name); + // Should correspond to PackageFetcher::channel() + REQUIRE(req.mirror_name == "oci://ghcr.io/channel-mirrors/conda-forge"); + // Should correspond to PackageFetcher::url_path() + REQUIRE(req.url_path == "linux-64/xtensor-0.25.0-h00ab1b0_0.conda"); + } + } +} diff --git a/micromamba/tests/test_create.py b/micromamba/tests/test_create.py index ba6723988..dd897c83b 100644 --- a/micromamba/tests/test_create.py +++ b/micromamba/tests/test_create.py @@ -1304,13 +1304,10 @@ def test_create_with_non_existing_subdir(tmp_home, tmp_root_prefix, tmp_path): "https://conda.anaconda.org/conda-forge/linux-64/abacus-3.2.4-hb6c440e_0.conda", ], ) -def test_create_with_explicit_url(tmp_home, tmp_root_prefix, tmp_path, spec): +def test_create_with_explicit_url(tmp_home, tmp_root_prefix, spec): """Attempts to install a package using an explicit url.""" - empty_root_prefix = tmp_path / "empty-root-create-from-explicit-url" env_name = "env-create-from-explicit-url" - os.environ["MAMBA_ROOT_PREFIX"] = str(empty_root_prefix) - res = helpers.create( spec, "--no-env", "-n", env_name, "--override-channels", "--json", default_channel=False ) @@ -1337,6 +1334,33 @@ 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_from_mirror(tmp_home, tmp_root_prefix): + """ + Attempts to install a package using an explicit channel/mirror. + Non-regression test for https://github.com/mamba-org/mamba/issues/3804 + """ + env_name = "env-create-from-mirror" + + res = helpers.create( + "cpp-tabulate", + "-n", + env_name, + "-c", + "https://repo.prefix.dev/emscripten-forge-dev", + "--platform=emscripten-wasm32", + "--json", + default_channel=False, + ) + assert res["success"] + + assert any( + package["name"] == "cpp-tabulate" + and package["channel"] == "https://repo.prefix.dev/emscripten-forge-dev" + and package["subdir"] == "emscripten-wasm32" + for package in res["actions"]["LINK"] + ) + + @pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True) def test_create_with_multiple_files(tmp_home, tmp_root_prefix, tmpdir): env_name = "myenv" @@ -1458,8 +1482,8 @@ def test_create_from_oci_mirrored_channels(tmp_home, tmp_root_prefix, tmp_path, assert pkg["name"] == "pandoc" if spec == "pandoc=3.1.13": assert pkg["version"] == "3.1.13" - assert pkg["base_url"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" - assert pkg["channel"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" + assert pkg["base_url"] == "oci://ghcr.io/channel-mirrors/conda-forge" + assert pkg["channel"] == "oci://ghcr.io/channel-mirrors/conda-forge" @pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True) @@ -1487,14 +1511,14 @@ def test_create_from_oci_mirrored_channels_with_deps(tmp_home, tmp_root_prefix, assert len(packages) > 2 assert any( package["name"] == "xtensor" - and package["base_url"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" - and package["channel"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" + and package["base_url"] == "oci://ghcr.io/channel-mirrors/conda-forge" + and package["channel"] == "oci://ghcr.io/channel-mirrors/conda-forge" for package in packages ) assert any( package["name"] == "xtl" - and package["base_url"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" - and package["channel"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" + and package["base_url"] == "oci://ghcr.io/channel-mirrors/conda-forge" + and package["channel"] == "oci://ghcr.io/channel-mirrors/conda-forge" for package in packages ) @@ -1528,8 +1552,8 @@ def test_create_from_oci_mirrored_channels_pkg_name_mapping( assert len(packages) == 1 pkg = packages[0] assert pkg["name"] == "_go_select" - assert pkg["base_url"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" - assert pkg["channel"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" + assert pkg["base_url"] == "oci://ghcr.io/channel-mirrors/conda-forge" + assert pkg["channel"] == "oci://ghcr.io/channel-mirrors/conda-forge" @pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)