Skip to content

Conversation

@jaimergp
Copy link
Contributor

@jjerphan jjerphan added the release::bug_fixes For PRs fixing bugs label Mar 19, 2025
@Hind-M
Copy link
Member

Hind-M commented Mar 20, 2025

Thanks!

Maybe also add some tests in test_package_info.cpp:

  • Always check pkg.md5 and pkg.sha256 values
  • Add a section for a hash.size=64 (sha256 case)

Should we support sha256: in urls in this PR as well? (it can be done later otherwise since it's not mandatory).

@jaimergp
Copy link
Contributor Author

Should we support sha256: in urls in this PR as well? (it can be done later otherwise since it's not mandatory).

I'll try my best 😬

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution, @jaimergp.

This LGTM modulo one discussion.

Copy link
Member

Choose a reason for hiding this comment

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

I think we might want to do something in else cases.

How about raising a warning when hashes are malformed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that sounds sensible. I didn't add them to respect the existing logic (silent ignore). I'm not sure how to add warnings in this codebase, so feel free to commit to the branch directly :)

Copy link
Member

Choose a reason for hiding this comment

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

You are right, and thinking again I am not sure we want to add warnings to spam users if this does not cause fatal behavior and if they cannot do anything about it.

@jjerphan jjerphan merged commit b3a48ce into mamba-org:main Mar 21, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release::bug_fixes For PRs fixing bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants