ConsolidatePackages: Use System.IO.Compression and slightly tweak extraction loop#1913
Open
ConsolidatePackages: Use System.IO.Compression and slightly tweak extraction loop#1913
Conversation
borland
commented
May 5, 2026
| { | ||
| foreach (var fileTransfer in platformFiles) | ||
| { | ||
| var sourceEntry = source.Entries.FirstOrDefault(e => e.Key is not null && e.Key.Equals(fileTransfer.Source)); |
Author
There was a problem hiding this comment.
The nested loop means that if there were thousands of PlatformFiles and thousands of Entries, we'd end up doing millions of string-equals checks which can take meaningful amounts of time.
The numbers are probably smaller than that, and it's probably not a big deal, but why not pick the low-hanging fruit while we're in here 🤷
Also: The new code iterates the entries once in a forward-only direction, which is much more friendly to things like disk caches. Iterating in platformfiles order could potentially have us jumping around randomly. In the modern days of SSD's, random jumping around isn't so bad, but sequential reads are still better than random ones
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
https://octopusdeploy.slack.com/archives/C03SG0LFJHX/p1777933912502139
Renovate pulled through an update to the SharpCompress library for Octopus Server. SharpCompress is at version
0.xand reserves the right to make breaking changes at any time, and they've moved a few things around and renamed some methods in the public API surface.In particular, the
ZipArchive.Openmethod is nowZipArchive.OpenArchive.When we update Server to the new SharpCompress,
Calamari.ConsolidatePackagesis also loaded into the Server process, and is still expecting the old method signature. The code throws a Method Not Found exception at runtime.Analysis
We could simply update the version of SharpCompress used by
Calamari.ConsolidatePackagesto align with Server, however our default preference is to use the builtin libraries. They are generally much more stable, better supported, and sometimes more performant than third-party libraries -- Unless there is some case-specific reason not to.Calamari.ConsolidatePackagesis just doing ordinary zip-reading things, it isn't complex and there's no niche requirements, we can easily drop SharpCompress here and switch toSystem.IO.Compression, avoiding any future breaking changes that SharpCompress may have.Results
SharpCompresspackage reference fromCalamari.ConsolidatePackagesThis change does not require a Server PR.
Reducing Risk
This is the sticky part. The changes are simple, and to the best of my understanding, should be safe, but I don't have any knowledge or ability to test them. Are Calamari's TeamCity tests sufficient for us to be able to merge here? Do we need to do any extra testing? I would appreciate some advice from the owning team please.