-
Notifications
You must be signed in to change notification settings - Fork 40
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
Upgrade zip to 2.1.3 #6964
Upgrade zip to 2.1.3 #6964
Conversation
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.
Tentative approval; I think we have sufficient test coverage to ensure that a given version of Omicron can read the TUF repos generated by the same version, but this is a large-enough jump of a complicated-enough file format that I want to test a couple of cases that I don't think are in CI:
- Can Omicron R11 read a TUF repo generated with zip v2.2.0? (the actually important one to test)
- Can zip v2.2.0 read the Omicron R11 release TUF repo? (not as important but could be indicative of a bug of some kind)
If neither of us have a chance to test this in the immediate term, we could merge and see if an upgrade works on dogfood, which would test the first case but that might be more of a risk appetite than we want right now? (Particularly if the second case is a problem and we have to revert.)
Was glancing at the repo and spotted zip-rs/zip2#231, a performance regression in v2.1.4+ on very large archives. edit: Apparently also affects v2.1.3 per mozilla/sccache#2227. |
What do you think about upgrading to v2.1.2 until zip-rs/zip2#247 is merged? |
I'll heavily lean on the mozilla/supply-chain review from 0.6.4 to 2.1.3 and say it's probably fine. However I'm not seeing anything in the changelog that would indicate 2.1.3 is implicated by a performance change, and the bisected commit found in 231 is part of 2.1.4. Based on some of the issues I've been reading it seems like the general performance of the crate has degraded since 0.6.6 an acceptable amount, it's just the 2.1.4 regression I'm worried about since it directly impacts anywhere we upload a TUF repo. |
That is, either 2.1.2 or 2.1.3 is fine by me. |
Sweet, good find on the audit. I'm using 2.1.2 instead of 2.1.3 because mozilla/sccache#2227 indicated that 2.1.3 might also include a performance regression? Either way, 2.1.2 seems low-risk, and we should be able to easily roll forward once the fixes are in. |
I think the sccache issue is indicating a performance regression in 0.6.6->2.1.3 overall, and not necessarily implicating the 2.1.2->2.1.3 changes. |
Ack, I'll use |
There are some features in zip that I'd like to use in #6782, but which are only supported in newer versions.
This PR upgrades zip across the repo to
2.1.3
as a standalone change