Skip to content
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

Merged
merged 5 commits into from
Nov 1, 2024
Merged

Upgrade zip to 2.1.3 #6964

merged 5 commits into from
Nov 1, 2024

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Oct 30, 2024

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

@smklein smklein requested a review from iliana October 31, 2024 00:52
Copy link
Contributor

@iliana iliana left a 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:

  1. Can Omicron R11 read a TUF repo generated with zip v2.2.0? (the actually important one to test)
  2. 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.)

@iliana
Copy link
Contributor

iliana commented Oct 31, 2024

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.

@smklein
Copy link
Collaborator Author

smklein commented Oct 31, 2024

What do you think about upgrading to v2.1.2 until zip-rs/zip2#247 is merged?

@smklein smklein changed the title Upgrade zip to 2.2.0 Upgrade zip to 2.1.2 Oct 31, 2024
@iliana
Copy link
Contributor

iliana commented Oct 31, 2024

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.

@iliana
Copy link
Contributor

iliana commented Oct 31, 2024

That is, either 2.1.2 or 2.1.3 is fine by me.

@smklein
Copy link
Collaborator Author

smklein commented Oct 31, 2024

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.

@iliana
Copy link
Contributor

iliana commented Oct 31, 2024

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.

@smklein smklein changed the title Upgrade zip to 2.1.2 Upgrade zip to 2.1.3 Oct 31, 2024
@smklein
Copy link
Collaborator Author

smklein commented Oct 31, 2024

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 2.1.3 then

@smklein smklein merged commit f7aff49 into main Nov 1, 2024
18 checks passed
@smklein smklein deleted the upgrade-zip branch November 1, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants