mamba-org / mamba

The Fast Cross-Platform Package Manager
https://mamba.readthedocs.io
BSD 3-Clause "New" or "Revised" License
6.57k stars 343 forks source link

A quick fix for the issue of update env with --prune flag #3243

Closed ZHG2017 closed 3 months ago

ZHG2017 commented 3 months ago

This is a quickfix for the issue #3174

Added install request construction for the update process

Added condition test to avoid conflict for removing and updating the same package at the same time

Added unit test to validate the fix

Hind-M commented 3 months ago

Thanks for the PR. Can you only include in this one the relevant files for the bug fix? The documentation related files should be in another PR (I think reopening #3217 will do). For the Linters failure in the CI, running pre-commit run --all in the mamba source folder locally before committing should fix it (it will most likely modify the non-compliant files to follow the linting rules (defined here), so you should add them and commit them).

ZHG2017 commented 3 months ago

Thanks for the PR. Can you only include in this one the relevant files for the bug fix? The documentation related files should be in another PR (I think reopening #3217 will do). For the Linters failure in the CI, running pre-commit run --all in the mamba source folder locally before committing should fix it (it will most likely modify the non-compliant files to follow the linting rules (defined here), so you should add them and commit them).

Hi @Hind-M , Thank you for the suggestion. Let me figure out how to use hte linter then. And I think I will create a new branch as well as a new PR for the documentation since I messed up some commits.

Hind-M commented 3 months ago

What was the reason the bug is happening, the corresponding test is supposed to check the --prune flag's behavior... Can you add a test reproducing the issue and showing that this PR patch is fixing it?

ZHG2017 commented 3 months ago

Thanks for the PR. Can you only include in this one the relevant files for the bug fix? The documentation related files should be in another PR (I think reopening #3217 will do). For the Linters failure in the CI, running pre-commit run --all in the mamba source folder locally before committing should fix it (it will most likely modify the non-compliant files to follow the linting rules (defined here), so you should add them and commit them).

Hi, I have tried to use the pre-commit but did not find it any where, I checked in the folder .git/hooks/ but there is nothing related to this CI check. I wonder if this CI check only happens on the server

ZHG2017 commented 3 months ago

What was the reason the bug is happening, the corresponding test is supposed to check the --prune flag's behavior... Can you add a test reproducing the issue and showing that this PR patch is fixing it?

Yes, I have just realized that I did not add any unit test to validate my fix, actually I only manually typed some CLI command to verify if the fix works for the concerned issue. The issue happens because the update inside the mamba will check if any package is previously installed in the target environment, if so, the update process will mark it as remove but just after that the internal process will mark the same package to be updated which is seemingly a conflict. Meanwhile, the most recent version of mamba will just ignore any installed package for a given environment file and will not install any package if it is missing in a given environment file.

Hind-M commented 3 months ago

Thanks for the PR. Can you only include in this one the relevant files for the bug fix? The documentation related files should be in another PR (I think reopening #3217 will do). For the Linters failure in the CI, running pre-commit run --all in the mamba source folder locally before committing should fix it (it will most likely modify the non-compliant files to follow the linting rules (defined here), so you should add them and commit them).

Hi, I have tried to use the pre-commit but did not find it any where, I checked in the folder .git/hooks/ but there is nothing related to this CI check. I wonder if this CI check only happens on the server

So if you're working within an environment created from the environment-dev.yml file, pre-commit should be already installed (you can check using mamba list if mamba is the package manager you're using). Otherwise, you can install it: mamba install pre-commit -c conda-forge

Then, you only need to run pre-commit run --all in your git root folder. If you run git status afterwards, you will see the modified files that you'll need to commit.

ZHG2017 commented 3 months ago

Thanks for the PR. Can you only include in this one the relevant files for the bug fix? The documentation related files should be in another PR (I think reopening #3217 will do). For the Linters failure in the CI, running pre-commit run --all in the mamba source folder locally before committing should fix it (it will most likely modify the non-compliant files to follow the linting rules (defined here), so you should add them and commit them).

Hi, I have tried to use the pre-commit but did not find it any where, I checked in the folder .git/hooks/ but there is nothing related to this CI check. I wonder if this CI check only happens on the server

So if you're working within an environment created from the environment-dev.yml file, pre-commit should be already installed (you can check using mamba list if mamba is the package manager you're using). Otherwise, you can install it: mamba install pre-commit -c conda-forge

Then, you only need to run pre-commit run --all in your git root folder. If you run git status afterwards, you will see the modified files that you'll need to commit.

Hi, I have used the pre-commit run --all to format the code of mamba meanwhile I am still working on how to setup a unit test to prove my modification.

Hind-M commented 3 months ago

There is test_classic_spec in test_update.py failing here. The modifications should have broken it. This is due to the solution actions in transaction not empty and so the "message" key is missing.

ZHG2017 commented 3 months ago

There is test_classic_spec in test_update.py failing here. The modifications should have broken it. This is due to the solution actions in transaction not empty and so the "message" key is missing.

Well, I have no idea how to fix this, I tried once using the built executable with my modification and the one built without my modification manually, and I saw no difference for the output. I don't know what can I do to pass this test. By the way, there is a comment # TODO fix this?! above the line which fails, I wonder if the output is not always the same or could have already changed in the past.