Conversation
iliana
left a comment
There was a problem hiding this comment.
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.3as a standalone change