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.

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

Respect subdir in match spec #2300

Merged
merged 4 commits into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
Respect subdirectory in spec
  • Loading branch information
ThomasBlauthQC authored and jonashaag committed Jul 23, 2023
commit 00347802bcaf53d6fb650343a6d20510f7f4aa7d
24 changes: 23 additions & 1 deletion libmamba/src/core/pool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,25 @@ namespace mamba
return false;
}

bool
subdir_match(std::string candidate_url, std::string needle_spec)
{
ThomasBlauthQC marked this conversation as resolved.
Show resolved Hide resolved
ThomasBlauthQC marked this conversation as resolved.
Show resolved Hide resolved
std::string needle_channel = split(needle_spec, ":", 1)[0];
if (!contains(needle_channel, "/")){
ThomasBlauthQC marked this conversation as resolved.
Show resolved Hide resolved
return true;
}
std::string needle_subdir = rsplit(needle_channel, "/", 1)[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

@jonashaag, concerning #2431, I think this is the problematic part. It assumes that if the channel name contains a slash it's because it is specifying a subdir, but pkgs/main is a valid channel name (whether we like it or not 😬). So I'd say that we add some logic after this to check whether needle_subdir is indeed a known subdir, and ignore it otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds correct. That's really unfortunate :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there any known channels with / other than the official Anaconda ones? I wonder if we could get away with hardcoding them if the subdir lookup thing is too complicated to implement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jaimergp I did some investigation. This part of the code is never run, I think because this PR collided with some other refactorings that were done. So while incorrect, it's not the cause of the bug.

Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL KNOWN_PLATFORMS in channel.cpp


std::string candidate_subdir = rsplit(candidate_url, "/", 1)[1];

if (candidate_subdir == needle_subdir){
return true;
}
throw std::runtime_error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this is the right way to handle this case here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure myself, open for any suggestions!

fmt::format("The package \"{}\" is not available for the specified platform", needle_spec
));
}

/**
* Add function to handle matchspec while parsing is done by libsolv.
*/
Expand Down Expand Up @@ -197,9 +216,12 @@ namespace mamba
auto const url = std::string(repo.url());
if (channel_match(channel_context, channel_context.make_channel(url), c))
{
selected_pkgs.push_back(s.id());
if (subdir_match(url, ms.spec)){
selected_pkgs.push_back(s.id());
}
}
}

);
solv::StringId const repr_id = pool.add_string(repr);
::Id const offset = pool_queuetowhatprovides(pool.raw(), selected_pkgs.raw());
Expand Down
44 changes: 44 additions & 0 deletions mamba/tests/test_all.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,24 @@ def test_create_dry_run(experimental, use_json, tmpdir):
assert not env_dir.check()


def test_create_subdir(tmpdir):
env_dir = tmpdir / str(uuid.uuid1())

try:
output = subprocess.check_output(
"mamba create --dry-run --json "
f"-p {env_dir} "
f"conda-forge/noarch::xtensor",
shell=True,
)
except subprocess.CalledProcessError as e:
result = json.loads(e.output)
assert result["error"] == (
"RuntimeError('The package \"conda-forge/noarch::xtensor\" is"
" not available for the specified platform')"
)


def test_create_files(tmpdir):
"""Check that multiple --file arguments are respected."""
(tmpdir / "1.txt").write(b"a")
Expand Down Expand Up @@ -218,6 +236,32 @@ def test_multi_channels(config_file, tmpdir):
assert pkg["base_url"] == "https://conda.anaconda.org/conda-forge"


@pytest.mark.parametrize("config_file", [multichannel_config], indirect=["config_file"])
def test_multi_channels_with_subdir(config_file, tmpdir):
# we need to create a config file first
call_env = os.environ.copy()
call_env["CONDA_PKGS_DIRS"] = str(tmpdir / random_string())
try:
output = subprocess.check_output(
[
"mamba",
"create",
"-n",
"multichannels_with_subdir",
"conda-forge2/noarch::xtensor",
"--dry-run",
"--json",
],
env=call_env,
)
except subprocess.CalledProcessError as e:
result = json.loads(e.output)
assert result["error"] == (
"RuntimeError('The package \"conda-forge2/noarch::xtensor\" is"
" not available for the specified platform')"
)


def test_update_py():
# check updating a package when a newer version
if platform.system() == "Windows":
Expand Down
15 changes: 15 additions & 0 deletions micromamba/tests/test_create.py
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,21 @@ def test_spec_with_channel(tmp_home, tmp_root_prefix, tmp_path):
assert link["version"].startswith("0.22.")


def test_spec_with_channel_and_subdir():
Copy link
Contributor

Choose a reason for hiding this comment

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

We should extend this test to make sure pkgs/main::package_name works correctly.

env_name = "myenv"
try:
res = helpers.create(
"-n", env_name, "conda-forge/noarch::xtensor", "--dry-run"
)
except subprocess.CalledProcessError as e:
assert (
e.stderr.decode()
== (
"critical libmamba The package \"conda-forge/noarch::xtensor\" is "
"not available for the specified platform\n"
)
)

@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
def test_channel_nodefaults(tmp_home, tmp_root_prefix, tmp_path):
rc_file = tmp_path / "rc.yaml"
Expand Down