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

riotdocker-base: Split out build logic #225

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maribu
Copy link
Member

@maribu maribu commented Feb 17, 2023

This splits out all the build logic into the bash script build.sh. This has two advantages:

  • Only a single layer is added for this Dockerfile - This reduces overhead, especially with the VFS storage driver - Still takes full advantage of de-duplication of the layers concept: No image is based on intermediate steps anyway
  • Improves maintainability - Strict split of meta data (--> Dockerfile) and build commands (--> build.sh) - No need for long cmd_a && cmd_b && cmd_c && cmd_d stuff anymore

NORMAL="\e[0m"

step() {
export COUNTER_SUBSTEP=0
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think export is needed.

Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

I'm a bit conflicted about duplicating Docker functionality, but understand that there are two reasons to go this path:

  • It reduces the number of layers in the image.

    There is the "squash" image option, which would have the same effect, but is documented to be "experimental".

    We currently go out of our way to make the many images not much worse than a single one by cleaning up right after each step (if we did apt-get clean in a separate run command, the temporary files would linger). Embracing --squash would be an alternative here that'd solve the readability issues (the && chain would be replaced by individual RUN commands).

    Is there a concrete case where --squash would not suffice, or is it really "that experimental"?

  • This makes the setup easy to port over to non-Docker situations, but rummaging around in /root is probably unacceptable in all but container images. Is there a concrete scenario in which this would be used?

(I trust you have good answers to both parts; I'm generally OK with going this path, but I'd like to have these deliberations documented before hitting the button).

apt-get -y --no-install-recommends install git

substep "Installing Python"
apt-get -y --no-install-recommends install \
Copy link
Member

Choose a reason for hiding this comment

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

Doing multiple apt-get install in separate steps incurs some run-time cost from hooks being run after each apt-get.

But fine with me -- it's little time spent in CI, and much better readability gained.

@maribu
Copy link
Member Author

maribu commented Feb 20, 2023

  • This makes the setup easy to port over to non-Docker situations, but rummaging around in /root is probably unacceptable in all but container images. Is there a concrete scenario in which this would be used?

The COPY statement would create a new layer, so the files have to be bind-mounted. Mounting them into /tmp or /var/tmp results in rm -rf /tmp/* /var/tmp* done in the clean-up step to fail; and I don't like to pimp up that clean-up step to skip over a manually curated list of exceptions.

bind-mounting them instead into e.g. /buildtmp/ results in an empty /buildtmp/ folder being left over; which may not be a big issue, but leaking implementation details is also not nice. The home folder of the root users can be assumed to be there already and not being created just so that bind-mounting files into it works.

There is the "squash" image option, which would have the same effect, but is documented to be "experimental".

This doesn't work with the CI we use (at least yet). But I need the layers to be reduced to a sane number now.

Honestly, I still fail to understand why there is not dedicated command to generate a layer at sensible points in time, but rather force users to do RUN foo && bar && baz && ... to work around a fundamental design flaw in docker. This feels so insanely stupid...

@chrysn
Copy link
Member

chrysn commented Feb 20, 2023

I was mentioning /root not as a concrete objectionable path, but the whole topic of "doing a lot of things that exceed what a clean installer does". As said: It's fine, it just means that this will only ever be run in some kind of virtualized or throwaway systems. There could be a line somewhere that all files in the local directory are expected to be copied (or otherwise provided) into /root, but that's a minor note.

Do you have a pointer to the setups you'll use build.sh in once available?

Is the general roadmap behind this PR that once riotdocker-base is done, other images would follow?

One more minor suggestion (sic): If the step/substep functions detect that their terminal is GitHub, they could emit ::group:: in front of their output to make it collapsible. May need a ::endgroup:: and thus corresponding end_step / end_substep commands, though.

@maribu
Copy link
Member Author

maribu commented Feb 20, 2023

Do you have a pointer to the setups you'll use build.sh in once available?

My roadmap would be to first convert all docker containers to only add one layer on top of what they are based on. Afterwards, the toolchain installation stuff could be added to RIOT-OS/RIOT, so that Ubuntu users could just run that to install the toolchains. Once that is in place, the build.sh could just shallow-clone RIOT-OS/RIOT and invoke that script.

So basically, build.sh would never be used elsewhere. But the parts generally useful would eventually be split out of it.

}

endstep() {
if [ -n "${GITHUB_RUN_ID}" ]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

This apparently doesn't work. It actually is reassuring that environment variables do not leak into the container; but now I have no idea how to emit the ::group:: ... ::endgroup:: only when run in Github.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, if only they did the regular thing, advertised that they are their own TERM, or just packed these in vendor specific OSC codes.

Might be that the Docker environment would need to pass on the variable – but unless you particularly feel like it, let's not spend more time on this.

@chrysn
Copy link
Member

chrysn commented Feb 20, 2023

Ah, so while this particular build would not be run (neither setup_git nor /data/riotbuild makes sense outside), later parts would be shaped to be suitable for running on Ubuntu.

I'd still recommend against doing that unchecked (maybe inside a checkinstall), but that will be a matter of preferences then.

All fine with me, please squash / finalize. (Not pressing ACK here yet because unlike in RIOT there are no checks against accidental merges in the presence of fixups, AFAICT).

@maribu maribu force-pushed the riotdocker-base/reduce-layers branch from 51867f4 to 706e704 Compare February 22, 2023 07:39
This splits out all the build logic into the bash script build.sh. This
has two advantages:

- Only a single layer is added for this Dockerfile
    - This reduces overhead, especially with the VFS storage driver
    - Still takes full advantage of de-duplication of the layers
      concept: No image is based on intermediate steps anyway
- Improves maintainability
    - Strict split of meta data (--> Dockerfile) and build commands
      (--> build.sh)
    - No need for long `cmd_a && cmd_b && cmd_c && cmd_d` stuff anymore
@maribu maribu force-pushed the riotdocker-base/reduce-layers branch from 706e704 to ce7dd92 Compare February 22, 2023 07:42
@maribu
Copy link
Member Author

maribu commented Feb 22, 2023

The output of https://github.com/RIOT-OS/riotdocker/actions/runs/4240464444/jobs/7369508764 shows that ::group:: ... ::endgroup:: doesn't work here. I think that groups cannot be nested and that the build step is already a ::group::.

Since this apparently doesn't work, I just reverted to using human readable step indication.

@maribu
Copy link
Member Author

maribu commented Feb 22, 2023

IMO this is now ready

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