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

Fix support for the PackageType.VIRTUAL_SYSTEM enum #2923

Merged

Conversation

ogrisel
Copy link
Contributor

@ogrisel ogrisel commented Oct 19, 2023

While trying to use conda-lock with the latest conda and mamba on scikit-learn, I got errors due to the use of recently introduced conda.models.enums.PackageType.

Here is a PR to attempt to fix it while preserving backward compat.

Probably fixes: #2129, #1794, #1770.

@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 19, 2023

Note: since some of the linked issues are really old. The commit in conda that introduced the PackageType.VIRTUAL_SYSTEM enum is 4 years old. So maybe it has always been an enum rather than a string and the backward compat protection in my PR is useless:

@ogrisel ogrisel changed the title Fix support for the new PackageType.VIRTUAL_SYSTEM enum Fix support for the PackageType.VIRTUAL_SYSTEM enum Oct 19, 2023
mamba/mamba/utils.py Outdated Show resolved Hide resolved
@jonashaag
Copy link
Collaborator

Thanks for the PR! I haven't reviewed the changes so far but can you add tests for that reproduce the bugs (in the unfixed version)?

@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 19, 2023

I am not sure how to craft a minimal reproducer from the linked issues that wouldn't require access to the network and I am not familiar with how you write tests for this project.

Would you prefer a minimal unit test? Or an integration test, for instance for the case reported in #1770 using check_output in test_all.py?

@jonashaag
Copy link
Collaborator

Any test is fine really. We have lots of tests that need the network.

@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 19, 2023

I am having trouble setting up a dev environment for mamba following the instructions found here: https://mamba.readthedocs.io/en/latest/developer_zone/build_locally.html:

$ mamba env create --name mambadev --file ./mamba/environment-dev.yml
...
Installing collected packages: securesystemslib
Successfully installed securesystemslib-0.30.0

done
Unable to install package for sel(win).

Please double check and ensure your dependencies file has
the correct spelling.  You might also try installing the
conda-env-sel(win) package to see if provides the required
installer.

EDIT: will comment out line - sel(win): winreg and try again. But later.

@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 19, 2023

@jonashaag I have pushed a test. I am could not get my test setup to actually use my dev mamba/mamba/utils.py (even when following the instructions of the contributor guide), it would still use the mamba/mamba/utils.py from my miniforge3/lib/python3.10/site-packages for some reason.

Still I think the PR contents is working as intended and hopefully your CI can confirm this.

@jonashaag
Copy link
Collaborator

Perfect thank you! Would you mind also adding a Linux test? Could be a copy paste or a parameteized test

@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 19, 2023

Could be a copy paste or a parameterized test

@jonashaag I updated the test to parameterize it for Linux and Windows (in addition to macOS).

@jonashaag jonashaag merged commit d0b3ea0 into mamba-org:main Oct 20, 2023
15 of 16 checks passed
@AntoinePrv
Copy link
Member

Thanks for this ! It should also be backported to the 1.x branch if you want to see it a 1.5.3 release (not planned for now).

@JohanMabille
Copy link
Member

JohanMabille commented Oct 20, 2023

Thanks for the fix @ogrisel ! We should fix the build before backporting it.

@ogrisel ogrisel deleted the fix-package_type-virtual_system-enum branch October 20, 2023 08:29
@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 20, 2023

Indeed, I hardcoded the use of bash because the tests was originally written only for macOS. Now that it was parameterized for windows, we need to unhardcode the shell. I will do a follow-up PR.

@ogrisel
Copy link
Contributor Author

ogrisel commented Oct 20, 2023

@jonashaag @AntoinePrv @JohanMabille hopefully #2924 should fix the Windows CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with package __osx when removing package ipykernel on macOS
4 participants