Respect subdir in match spec #2300
Conversation
libmamba/src/core/solver.cpp
Outdated
| // This assumes, that explicitly specified subdirectories | ||
| // (e.g. conda-forge/linux-64::mamba) result in the platform member of needle | ||
| // only containing one entry. When the subdirectory is omitted, the paltfom | ||
| // member instead contains implicit <platform> and noarch entries. |
There was a problem hiding this comment.
What's a "paltfom"? 😛
Also I find this a little easier to parse:
| // This assumes, that explicitly specified subdirectories | |
| // (e.g. conda-forge/linux-64::mamba) result in the platform member of needle | |
| // only containing one entry. When the subdirectory is omitted, the paltfom | |
| // member instead contains implicit <platform> and noarch entries. | |
| // This assumes that explicitly specified subdirectories | |
| // (e.g. conda-forge/linux-64::mamba) result in needle.platforms.size() == 1 (e.g. needle.platforms() == {"linux-64"}). If the subdirectory is omitted from the spec, needle.platforms() is {"noarch", "<platform>"}. |
There was a problem hiding this comment.
I also like this! Not sure how useful the comment is in general, or if that is "common knowledge". I just wanted to make the == 1 comparison come a little less out of the blue.
|
|
||
| template <typename PkgRange> | ||
| auto create_mrepo_with_repo_meta(PkgRange const& packages, | ||
| std::string channel_name, | ||
| std::string subdir) | ||
| { | ||
| auto const tmp_dir = dir_guard(fs::temp_directory_path() / "mamba/tests" |
There was a problem hiding this comment.
test_satisfiability is not meant for testing this. Would you mind movinf these new tests to a test_solver.cpp, possibly moving test utility code to a common header.
There was a problem hiding this comment.
Sure, that makes a lot of sense!
|
@ThomasBlauthQC do you plan on following up on this? |
|
@AntoinePrv, @jonashaag, I'm very sorry for the delay; this fell through the cracks way too many times! I adapted the pr, the string parsing feels very hacky though... Let me know what you think! |
libmamba/src/core/pool.cpp
Outdated
| if (candidate_subdir == needle_subdir){ | ||
| return true; | ||
| } | ||
| throw std::runtime_error( |
There was a problem hiding this comment.
Not sure if this is the right way to handle this case here
There was a problem hiding this comment.
I'm not sure myself, open for any suggestions!
|
Thanks for updating @ThomasBlauthQC! |
@ThomasBlauthQC thanks for working on this, however I am currently doing significant rework of the repo/pool/solver/transcaction and I would prefer a different solution that will only be possible later on. Is this feature blocking for you? Or can it wait a couple more months? |
|
Would be nice to have this earlier than in a couple of months. |
Nice! I was wondering if this could ultimately end up in libsolv but I don't understand libsolv well enough to judge if that is possible. Looking forward to the restructuring 🙂 |
|
@AntoinePrv if it's fine with you I'd like to merge this next week. |
| // Subdir not specified, so any subdir is fine | ||
| return true; | ||
| } | ||
| std::string needle_subdir = rsplit(needle_channel, "/", 1)[1]; |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
The logic implemented in conda can be found at https://github.com/conda/conda/blob/a9b1838f059501528e6f48c35972fd5f379dec0a/conda/models/match_spec.py#L188-L199
There was a problem hiding this comment.
Sounds correct. That's really unfortunate :/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
TIL KNOWN_PLATFORMS in channel.cpp
| @@ -613,6 +613,17 @@ 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(): | |||
There was a problem hiding this comment.
We should extend this test to make sure pkgs/main::package_name works correctly.
This PR addresses #2041 and #2226.
If a package can't be found in the specified subdir, the output now looks like this.
I'm not too proficient in C++, so any feedback is highly appreciated! 🙂
cc @jonashaag