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

Updating package zipping #4212

Merged
merged 15 commits into from
Feb 10, 2025
Merged

Updating package zipping #4212

merged 15 commits into from
Feb 10, 2025

Conversation

brettsam
Copy link
Member

@brettsam brettsam commented Dec 13, 2024

The DotNetZip nuget package is deprecated and we need to remove support for it. On top of that, we've had a gozip utility that we build and maintain and that ships alongside Core Tools for the express reason of zipping packages that are mountable in Linux using fuse-zip, which is what is used in production.

The goal here is to move the default zipping to use a new in-proc zip utility rather than maintaining the gozip build and deployment long term to make things simpler.

The reason for gozip in the first place was that Linux executable file permissions were not being correctly applied when the zip file was created in Windows when using System.IO.Compression.ZipArchive. This is because of some uniqueness in the way gozip creates these zips and the way fuse-zip mounts them:

However, rather than pull in yet another external dependency, I decided to use ZipArchive, but modify the resulting stream when running on Windows to flip each file to be marked as CreatedByUnix.

I've also added a full test that, when running on CI, takes a zip created on Windows and mounts it on Linux using fuse-zip to ensure that it is runnable.

As an escape-hatch, I'm leaving gozip as part of the build and using the setting of FUNCTIONS_CORE_TOOLS_USE_GOZIP the local environment will change the zipping mechanism back. I also added telemetry so we can see usage. Once we're confident, we can remove this entirely in a future release.

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes do not need to be backported to a previous version
  • I have added all required tests (Unit tests, E2E tests)

liliankasem
liliankasem previously approved these changes Jan 29, 2025
Copy link
Member

@liliankasem liliankasem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I like that you included a feature flag as a fail safe via the environment variable + included telemetry! Woild like to see more tests for macos as well

aishwaryabh
aishwaryabh previously approved these changes Jan 29, 2025
Copy link
Contributor

@aishwaryabh aishwaryabh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left some nit comments :)

surgupta-msft
surgupta-msft previously approved these changes Jan 30, 2025
@aishwaryabh aishwaryabh mentioned this pull request Feb 3, 2025
5 tasks
Copy link
Member

@fabiocav fabiocav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but left some small comments.

@fabiocav
Copy link
Member

fabiocav commented Feb 6, 2025

Would also be good to add a note about this change to the release notes.

@brettsam brettsam merged commit 769acab into main Feb 10, 2025
11 checks passed
@brettsam brettsam deleted the brettsam/zip branch February 10, 2025 21:11
brettsam added a commit that referenced this pull request Feb 11, 2025
@brettsam brettsam mentioned this pull request Feb 11, 2025
5 tasks
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.

5 participants