Skip to content

ci: add cache for ClamAV and use dedicated script for package installation#2752

Closed
georglauterbach wants to merge 8 commits intomasterfrom
ci/build
Closed

ci: add cache for ClamAV and use dedicated script for package installation#2752
georglauterbach wants to merge 8 commits intomasterfrom
ci/build

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

Description

This PR does two things:

  1. Add build caching for ClamAV's bytecode
  2. Use a separate script for installing packages

The separate script provides a better overview of the installation process. While I was at it, I removed non-essential packages from the packages list. Therefore, this PR is a breaking change.

Moreover, users that want to build DMS themselves are now required to use a recent version of Docker because we need BuildKit (one needs Docker version >= 20.10.14).

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the style guidelines of this project
  • 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 (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

This commit introduces a new stage (`clamav-stage`) and the `--link`
flag for teh `COPY` directive. We now require `DOCKER_BUILDKIT` to be
set to `1`. Moreover, the `Dockerfile` syntax is explictly set in the
`Dockerfile` (required for `--link`).

A bit of refactoring was done in the Dockerfile to keep everything
consistent.
This commit does more refactoring. The build arguments names' were
changed too. I did this because I was not really sure what `_REF` was
for.
This commit prepares functionality for the next commit. It refactors the
script a bit too. The style of function definitions was changed, because
I think many people do not like the style I introduced a (long) while
back, so I want to make an effort here.
This commit outsources package installation into a dedicated script. I
think the `Dockerfile` was cluttered by this huge `RUN` statement, a
dedicated script is more fitting in my eyes.

I made sure to properly separate all different steps when it comes to
the installation process. Moreover, you will notices that

1. many packages were removed - these packages are almost all packages
   related to compressing / decompressing / reading archives, etc. which
   the core image of DMS does not rely on
2. the `rspamd` package was added (there is no configuration done at
   this point in time however - a separate PR will follow)

Therefore, THIS COMMIT IS A BREAKING CHANGE. Users that rely on the
various packages for reading certain archives will now need to install
these packages themselves.
@georglauterbach georglauterbach added area/ci priority/medium area/scripts kind/improvement Improve an existing feature, configuration file or the documentation pr/waiting for other PR to get merged This PR is waiting for another / other PR(s) to get merged stale-bot/ignore Indicates that this issue / PR shall not be closed by our stale-checking CI labels Aug 29, 2022
@georglauterbach georglauterbach added this to the v12.0.0 milestone Aug 29, 2022
@georglauterbach georglauterbach self-assigned this Aug 29, 2022
@georglauterbach
Copy link
Copy Markdown
Member Author

I noticed that this PR supersedes #2747. #2747 may be closed.

Accidently removed too many packages.. I hope this is the only package
that needs to be re-added..
The environemnt variables `LOG_LEVEL` and `DEBIAN_FRONTEND` are now
properly located in the `Dockerfile`. Previously, they were not
respected, because they were only build arguments.
Comment thread Dockerfile
Comment thread Dockerfile
Comment thread Dockerfile
Comment thread Dockerfile
Comment thread Makefile
Comment thread target/scripts/build/packages.sh
Comment thread target/scripts/build/packages.sh
Comment thread target/scripts/helpers/log.sh
Comment thread target/scripts/helpers/log.sh
Comment thread target/scripts/helpers/log.sh
Comment thread .github/workflows/default_on_push.yml
@polarathene

This comment was marked as outdated.

@casperklein
Copy link
Copy Markdown
Member

casperklein commented Aug 30, 2022

It was said somewhere else already and I second this again: To make reviews easier, keep them as small as possible and avoid changes not directly needed for the goal a PR achieves (such as beautifying things or introducing other little additions like rspamd).
Like the unix philosophy: do one thing well 👍

Edit: Ok, you already addressed this here.

@georglauterbach
Copy link
Copy Markdown
Member Author

I see, my PR became a bit bit and not UNIX-oid.. I will provide smaller ones that incorporate the review feedback - thank's @polarathene :)

georglauterbach added a commit that referenced this pull request Aug 30, 2022
@georglauterbach georglauterbach removed priority/medium pr/waiting for other PR to get merged This PR is waiting for another / other PR(s) to get merged labels Aug 30, 2022
@georglauterbach georglauterbach removed the stale-bot/ignore Indicates that this issue / PR shall not be closed by our stale-checking CI label Aug 30, 2022
@georglauterbach georglauterbach deleted the ci/build branch August 31, 2022 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci area/scripts kind/improvement Improve an existing feature, configuration file or the documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants