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

Added a trick to reduce the snap size #9

Open
wants to merge 8 commits into
base: stable
Choose a base branch
from

Conversation

sergio-costas
Copy link
Contributor

This trick reduces the size from 110 to 87 MBytes.

Pull Request Template

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please check only the options that are relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

Test Configuration:

  • OS (please include version):
  • Any other relevant environment information:

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have checked my code and corrected any misspellings

This trick reduces the size from 110 to 87 MBytes.
Copy link
Contributor

@seb128 seb128 left a comment

Choose a reason for hiding this comment

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

Thanks, could you explain the difference between the strategy used before and after your change?

We were already iterating over the core/theme/gnome snaps and removing anything provided by those so what's the difference?

Also the schemas handling change, is that part of the cleanup or is that an unrelated fix that should be split in its own commit perhaps?

@sergio-costas
Copy link
Contributor Author

sergio-costas commented Jun 15, 2023

@seb128 Previously, it only removed libraries (.so), while now it removes any file that is duplicated, including schemas, for example. This, of course, requires to add both the current schemas folder (the one in epiphany) and the original (the one in gnome-42-2204).

This can be done reliably only if we install the stage packages separately, instead of installing all of them at the same time that we compile epiphany, because in the later case (as it was being done before) we couldn't differentiate between a repeated file installed by a .deb package (for example, due to dependencies) and a file with the same name but created by compiling the sources.

So the idea now is to build first epiphany, and install all the files in stage, as usual, and only then, in a separate part, download and install all the stage-packages: they will be downloaded first into $CRAFT_PART_INSTALL, so we can delete any file that is already in $CRAFT_STAGE(because it was already created when compiling the epiphany source code) or in any of the base or extension snaps, without risking to delete a file from epiphany that has the same name than one in the base or extension snaps.

@sergio-costas
Copy link
Contributor Author

sergio-costas commented Jun 15, 2023

@seb128 Sorry, that list of changes was between the previous patch and this one; the full difference between stable and this PR is this:

changes.txt

@sergio-costas
Copy link
Contributor Author

@seb128 Oh, and also, the python script is much faster than the bash script, which allows to scan all the files, and not only the libraries, in a reasonable time.

@sergio-costas
Copy link
Contributor Author

In other words: before, in the original snapcraft.yaml, we mix in CRAFT_PART_INSTALL the files from the .deb packages and the files generated by compiling epiphany, so there is no way of knowing which files are really from the .deb packages and, thus, can be safely removed if they are already in core22 or gnome-42-2204, and which ones have been created by epiphany and must not be removed. Separating the .deb packages and the source code compilation in two different parts allows to know which ones can be safely removed if they are already in the core or extension snaps.

@seb128 seb128 requested a review from kenvandine June 15, 2023 13:16
@seb128
Copy link
Contributor

seb128 commented Jun 15, 2023

Tagging @kenvandine for input there. We have such optimization routines in most of our desktop snaps and it isn't great to have to redo the same things, that is going a step further. I don't think we want to start copying the script around, we should have one place for it where it can be shared (gnome sdk? snapcraft? ...?)

@sergio-costas
Copy link
Contributor Author

Fully agree.

Since the stage packages are installed at $CRAFT_PART_INSTALL
before the BUILD stage begins, we can run the clean script in
that point to remove all the duplicated files while keeping
the stage packages in their corresponding part.

This means that we have to run the cleaning script in each
part, but since we check the files in the INSTALL folder
(instead of checking all the files in the snaps and remove
whichever ones match), it is still much faster than the
previous method.
@sergio-costas
Copy link
Contributor Author

Ok, here is my proposal for sharing the scripts. They are stored in a common repository (where other useful scripts, like another one that ensures that all the python scripts have the correct shebang at the beginning), and just by copy/paste a part, they are available.

@hellsworth
Copy link

hellsworth commented Jun 20, 2023

I think this could be useful for large projects that have a lot of stage packages, however it's a hard sell to tell folks to now add this part to their snapcraft.yaml files.

The install file being run runs 2 scripts:

  1. set_python_runtime - makes sure all python script shebang lines are #!/usr/bin/env python3. But what about a project being snapped that isn't a python project or the dev doesn't want to run this?
  2. remove_common - "Removes files that are already in base snaps or have been generated in a previous part. Useful to remove files added by stage-packages due to dependencies, but that aren't required because they are already available in core22, gnome-42-2204 or gtk-common-themes, or have already been built by a previous part." (src) This is useful for sure, but it is not intuitive to add $CRAFT_PROJECT_DIR/snaptools/remove_common.py core22 gtk-common-themes gnome-42-2204 to each part wishing to clean up its dependencies.

@sergio-costas
Copy link
Contributor Author

sergio-costas commented Jun 21, 2023

@hellsworth The install script doesn't run the available tools; it just copies them into the project's folder, but doesn't run any of them. The idea is to make them available during the build, just in a single step. Also, the idea is to ensure that they are kept up-to-date in any build. The idea is to make that repository the place where to put all these kind of tools. Other interesting tools to-be-done could be ".desktop fixers", for example, to automagically put a list of .desktop files in the right place and modify them to point to the right place, instead of having to use a "sed magic spell".

About the second problem... well, the idea is to replace this snippet of code:

build-snaps: [core22, gtk-common-themes, gnome-42-2204]
override-prime: |
  for snap in "core22" "gtk-common-themes" "gnome-42-2204"; do
    cd "/snap/$snap/current" && find . -type f,l -name *.so.* -exec rm -f "$CRAFT_PRIME/{}" \;
  done

which, I think, is even less intuitive... (and much, much slower, BTW).

@sergio-costas
Copy link
Contributor Author

sergio-costas commented Jun 21, 2023

Also, my code is more secure than the original removal system, because the old code is run after all the files created and compiled in the snap have been laid off at PRIME, which means that if the snap builds a new minor version of a library already available in the system (either core22 or gnome-42-2204), it will be removed. That's why it can't be used in some snaps.

Instead, my script removes duplicated files before building anything, so it is completely safe.

Also, remember that it doesn't have to be run on all the parts; only on those that have "stage-packages".

Finally, that tool is really for "pro" snappers. It can be suggested after a snap has been created, as an enhancement. It isn't something mandatory.

snapcraft.yaml Outdated Show resolved Hide resolved
@sergio-costas
Copy link
Contributor Author

Ok, I prepared a draft document with the idea for the build tools: https://docs.google.com/document/d/10xKYNdOtYrKGWKhVpDYoTwvDG-f8LDQJZdXHvNcXJ_0/edit?usp=sharing

@hellsworth
Copy link

sorry tbc, I approved the changes but I want us to have more discussion before merging. So I approve the changes but not ready to merge yet :)

It is still incomplete, since sound doesn't work.
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.

3 participants