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

snap: use the -no-fragments mksquashfs option #4396

Merged
merged 2 commits into from Dec 20, 2017

Conversation

tyhicks
Copy link
Contributor

@tyhicks tyhicks commented Dec 13, 2017

Use of the Squashfs fragments feature results in unpredictable snap
composition due to how fragments are gathered and compressed using a
separate thread in mksquashfs. Disable fragments entirely in snap pack
and tests that call mksquashfs directly so that the review tools can
perform validation of snaps and to be consistent with what snapcraft is
doing.

Fixes: https://launchpad.net/bugs/1576763
Signed-off-by: Tyler Hicks tyhicks@canonical.com

This PR corresponds to the following:

canonical/snapcraft#1805
https://forum.snapcraft.io/t/proposal-to-disable-squashfs-fragments-in-snaps/3103

Use of the Squashfs fragments feature results in unpredictable snap
composition due to how fragments are gathered and compressed using a
separate thread in mksquashfs. Disable fragments entirely in `snap pack`
and tests that call mksquashfs directly so that the review tools can
perform validation of snaps and to be consistent with what snapcraft is
doing.

Fixes: https://launchpad.net/bugs/1576763
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
@codecov-io
Copy link

codecov-io commented Dec 13, 2017

Codecov Report

Merging #4396 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4396      +/-   ##
==========================================
+ Coverage   78.03%   78.04%   +<.01%     
==========================================
  Files         449      449              
  Lines       30906    30907       +1     
==========================================
+ Hits        24118    24121       +3     
+ Misses       4775     4774       -1     
+ Partials     2013     2012       -1
Impacted Files Coverage Δ
snap/squashfs/squashfs.go 65% <100%> (+0.59%) ⬆️
overlord/hookstate/hookmgr.go 72.83% <0%> (+1.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9a0807...554bed1. Read the comment docs.

@bboozzoo
Copy link
Collaborator

I've restarted Travis job since it hit a timeout.

@chipaca
Copy link
Contributor

chipaca commented Dec 13, 2017

Gosh. We shouldn't have all those mksquashfs lying around now that we have snap pack. You mind if I piggyback on this PR to move those over?

Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! :)

else
mksquashfs "$dir" "$snap" -comp gzip -Xcompression-level 1
mksquashfs "$dir" "$snap" -comp gzip -Xcompression-level 1 -no-fragments

Choose a reason for hiding this comment

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

Not blocking on this, but -no-fragments is only interesting for store uploads, and anything using -comp gzip is not going to be uploaded to the store anyway (since the resquash tests would fail).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I paused before adding it to that mksquashfs command precisely because of what you mentioned. Even though it isn't going to be uploaded to the store, I think it is still work testing with similar arguments to what all other snaps will be generated with.

Choose a reason for hiding this comment

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

@chipaca should comment on this part, but iirc gzip is being used to help address travis timeouts because it is faster. Is -no-fragments slower than without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, in my testing, the -no-fragments option actually speeds up mksquashfs ever so slightly.

@jdstrand
Copy link

I've restarted travis since it timed out.

@tyhicks
Copy link
Contributor Author

tyhicks commented Dec 13, 2017

@chipaca Yes, please feel free to push a commit to use snap pack everywhere.

@tyhicks
Copy link
Contributor Author

tyhicks commented Dec 13, 2017

The test failures are unrelated to the changes in this PR. Three test runs have failed due to Travis timeouts and another due to HTTP requests to the store timing out.

@tyhicks
Copy link
Contributor Author

tyhicks commented Dec 18, 2017

Is there anything preventing this change from being merged? The corresponding change to snapcraft has been merged (canonical/snapcraft@9a131cb) so I'd hate for this PR to linger for too long since we'd like to change the review-tools to require -no-fragments soon.

@chipaca
Copy link
Contributor

chipaca commented Dec 18, 2017

@tyhicks just that testing infra is a bit broken, as soon as it's passing things this'll get merged

@mvo5 mvo5 merged commit e1eddad into snapcore:master Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants