[Buildroot] [PATCH] manual: board support: add instructions to test defconfig in the official docker

[Arnout Vandecappelle]

On 12/02/2022 23:56, Luca Ceresoli wrote:
> Hi Giulio,
> 
> On 04/02/22 00:54, Giulio Benetti wrote:
>> Often new boards have not been tested with official docker so let's add
>> instructions to do it.
> 
> Thank you, I think this is a very useful addition to the documentation!
> However I would suggest some changes for it to look more "professional".
> 
>> Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
>> ---
>>   docs/manual/adding-board-support.txt | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/docs/manual/adding-board-support.txt b/docs/manual/adding-board-support.txt
>> index 33ed709535..f5fb3af371 100644
>> --- a/docs/manual/adding-board-support.txt
>> +++ b/docs/manual/adding-board-support.txt
>> @@ -46,3 +46,25 @@ create a directory +board/<manufacturer>+ and a subdirectory
>>   +board/<manufacturer>/<boardname>+. You can then store your patches
>>   and configurations in these directories, and reference them from the main
>>   Buildroot configuration. Refer to xref:customize[] for more details.
>> +
>> +Before submitting patches for new boards it would be better to test it
> 
> "it would be better" -> "it is recommended".
> 
>> +by building it using .gitlab-ci.yml specified docker. For example at the
> 
> I think this should be reworded in a simpler way: "by building it using
> the docker specified in .gitlab-ci.yml".
> 
> BTW as I am a docker newbie: is it common to say "the docker"? Or would
> "the docker container" be more correct? -- By comparison, I would never
> say "using the virtualbox" but rather "using the virtualbox machine".

  I would say "the container" since you can use it with any container manager 
that follows the OCI spec.

> 
>> +time of this writing the docker is:
> 
> Remove the ':' from this line, or you'll have multiple ':' per line,
> which looks awkward.
> 
>> +$CI_REGISTRY/buildroot.org/buildroot/base:20220105.2314
> 
> Hm, this string is already old.

  Actually, this part of the documentation is already superseded since we now 
have utils/docker-run that does everything.

> There's no sane way to keep docs and
> .yml in sync. I wonder whether we should have in the manual a command
> line that always use the current string, such as:
> 
> DOCKER_IMAGE=$(cat .gitlab-ci.yml | \
>                 sed -n '/^image/s/^.*CI_REGISTRY/registry.gitlab.com/p')
> docker pull $DOCKER_IMAGE
> sudo docker run -it  $DOCKER_IMAGE
> 
> However I must admit this is not very readable in the docs... :( What
> about adding a simple script (utils/run-docker?) that does the trick and
> just mention that in the docs?
> 
>> +so:
> 
> Add an empty line here, so that the output separates from the next line.
> 
>> +Pull the docker:
>> +--------------------
>> + $ docker pull registry.gitlab.com/buildroot.org/buildroot/base:20220105.2314
> 
> Missing 'sudo'?

  Docker access is usually managed through the "docker" group rather than sudo.

  And if you use podman as docker replacement, it can even be done in an 
unprivileged container. Not that I tried it, but I think so.

  Oh BTW, the pull is in fact not needed, both podman and docker pull 
automatically when you start a container. That's the reason the container name 
is so convoluted.

> 
>> +--------------------
> 
> Add an empty line here. This has no effect on the output but makes
> source code more readable.
> 
>> +Run the docker:
>> +--------------------
>> + $ sudo docker run -it registry.gitlab.com/buildroot.org/buildroot/base:20220105.2314 /bin/bash
>> +--------------------
> 
> As above, add an empty line here.
> 
>> +Inside the docker hint:
>> +--------------------
>> + $ git clone git://git.busybox.net/buildroot
>> + $ cd buildroot
>> + $ make +<boardname>_defconfig+
>> + $ make
>> +--------------------
> 
> As above, add an empty line here.
> 
>> +Wait until build finishes and eventually add host dependencies.
> 
> If I understand what you mean here, it should be "and add host
> dependencies if needed" ("eventually" is not the english translation of
> italian "eventualmente"). If my understanding is correct, I don't find
> this sentence very useful: a docker newbie perhaps doesn't know how to
> add a host dependency (and maybe not even how to understand that they
> are missing one).
> 
> I would just remove this line, but if you think it is very important I'd
> clarify it, maybe with some examples.

  Yes, I think this is what triggered the addition of this documentation. If you 
have e.g. libopenssl-dev installed on your build host, then you usually won't 
notice in your test builds that a dependency on host-openssl is needed. So test 
builds should be done in a minimal container.

  Unfortunately, the buildroot/base container is not exactly minimal. It's 
really what is meant to be used for running CI tests, not exactly what is needed 
for build tests. Ideally, we'd have

- an absolutely minimal container that can be used for build tests - ideally in 
a couple of variants for different distros;
- a container for CI;
- a more complete container you could use for development, though I can't 
immediately think of extra stuff you'd want in there (but then, I wouldn't use a 
container for development).


  Regards,
  Arnout