Closed Bug 1621066 Opened 4 years ago Closed 4 years ago

compress flatpak tars with xz instead of gzip

Categories

(Release Engineering :: Release Automation: Other, enhancement)

Product:
Release Engineering
Component:
Release Automation: Other
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox75 fixed, firefox76 fixed)

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
firefox75 --- fixed
firefox76 --- fixed

People

(Reporter: mtabara, Assigned: MaxPham99, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(3 files)

Bug 1621066 - Compress flatpak tars with xz instead of gzip.
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review
Bug 1621066 - correcting path when generate_upstream_artifacts
47 bytes, text/x-phabricator-request
Details | Review
[pushflatpakscript] replace gzip with xz for compressing the tarball
63 bytes, text/x-github-pull-request
Details | Review

We currently package flatpaks as tar.gz format. While that's totally fine, we should use xz compression instead to comply with the standard we have in-tree.

This requires changes in:

  • run.me - when we create the archive
  • pushflatpakscript - when we unpack the archive

Happy to provide more details for those interested in grabbing this bug.

Hi there,
I would love to take on this bug !

Flags: needinfo?(mtabara)

I did some initial research on the rationale of the standard that was set. And it seems that LZMA has greater compression rate than gzip, but slower in compression and decompression time. But through my Firefox builds, it would make sense to try to minimize the size of the files since it does already take some time to compile the app.

I would love to hear your feedbacks on my intuition that makes this bug, a bug. As it provides me with some rationale on the expected results of this change.

Flags: needinfo?(mtabara)

(In reply to Max from comment #1)

Hi there,
I would love to take on this bug !

First of all, thank you for voluteering to work on this!

(In reply to Max from comment #2)

I did some initial research on the rationale of the standard that was set. And it seems that LZMA has greater compression rate than gzip, but slower in compression and decompression time. But through my Firefox builds, it would make sense to try to minimize the size of the files since it does already take some time to compile the app.

I would love to hear your feedbacks on my intuition that makes this bug, a bug. As it provides me with some rationale on the expected results of this change.

You're raising a very good point. Frankly, I don't know the rationale behind the standardization of this, but I can surely dig in to find more details. Tangently related, I remember seeing some benchmarks (unrelated to this bug) present in https://github.com/facebook/zstd#benchmarks regarding LZMA compressions.

The main idea that drives this change is that we want to use the xz compression to be consistent with the way we archive sourceballs in-tree (for example https://archive.mozilla.org/pub/firefox/candidates/75.0b2-candidates/build1/source/firefox-75.0b2.source.tar.xz)

What needs to change?

  1. in run.me script from here we use tar and gzip() to archive the repo folder. We should instead create an xz() counterpart.

  2. in pushflatpakscript we uncompress this tar to read its contents - a) we first check if it's valid archive type in here and then we extract all in here. Left to be seen whether we actually need to make any change there or if tarfile complies with xz compression function too. At the very least, it's worth updating the comment from here to mention that we're using the xz instead.

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #4)

The main idea that drives this change is that we want to use the xz compression to be consistent with the way we archive sourceballs in-tree (for example https://archive.mozilla.org/pub/firefox/candidates/75.0b2-candidates/build1/source/firefox-75.0b2.source.tar.xz)

What needs to change?

  1. in run.me script from here we use tar and gzip() to archive the repo folder. We should instead create an xz() counterpart.

  2. in pushflatpakscript we uncompress this tar to read its contents - a) we first check if it's valid archive type in here and then we extract all in here. Left to be seen whether we actually need to make any change there or if tarfile complies with xz compression function too. At the very least, it's worth updating the comment from here to mention that we're using the xz instead.

  1. Considering the tar library, it does support compression in xz format. Of which, I propose the following changes:
# line 20 should be changed to:
TARGET_TAR_XZ_FULL_PATH="$ARTIFACTS_DIR/target.flatpak.tar.xz"
# line 125 should be changed to:
tar cvfJ flatpak.tar.xz repo
# line 127 should be changed to: 
mv -- flatpak.tar.xz "$TARGET_TAR_XZ_FULL_PATH"
  1. is_tarfile I think is fine to keep since it doesn't seem to require a specifier in params. Similarly for extractall as well.
    For the comment, I also agree to update the comment.

I'll try to build it and test around to see if the compression is lossy enough. Will keep you updated.

All of this sounds great! Thank you!
(more comments inline)

(In reply to Max from comment #5)

  1. Considering the tar library, it does support compression in xz format. Of which, I propose the following changes:
# line 20 should be changed to:
TARGET_TAR_XZ_FULL_PATH="$ARTIFACTS_DIR/target.flatpak.tar.xz"
# line 125 should be changed to:
tar cvfJ flatpak.tar.xz repo
# line 127 should be changed to: 
mv -- flatpak.tar.xz "$TARGET_TAR_XZ_FULL_PATH"

Precisely!

  1. is_tarfile I think is fine to keep since it doesn't seem to require a specifier in params. Similarly for extractall as well.
    For the comment, I also agree to update the comment.

I'll try to build it and test around to see if the compression is lossy enough. Will keep you updated.

Sounds good! FTR, we use Python 3.8 in the pushflatpakscript.

Assignee: nobody → MaxPham99
Status: NEW → ASSIGNED

Is it fine that moz-phab created 2 different revisions ? I wasn't sure how to commit to the same one. It's been quite a learning curve to get use to Hg.

(In reply to Max from comment #9)

Is it fine that moz-phab created 2 different revisions ? I wasn't sure how to commit to the same one. It's been quite a learning curve to get use to Hg.

Firstly, I know your pain. I've been using it for years and still don't grasp it completely :)
Ideally we have one single patch, let me see if I can bake them together or chain them as connected revisions.

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #10)

(In reply to Max from comment #9)

Is it fine that moz-phab created 2 different revisions ? I wasn't sure how to commit to the same one. It's been quite a learning curve to get use to Hg.

Firstly, I know your pain. I've been using it for years and still don't grasp it completely :)
Ideally we have one single patch, let me see if I can bake them together or chain them as connected revisions.

Hm, wanted to land them at once, since I saw you already chained them together as parent-child. Hitting some Phabricator issues, will get back to you once #lando team replies to my question.

Thanks for your contribution again, once this lands and gets on beta, we should be good to go! I'll close the bug once we have a successful beta pushed, likely early next week.

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #11)

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #10)

(In reply to Max from comment #9)

Is it fine that moz-phab created 2 different revisions ? I wasn't sure how to commit to the same one. It's been quite a learning curve to get use to Hg.

Firstly, I know your pain. I've been using it for years and still don't grasp it completely :)
Ideally we have one single patch, let me see if I can bake them together or chain them as connected revisions.

Hm, wanted to land them at once, since I saw you already chained them together as parent-child. Hitting some Phabricator issues, will get back to you once #lando team replies to my question.

Ah no worries, just let me know if there's anything I can help with.

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #12)

Thanks for your contribution again, once this lands and gets on beta, we should be good to go! I'll close the bug once we have a successful beta pushed, likely early next week.

That's awesome. Can't remember the last time I got my code pushed to production that fast haha. Thanks for your articulate explanations and help !

Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/integration/autoland/rev/ef2412788c58
Compress flatpak tars with xz instead of gzip. r=mtabara

https://hg.mozilla.org/mozilla-central/rev/ef2412788c58

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox76: --- → fixed
Resolution: --- → FIXED

Comment on attachment 9133056 [details]
Bug 1621066 - Compress flatpak tars with xz instead of gzip.

Beta/Release Uplift Approval Request

  • User impact if declined: None for user, this concernes flatpaks internals. Switching to xz() compression for flatpaks internally in order to comply with the source archive format.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String changes made/needed:
Attachment #9133056 - Flags: approval-mozilla-beta?

[Tracking Requested - why for this release]:

status-firefox75: --- → affected
tracking-firefox75: --- → ?

Once this patch is being upifted to beta, we can also land the scriptworker side - https://github.com/mozilla-releng/scriptworker-scripts/pull/176

Comment on attachment 9133056 [details]
Bug 1621066 - Compress flatpak tars with xz instead of gzip.

flatpak tweak, approved for 75.0b5

Attachment #9133056 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Worked like a charm in 75.0b5: repackaged job generating the xz archive - https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/SKMCWGe5TceEvRI-OxRCHw/runs/0/artifacts/public/build/target.flatpak.tar.xz and the push job successfully unpacking the tarball, see logs here

Thank you again for your contribution! \o/

Status: RESOLVED → VERIFIED

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #22)

Worked like a charm in 75.0b5: repackaged job generating the xz archive - https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/SKMCWGe5TceEvRI-OxRCHw/runs/0/artifacts/public/build/target.flatpak.tar.xz and the push job successfully unpacking the tarball, see logs here

Thank you again for your contribution! \o/

Yay !!!! No problemo :D

Blocks: 1441922
Component: Release Automation: Flatpak → Release Automation: Other
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: