Re: [docs] [yocto-docs PATCH] Add scripts to build the docs in containers

["Antonin Godard" <antonin.godard@bootlin.com>]

Hi Quentin,

On Mon Nov 25, 2024 at 7:26 PM CET, Quentin Schulz wrote:
> Hi Antonin,
>
> On 11/21/24 2:22 PM, Antonin Godard via lists.yoctoproject.org wrote:
>> Add two scripts for building a container image and building the
>> documentation in that image: build-docs-container and
>> container-build-docs.
>> 
>> For now, the documentation/tools/dockerfiles directory contains two
>> Dockerfiles for building with Ubuntu 24.04 and Debian 12, but we could
>> extend this to the supported distros.
>> 
>> It should be possible to build the full documentation with two commands:
>> 
>>    ./documentation/tools/build-docs-container ubuntu-24-04
>>    ./documentation/tools/build-docs ubuntu-24-04
>> 
>> The first command builds the container image by pulling the dependencies
>> listed in poky.yaml.in. The second command uses this image to build the
>> docs.
>> 
>> CONTAINERCMD can be replaced by "podman" to build with podman.
>> 
>> Signed-off-by: Antonin Godard <antonin.godard@bootlin.com>
>> ---
>>   documentation/tools/build-docs                     | 48 ++++++++++++++++++++
>>   documentation/tools/build-docs-container           | 51 ++++++++++++++++++++++
>>   .../tools/dockerfiles/Dockerfile.debian-12         | 17 ++++++++
>>   .../tools/dockerfiles/Dockerfile.ubuntu-24-04      | 17 ++++++++
>>   4 files changed, 133 insertions(+)
>> 
>> diff --git a/documentation/tools/build-docs b/documentation/tools/build-docs
>> new file mode 100755
>> index 0000000000000000000000000000000000000000..2fe8aff3a9834e4f5015e04d64b7462190278004
>> --- /dev/null
>> +++ b/documentation/tools/build-docs
>> @@ -0,0 +1,48 @@
>> +#!/usr/bin/env bash
>> +#
>> +# Build the documentation in a container built by build-docs-container.
>> +#
>> +# Usage:
>> +#
>> +#   ./documentation/tools/build-docs <image> [<command>]
>> +#
>> +# Where <image> is one of the keys in
>> +# documentation/tools/build-docs-container's DEPS_KEYS_YQ.
>> +# And <command> an optional command to run, default being to run "make publish".
>> +#
>> +# E.g.:
>> +#
>> +#   ./documentation/tools/build-docs ubuntu-24-04
>> +
>> +SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &> /dev/null && pwd)
>> +CONTAINERCMD=${CONTAINERCMD:-docker}
>> +
>> +DOCS_DIR="$SCRIPT_DIR/../.."
>> +
>> +main ()
>> +{
>> +  local image="$1"
>> +  shift
>> +  local command="${*:-make -C documentation publish}"
>> +
>
> Should we only require "publish" to be passed instead of make -C 
> documentation publish? I believe this should be doable with ENTRYPOINT?

I agree. I don't see any other command we'd want to run instead of "make
<something>".

About the entrypoint, what would be the added value of having one?

>> +  if [ "$CONTAINERCMD" = "docker" ]; then
>> +    EXTRA_ARGS_RUN="--user $(id -u):$(id -g)"
>> +  elif [ "$CONTAINERCMD" = "podman" ]; then
>> +    # we need net access to fetch bitbake terms
>> +    EXTRA_ARGS_RUN="\
>> +      --cap-add=NET_RAW \
>> +      --userns=keep-id"
>
> please add
>
> --security-opt label=disable
>
> here as well, for SELinux based systems such as Fedora. My understanding 
> is that for anything that is to be used by either the host or the 
> container should disable labeling.

Thanks for the tip. Will add (assuming this is not an issue for other distros).

>> +  fi
>> +
>> +  $CONTAINERCMD run \
>> +    --rm --interactive --tty \
>> +    --volume "$DOCS_DIR:/docs:rw" \
>> +    --workdir "/docs" \
>> +    $EXTRA_ARGS_RUN \
>> +    "yocto-docs-$image" \
>
> please prepend the image name with localhost/ to be sure we don't end up 
> downloading some yocto-docs-ubuntu from docker.io for example :)

Nice tip, didn't know about that one, thanks :)

>> +    $command
>> +}
>> +
>> +set -eu -o pipefail
>> +
>> +main "$@"
>> diff --git a/documentation/tools/build-docs-container b/documentation/tools/build-docs-container
>> new file mode 100755
>> index 0000000000000000000000000000000000000000..8cfe0f7aa75bf5fcfbbc0e838d66e0ef10f20696
>> --- /dev/null
>> +++ b/documentation/tools/build-docs-container
>> @@ -0,0 +1,51 @@
>> +#!/usr/bin/env bash
>> +#
>> +# Build a container ready to build the documentation be reading the dependencies
>> +# listed in poky.yaml.in.
>> +#
>> +# Usage:
>> +#
>> +#   ./documentation/tools/build-docs-container <image>
>> +#
>> +# Where <image> is one of the keys in DEPS_KEYS_YQ. E.g.:
>> +#
>> +#   ./documentation/tools/build-docs-container ubuntu-24-04
>> +
>> +SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &> /dev/null && pwd)
>> +CONTAINERCMD=${CONTAINERCMD:-docker}
>> +
>> +# Keys used by yq to get the dependencies from poky.yaml.in
>> +declare -A DEPS_KEYS_YQ=(
>> +  [ubuntu-24-04]=".UBUNTU_DEBIAN_HOST_PACKAGES_DOC .UBUNTU_DEBIAN_HOST_PACKAGES_DOC_PDF"
>> +  [debian-12]=".UBUNTU_DEBIAN_HOST_PACKAGES_DOC .UBUNTU_DEBIAN_HOST_PACKAGES_DOC_PDF"
>> +)
>> +
>> +main ()
>> +{
>> +  local image="$1"
>> +
>> +  for cmd in $CONTAINERCMD yq; do
>> +    if ! which "$cmd" >/dev/null 2>&1; then
>> +      echo "The $cmd command was not found. Make sure you have $cmd installed."
>> +      exit 1
>> +    fi
>> +  done
>> +
>> +  local dockername="Dockerfile.$image"
>> +
>> +  # Put all the dependencies in the deps variable
>> +  for dep_key in ${DEPS_KEYS_YQ[$image]}; do
>> +    deps="$deps $(yq --raw-output "$dep_key" "$SCRIPT_DIR/../poky.yaml.in")"
>> +  done
>> +
>> +  ( cd "$SCRIPT_DIR/dockerfiles" \
>> +    && $CONTAINERCMD build \
>> +    --tag "yocto-docs-$image:latest" \
>> +    --file "$dockername" \
>> +    --build-arg DEPS="$deps" \
>
> I am not sure this is future-proof. We'd be limited to the max length of 
> parameters one can use in a terminal. What about creating a temporary 
> file and using that temporary file from within the Dockerfile via COPY. 
> Also, you really want COPY and not MOUNT because changes in files in 
> MOUNT won't invalidate the layer IIRC.

Yes, I agree this would be better, I'll work on adding that instead.

>> +    . )
>
> Can't we just run
>
> docker build [...] -f $dockername "$SCRIPT_DIR/dockerfiles"
>
> to avoid having to cd into it in a subshell?

I don't remember why but I had trouble with this. For some reason I had to be in
the same directory as the Dockerfile... I'll give it another shot.

>> +}
>> +
>> +set -e -o pipefail
>> +
>
> set -u
>
> while we are at it :)

Forgot this one, thanks!

>> +main "$@"
>
> Question: why do we split the image creation from the running?

Good question. Initially it made sense to me to split both to avoid running
"docker build" each time, but knowing it would get cached I hesitated. If
checking for the cache is a low-cost operation (seems to be on my computer),
I also vote for having a single script that runs docker build each time. That
way we don't forget to rebuild when adding things to poky.yaml.in.

>> diff --git a/documentation/tools/dockerfiles/Dockerfile.debian-12 b/documentation/tools/dockerfiles/Dockerfile.debian-12
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..18c9c45c3ccfc5d90a09057d874512c6b9826611
>> --- /dev/null
>> +++ b/documentation/tools/dockerfiles/Dockerfile.debian-12
>> @@ -0,0 +1,17 @@
>> +FROM debian:12
>
> Would be nice to pass the version number from the shell script as well I 
> guess?
>
> There's a trick for that one IIRC, I'd have to check how I've done that 
> and get back to you if you're interested.

Yes, that'd be an improvement.

>> +
>> +ENV DEBIAN_FRONTEND=noninteractive
>> +
>> +ARG DEPS
>> +ENV DEPS=${DEPS}
>> +
>> +RUN apt-get update \
>> + && apt-get --yes --fix-broken upgrade \
>
> That should not be necessary?

Probably not. I picked that up from an older Dockerfile, and I don't remember
the reason why I put that here. Will remove.

>> + && apt-get --yes --no-install-recommends install \
>> +    ${DEPS} \
>> + && apt-get --yes autoremove \
>> + && apt-get clean
>> +
>> +RUN LANG="en_US.UTF-8" locale-gen
>> +
>> +RUN git config --global --add safe.directory /docs
>> diff --git a/documentation/tools/dockerfiles/Dockerfile.ubuntu-24-04 b/documentation/tools/dockerfiles/Dockerfile.ubuntu-24-04
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..6ac384c4f242b508027a92d2acad490da41dc374
>> --- /dev/null
>> +++ b/documentation/tools/dockerfiles/Dockerfile.ubuntu-24-04
>> @@ -0,0 +1,17 @@
>> +FROM ubuntu:24.04
>> +
>> +ENV DEBIAN_FRONTEND=noninteractive
>> +
>> +ARG DEPS
>> +ENV DEPS=${DEPS}
>> +
>> +RUN apt-get update \
>> + && apt-get --yes --fix-broken upgrade \
>> + && apt-get --yes --no-install-recommends install \
>> +    ${DEPS} \
>> + && apt-get --yes autoremove \
>> + && apt-get clean
>> +
>> +RUN LANG="en_US.UTF-8" locale-gen
>> +
>
> I believe this is not working if I remember correctly the thread with 
> Guénaël?

You're right, actually this does nothing, and modifying /etc/locale.gen seems to
be the proper way (for debian/ubuntu compatibility).

> Otherwise, the Dockerfile are identical, so maybe parameterizing the 
> FROM line would be better than duplicating?

Agreed, goes with the improvement/passing the version trick you mentioned above.
I'll give a thought and see how I can do that.


Thank you,
Antonin

-- 
Antonin Godard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com