Fix 'conan remove': output only confirmed entries #14478
Conversation
|
|
Tobulus
force-pushed
the
fix_remove_output
branch
from
baf5943 to
f33a5a7
Compare
There was a problem hiding this comment.
Thanks for the fix @Tobulus, it is looking good.
It would be necessary to add a unit test, to guarantee that this is not broken in the future. Do you think you could add it, or need some help with that?
conan/cli/commands/remove.py
Outdated
|
|
||
| results_pkg_list = PackagesList() | ||
| results = MultiPackagesList() |
There was a problem hiding this comment.
You cannot just recreate the lists here, unconditionally, because this discard the information in the above multi_package_list? A lot of tests in https://ci.conan.io/blue/organizations/jenkins/ConanTestSuitev2/detail/PR-14478/2/pipeline/36 are broken, please have a look.
There was a problem hiding this comment.
the idea was to use multi_package_list only for iterating the stuff possible to be removed by the pattern and collect what was confirmed by the user in a separate list.
I'll take a look at the failing tests.
There was a problem hiding this comment.
Ok, that makes sense, now I see the add_xxx calls and less failing tests.
I am facing a similar problem in my "upload" branch (I am preparing the same fix for the upload command), and I have done something similar.
The issue with this is that it is missing the package_id configuration description. Don't worry about it, it is fine, I will revisit this myself when finishing the upload PR, I might change the API a bit for PackageLists to help with this.
|
Hi! Thanks a lot for your PR. Quick question, as I'm afk right now and cant check by myself: conan upload also has confirmation logic. Does the same issue also arise there? |
Tobulus
force-pushed
the
fix_remove_output
branch
from
f33a5a7 to
c49900f
Compare
Let me take a look, I try to add testing. If I need help, I will ask. Thanks! |
Looks like the same problem there. Maybe we should fix that in a separate PR? |
Yes, I think it is worth doing the upload in a separate PR, as there is some stuff there we need to discuss regarding the UploadAPI and how the |
|
@memsharded The last failing test case actually revealed another problem, if I understand correctly. This outputs that some binaries are deleted, but if you step through it with the debugger, this is not actually done (which now pops up as the output is corrected). The problem seems this part which is not providing elements for iteration. I don't understand the datastructure completly to tell why the iterator is empty ... can you help out? If its helpful I can still provide a test case. As asked above: is it ok to not provide a unit test but a integration test? |
|
Yes, it is possible that some tests are incorrect/buggy too, and those should be fixed. But there are too many tests broken, that typically means that there is something that is actually breaking valid working behavior.
yes, integration tests are usually our preferred choice for most tests. |
2 failed, 1821 passed I think you looked at the old test run? I force pushed one commit which I forgot, so there is no newer commit visible. |
|
Yes, my bad, I hadn't realize about the new commits and new test run, looking better. Sure, you can fix the tests and include them in the PR. |
|
The PR #14512 for fixing the Finally no API has changed, no new methods in PackagesList. Please let me know if you would like a hand with this PR or you can manage to fix the tests. |
I'm out for the next two weeks. So I would be happy for help. |
Changelog: Bugfix: 'conan remove' was outputting all entries in the cache matching the filter not just the once which where confirmed by the user.
Docs: Omit
developbranch, documenting this one.