Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix channel and base_url in list cmd #3488

Merged
merged 3 commits into from
Oct 2, 2024
Merged

Fix channel and base_url in list cmd #3488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1221fed
Fix channel and base_url in list cmd
Hind-M Sep 30, 2024
8b1f849
Add func
Hind-M Oct 1, 2024
105e18e
Fix oci tests
Hind-M Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/source/developer_zone/changes-2.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,5 +185,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
─────────────────────────────────────────────────────────────────────────────────────
pandoc 3.2 ha770c72_0 https://pkg-containers.githubusercontent.com/ghcr1/blobs
51 changes: 42 additions & 9 deletions libmamba/src/api/list.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "mamba/core/channel_context.hpp"
#include "mamba/core/context.hpp"
#include "mamba/core/prefix_data.hpp"
#include "mamba/util/string.hpp"

namespace mamba
{
Expand All @@ -34,6 +35,31 @@ namespace mamba
return a.name < b.name;
}

// This is more or less an implementation of `util::rstrip` specific to this use case
// (for printing purposes), but using `std::string` instead of `std::string_view`
// `util::rstrip` is not used here because it leads to an UB,
// `using non owned/tracked strings from Channel (& co) and PackageInfo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure to understand why util::rstrip leads to UB. Is it still the case if you assign its result to a std::string?
Anyway, if we keep this implementation, we should open an issue to try to fix the UB and use a single implementation of rstrip.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, since we use it recursively, it will imply doing a lot std::string conversions?
Not sure if doing this is cleaner than just having a string version for rstrip?
Anyway, I agree that we should definitely reevaluate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant using the funciton degined in util::string and assign the final result of the recursive call to a string. The function returns temporary string_view passed by copies, so there hsould not be any dangling temporary issues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well doing:

std::string res = std::string(util::rstrip(util::rstrip(util::rstrip(util::rstrip(full_str, filename), "/"), platform), "/"));

instead of using the reimplemented rstrip in strip_from_filename_and_platform definitely leads to UB, if that's what you meant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep that's what I meant :( OK let's merge as is and open an issue to fix that UB later

std::string rstrip(const std::string& full_str, const std::string& sub_str)
{
if (util::ends_with(full_str, sub_str))
{
return full_str.substr(0, full_str.length() - sub_str.length());
}
else
{
return full_str;
}
}

std::string strip_from_filename_and_platform(
const std::string& full_str,
const std::string& filename,
const std::string& platform
)
{
return rstrip(rstrip(rstrip(rstrip(full_str, filename), "/"), platform), "/");
}

void list_packages(
const Context& ctx,
std::string regex,
Expand Down Expand Up @@ -73,17 +99,20 @@ namespace mamba

if (regex.empty() || std::regex_search(pkg_info.name, spec_pat))
{
auto display_channels = channel_context.make_channel(pkg_info.channel);
auto url_channels = channel_context.make_channel(pkg_info.package_url);
assert(display_channels.size() == 1); // A URL can only resolve to one
// channel
assert(url_channels.size() == 1); // A URL can only resolve to one channel
obj["base_url"] = url_channels.front().url().str(
specs::CondaURL::Credentials::Remove
auto channels = channel_context.make_channel(pkg_info.package_url);
assert(channels.size() == 1); // A URL can only resolve to one channel
obj["base_url"] = strip_from_filename_and_platform(
channels.front().url().str(specs::CondaURL::Credentials::Remove),
pkg_info.filename,
pkg_info.platform
);
obj["build_number"] = pkg_info.build_number;
obj["build_string"] = pkg_info.build_string;
obj["channel"] = display_channels.front().display_name();
obj["channel"] = strip_from_filename_and_platform(
channels.front().display_name(),
pkg_info.filename,
pkg_info.platform
);
obj["dist_name"] = pkg_info.str();
obj["name"] = pkg_info.name;
obj["platform"] = pkg_info.platform;
Expand Down Expand Up @@ -119,7 +148,11 @@ namespace mamba
{
auto channels = channel_context.make_channel(package.second.channel);
assert(channels.size() == 1); // A URL can only resolve to one channel
formatted_pkgs.channel = channels.front().display_name();
formatted_pkgs.channel = strip_from_filename_and_platform(
channels.front().display_name(),
package.second.filename,
package.second.platform
);
}
packages.push_back(formatted_pkgs);
}
Expand Down
20 changes: 8 additions & 12 deletions micromamba/tests/test_create.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,9 +1231,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"].startswith(
"https://pkg-containers.githubusercontent.com/ghcr1/blobs/pandoc"
)
assert pkg["base_url"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs"
assert pkg["channel"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could set this to something else like oci or ghcr?
But that could be done in another PR once it's all sorted out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah let's fix the bugs for now;, and figure out what to do for OCI in a dedicated PR.



@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
Expand Down Expand Up @@ -1261,16 +1260,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"].startswith(
"https://pkg-containers.githubusercontent.com/ghcr1/blobs/xtensor"
)
and package["base_url"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs"
and package["channel"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs"
for package in packages
)
assert any(
package["name"] == "xtl"
and package["base_url"].startswith(
"https://pkg-containers.githubusercontent.com/ghcr1/blobs/xtl"
)
and package["base_url"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs"
and package["channel"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs"
for package in packages
)

Expand Down Expand Up @@ -1304,9 +1301,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"].startswith(
"https://pkg-containers.githubusercontent.com/ghcr1/blobs/_go_select"
)
assert pkg["base_url"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs"
assert pkg["channel"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs"


@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
Expand Down
4 changes: 4 additions & 0 deletions micromamba/tests/test_list.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ def test_list(tmp_home, tmp_root_prefix, tmp_env_name, tmp_xtensor_env, env_sele
names = [i["name"] for i in res]
assert "xtensor" in names
assert "xtl" in names
assert all(
i["channel"] == "conda-forge" and i["base_url"] == "https://conda.anaconda.org/conda-forge"
for i in res
)


@pytest.mark.parametrize("quiet_flag", ["", "-q", "--quiet"])
Expand Down
Loading