-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add CRC mode to make CRC checks opt-in by default #12706
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
Conversation
|
LGTM. |
|
We should probably make this a global setting so it shows up in settings, etc. But I know that's more work, can be a TODO. |
| Enforce, | ||
| /// Warn on CRC mismatch, but continue. | ||
| Lax, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why not "Error" and "Warn" here which feel more canonical to me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(which would also be "Ignore" instead of "None", I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we'd then want a different name? UV_CRC_CHECK = error | warn | ignore? I feel less strongly about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to do the change once I'm back to personal laptop
|
I'm gonna put up a PR for an alternative approach. |
|
I'm going to close this in favour of #12722, but if that fix proves insufficient this is a more robust hammer we should revisit. |
|
Thanks for hoping on this anyway Sam. |
Summary
This is a POC quick fix as proposed in #12618 (comment)
This adds a
UV_CRC_MODEenv var that allowsenforceorlaxto be set or default tononewhich does nothing.Avoided reworking out the interfaces as that led to a larger change across uv-extract and uv-metadata, but longer term that's probably a better solution.
Looking at the dependency graph, uv-distribution-filename was sort-off the best place for CRC Mode, but I'd be happy to move it.Test Plan
Updated existing crc test.