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

Allow compressing lima-guestagent with gzip #2438

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

afbjorklund
Copy link
Contributor

Remove all compression configuration, and make it boolean.

         compressed        uncompressed  ratio uncompressed_name
           10829118            39059456  72.3% _output/share/lima/lima-guestagent.Linux-aarch64
           11190957            38797312  71.2% _output/share/lima/lima-guestagent.Linux-armv7l
           11431176            39387136  71.0% _output/share/lima/lima-guestagent.Linux-riscv64
           12076900            40189952  70.0% _output/share/lima/lima-guestagent.Linux-x86_64
           45528151           157433856  71.1% (totals)

Could become the default config (y), after some evaluation.

Replaces #2427
Closes #2426

@AkihiroSuda AkihiroSuda added this to the v0.23.0 (tentative) milestone Jun 26, 2024
AkihiroSuda
AkihiroSuda previously approved these changes Jun 26, 2024
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda requested a review from a team June 26, 2024 02:05
jandubois
jandubois previously approved these changes Jun 30, 2024
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

I feel like usrlocalsharelima.Open does not belong in that package because it has nothing to do with that location, but I don't have any other suggestion. Maybe fileutils?

Anyways, I'm fine with merging as-is.

@afbjorklund
Copy link
Contributor Author

afbjorklund commented Jun 30, 2024

I can move the code back to pkg/cidata again, now that it is a lot smaller.

I wanted the compression to be abstract/local, but it leaked out anyway...

@afbjorklund afbjorklund dismissed stale reviews from jandubois and AkihiroSuda via 1a7c043 June 30, 2024 15:41
@afbjorklund afbjorklund force-pushed the guest-gzip branch 2 times, most recently from 1a7c043 to fa19eff Compare June 30, 2024 15:52
@afbjorklund
Copy link
Contributor Author

afbjorklund commented Jun 30, 2024

Maybe fileutils?

Something like gzopen would have been nice to have.

gzopen can be used to read a file which is not in gzip format; in this case gzread will directly read from the file without decompression. When reading, this will be detected automatically by looking for the magic two-byte gzip header.

But I can't find the format detection (file magic) either?

i.e. it is also missing from the compress/gzip module

jandubois
jandubois previously approved these changes Jun 30, 2024
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

The logic seems correct, but I would prefer a simplified expression that keeps the error handling close to the location the error occurred.

pkg/cidata/cidata.go Outdated Show resolved Hide resolved
Signed-off-by: Anders F Björklund <[email protected]>
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@jandubois jandubois merged commit caced74 into lima-vm:master Jul 1, 2024
27 checks passed
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.

Compress the lima-guestagent in the installation directory
3 participants