Skip to content

Handle malformed version metadata error and skip the package #13513

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sepehr-rs
Copy link
Contributor

@sepehr-rs sepehr-rs commented Jul 29, 2025

Fixes #13443 by wrapping the version property in _dists.py with a try/except block and handling the fallback in _envs.py.
I saw @pfmoore’s suggestion that raising an error might be the better approach, but after observing how pip handles invalid name metadata entries, I chose not to modify the rest of _envs.py that handles error reporting to keep things simple for now. I'm more than happy to revisit that part if needed. Feedback is very welcome!

@sepehr-rs sepehr-rs force-pushed the fix-version-metadata-error branch from af66b65 to 13f70d7 Compare July 29, 2025 07:25
@sepehr-rs
Copy link
Contributor Author

sepehr-rs commented Jul 29, 2025

I fixed the pre-commit errors, but I think the pre-commit test didn't run correctly cause it's still failing on a line that I changed.
I would really appreciate any help on how to fix the other failing tests :).

@notatallshaw
Copy link
Member

That is odd, I'm not sure why pre-commit is doing that.

@notatallshaw
Copy link
Member

Though the failing tests do need to be fixed, it is important that users can continue to uninstall packages even if the version is invalid.

@sepehr-rs
Copy link
Contributor Author

sepehr-rs commented Aug 1, 2025

Though the failing tests do need to be fixed, it is important that users can continue to uninstall packages even if the version is invalid.

I followed the same approach used for the Name metadata value, so I'm unsure how to proceed from here.
Also, how would you recommend I proceed with the failing tests? Should I update the tests themselves, or is there an issue with my implementation?

@pfmoore
Copy link
Member

pfmoore commented Aug 1, 2025

@notatallshaw is that a behaviour change, though? The original issue #13443 suggests that there is already an error (just an unhandled exception, not a “proper” error). If we’ve turned working code (uninstalling a project with an invalid version) into a failure, that suggests the fix has modified something it shouldn’t.

I'm not against allowing the uninstall of packages with an invalid version, but the “blast radius” of this PR seems to have become larger than the original issue…

@sepehr-rs
Copy link
Contributor Author

@notatallshaw is that a behaviour change, though? The original issue #13443 suggests that there is already an error (just an unhandled exception, not a “proper” error). If we’ve turned working code (uninstalling a project with an invalid version) into a failure, that suggests the fix has modified something it shouldn’t.

I'm not against allowing the uninstall of packages with an invalid version, but the “blast radius” of this PR seems to have become larger than the original issue…

Thank you for your comment, @pfmoore, I appreciate the feedback. From what I can tell, this approach seems consistent with how the Name metadata value is handled, so I'm still trying to understand why it might not be appropriate here. I hope I’m not overlooking something obvious — I'm still learning the nuances of contributing to FOSS and genuinely eager to improve.

Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a justification for the changes in _envs.py.

@notatallshaw
Copy link
Member

@notatallshaw is that a behaviour change, though?

The PR is already updated at this point (thanks @sepehr-rs ), but going back to this, yes, users have been able to fix their issue by uninstalling the package using pip, we've had several GitHub issues raised where this has been the confirmed solution.

@pfmoore
Copy link
Member

pfmoore commented Aug 1, 2025

Thanks, @notatallshaw - once I'd realised that the problem was the extra, unnecessary, check, it became obvious that was what was impacting the uninstall command.

This now looks OK to me. I won't merge yet, as I think @ichard26 wants to keep main frozen for a while in case we need a bugfix release for 25.3.

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

Successfully merging this pull request may close these issues.

Unhandled exception if version is not determinable
3 participants