-
Notifications
You must be signed in to change notification settings - Fork 371
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
Fix micromamba env export
to get channel name instead of full url
#2260
Conversation
Hmm... is this a special case in the Conda source code? If so, could you post a link to the piece of code that does it? I'm curious to see how exactly it is implemented. |
Actually, I just run
And looking at the Conda source code, I found this: |
I'd be interested if Conda always prints the name of the channel that has been used to install (like |
micromamba/src/env.cpp
Outdated
if (chan_name == "pkgs/main") | ||
{ | ||
channels.insert("defaults"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like using either pkgs/main
or defaults
works so I would be conservative and say we do not need to change? I'm no expert here. What do you think?
In the refactorign reflection, this should go in Channel
, eventually through a new method name like cannonical_name
so that we don't manage edge cases everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we could leave pkgs/main
then (less prone to bugs and avoid edge cases). And yes, I agree that we should use something like canonical_name
like Conda does IIRC.
@@ -120,7 +120,7 @@ def test_env_export(self): | |||
create("", "-n", env_name, "-f", spec_file) | |||
ret = yaml.safe_load(run_env("export", "-n", env_name)) | |||
assert ret["name"] == env_name | |||
assert set(ret["channels"]) == {"https://conda.anaconda.org/conda-forge"} | |||
assert set(ret["channels"]) == {"conda-forge"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do add that the defaults
naming, we should also add a test for it, so that we do not loose it through a refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
Did anyone check this? |
Nope. I've looked a bit into their code base but it wasn't obvious to spot and have the full picture. I would probably spend more time on it later if no one else checks it or has already some prior background to narrow the search. |
OK! Should we wait until we have a definite answer? I guess solving this issue isn't urgent, and if we change behavior twice that might be confusing to users. |
Yes, that seems reasonable! |
I had a look:
Now with
IIUC we should always print the multichannel name in |
Interestingly, the order of channels is different with Micromamba. Is that a bug? cc @wolfv |
Thanks!
But I still find the general logic not that clear:
But anyway, I think that any other changes should be done in other PRs since they would be considered as additional features (handle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!
Fix #2235
In case of
pkgs/main
,defaults
is used to be consistent with conda.