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 dependency and subdir in repoquery whoneeds #3743

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix dependency and subdir in repoquery whoneeds #3743

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb9bcd9
Fix subdir in repoquery
Hind-M Jan 14, 2025
49c124f
Use string instead of string_view
Hind-M Jan 14, 2025
04e78bd
Code review: use var to store res transformation
Hind-M Jan 17, 2025
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
13 changes: 3 additions & 10 deletions libmamba/src/core/query.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,13 +581,6 @@ namespace mamba
{
return util::split(str, "/", 1).front(); // Has at least one element
}

/** Get subdir from channel name. */
auto get_subdir(std::string_view str) -> std::string
{
return util::split(str, "/").back();
}

}

auto QueryResult::table(std::ostream& out, const std::vector<std::string_view>& columns) const
Expand All @@ -599,7 +592,7 @@ namespace mamba
}

std::vector<mamba::printers::FormattedString> headers;
std::vector<std::string_view> cmds, args;
std::vector<std::string> cmds, args;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the bug as cmds and args were refering to sfmt after it was out of scope, and this was only appearing in osx and windows...

std::vector<mamba::printers::alignment> alignments;
for (auto& col : columns)
{
Expand All @@ -621,7 +614,7 @@ namespace mamba
else if (col.find_first_of(":") == col.npos)
{
headers.emplace_back(col);
Copy link
Member

Choose a reason for hiding this comment

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

Could the current code have problem as well? How about?

Suggested change
headers.emplace_back(col);
headers.emplace_back(std::string(col));

Copy link
Member Author

Choose a reason for hiding this comment

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

Converting col to std::string in cmds.push_back(std::string(col)); is due to changing cmds to std::vector<std::string> (instead of std::vector<std::string_view>).
But it's not needed in headers.emplace_back(col); as headers(std::vector<mamba::printers::FormattedString>) already have a constructor accepting std::string_view.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, my remark was not clear.

I was wondering whether we could also have this problem for all vectors referencing std::string_views directly or indirectly, i.e. could we have the problem with headers again here if the original std::string of std::string_view are freed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since headers is a vector of a struct (FormattedString) containing a string and not a string_view, it should be okay.
The only string_views pointing to a local string being out of scope later (sfmt) were cmds and args which were modified to string now.
So seems ok so far.

cmds.push_back(col);
cmds.push_back(std::string(col));
args.emplace_back("");
}
else
Expand Down Expand Up @@ -670,7 +663,7 @@ namespace mamba
}
else if (cmd == "Subdir")
{
row.emplace_back(get_subdir(pkg.channel));
row.emplace_back(pkg.platform);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was not tested and was returning the channel conda-forge instead of the subdir.

}
else if (cmd == "Depends")
{
Expand Down
6 changes: 6 additions & 0 deletions micromamba/tests/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import os
import platform
import random
import re
import shutil
import string
import subprocess
Expand Down Expand Up @@ -72,6 +73,11 @@ def random_string(n: int = 10) -> str:
return "".join(random.choices(string.ascii_uppercase + string.digits, k=n))


def remove_whitespaces(s: str) -> str:
"""Return the input string with extra whitespaces removed."""
return re.sub(r"\s+", " ", s).strip()


def shell(*args, cwd=os.getcwd(), **kwargs):
umamba = get_umamba(cwd=cwd)
cmd = [umamba, "shell"] + [arg for arg in args if arg]
Expand Down
24 changes: 16 additions & 8 deletions micromamba/tests/test_repoquery.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,15 +146,9 @@ def test_whoneeds_remote(yaml_env: Path):
def test_whoneeds_not_installed_with_channel(yaml_env: Path, with_platform):
if with_platform:
res = helpers.umamba_repoquery(
"whoneeds",
"-c",
"conda-forge",
"xtensor=0.24.5",
"--platform",
"osx-64",
"--json",
"whoneeds", "-c", "conda-forge", "xtensor=0.24.5", "--platform", "osx-64", "--json"
)
assert res["result"]["pkgs"][0]["subdir"] == "osx-64"
assert all("osx-64" in pkg["subdir"] for pkg in res["result"]["pkgs"])
else:
res = helpers.umamba_repoquery("whoneeds", "-c", "conda-forge", "xtensor=0.24.5", "--json")

Expand All @@ -168,6 +162,20 @@ def test_whoneeds_not_installed_with_channel(yaml_env: Path, with_platform):
assert any(x["name"] == "qpot" for x in pkgs)


@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
@pytest.mark.parametrize("spec", ("xtensor", "xtensor=0.24.5"))
def test_whoneeds_not_installed_with_channel_no_json(yaml_env: Path, spec):
res = helpers.umamba_repoquery("whoneeds", "-c", "conda-forge", spec, "--platform", "osx-64")
assert "Name Version Build Depends Channel Subdir" in helpers.remove_whitespaces(res)
if spec == "xtensor=0.24.5": # `Depends` column is empty here (bug?)
assert "cascade 0.1.1 py38h5ce3968_0 conda-forge osx-64" in helpers.remove_whitespaces(res)
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if the current behavior in this case (specifying the spec version) is the appropriate one (need to look into it more with maybe more context).

Copy link
Member

@jjerphan jjerphan Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The first builds of cascade (i.e. the ones of 0.1.1) depend on xtensor, but not on xtensor==0.24.5, here is one of their index.json:

{
  "arch": "x86_64",
  "build": "py310ha6c322b_0",
  "build_number": 0,
  "depends": [
    "boost-cpp >=1.78.0,<1.79.0a0",
    "fmt >=9.1.0,<10.0a0",
    "hdf5 >=1.12.2,<1.12.3.0a0",
    "heyoka >=0.21.0",
    "heyoka >=0.21.0,<0.22.0a0",
    "heyoka.py >=0.21.0",
    "libcxx >=14.0.6",
    "numpy",
    "pybind11-abi 4",
    "python >=3.10,<3.11.0a0",
    "python_abi 3.10.* *_cp310",
    "spdlog >=1.11.0,<1.12.0a0",
    "tbb >=2021.8.0",
    "xtensor",
    "xtensor-blas"
  ],
  "license": "MPL-2.0",
  "name": "cascade",
  "platform": "osx",
  "subdir": "osx-64",
  "timestamp": 1677367283346,
  "version": "0.1.1"
}

Probably those builds of cascade have been done against xtensor==0.24.5.

I think xtensor should be reported in this case, as it is the case with:

micromamba repoquery whoneeds xtensor -c conda-forge --platform osx-64 | grep cascade

 cascade                           0.1.1   py38h5ce3968_0               xtensor                       conda-forge conda-forge
 cascade                           0.1.1   py310ha6c322b_0              xtensor                       conda-forge conda-forge
 cascade                           0.1.1   py39hb1d9d96_0               xtensor                       conda-forge conda-forge

What do you think?

else:
assert (
"cascade 0.1.1 py38h5ce3968_0 xtensor conda-forge osx-64"
in helpers.remove_whitespaces(res)
)
Hind-M marked this conversation as resolved.
Show resolved Hide resolved


@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
def test_whoneeds_tree(yaml_env: Path):
res = helpers.umamba_repoquery("whoneeds", "-c", "conda-forge", "xtensor=0.24.5", "--tree")
Expand Down
Loading