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

[Giulio Benetti <giulio.benetti@benettiengineering.com>]

Hi Luca, Arnout, Thomas P.,

On 13/02/22 11:39, Arnout Vandecappelle wrote:
> 
> 
> 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".

Always welcome!

>>> Signed-off-by: Giulio Benetti <giulio.benetti@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".

Ok

>>> +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".

Yes

>> 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.

Ok

>>
>>> +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.

Yes :-/

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

Oh, I've missed that, just checked and it eases life not few!
I have a 120 columns command to start it that I copy and paste 
everytime. Because also, what I don't take care of here is the -v flag 
that allows you mount a host folder.

>> 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?

utils/docker-run then. Now we know it exists :-)

>>> +so:
>>
>> Add an empty line here, so that the output separates from the next line.

Ok

>>> +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.

All new thing I didn't know!

>>
>>> +--------------------
>>
>> 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.

+1

>>> +Inside the docker hint:
>>> +--------------------
>>> + $ git clone git://git.busybox.net/buildroot
>>> + $ cd buildroot
>>> + $ make +<boardname>_defconfig+
>>> + $ make
>>> +--------------------
>>
>> As above, add an empty line here.

+1

>>> +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").

"...and possibly add host dependencies", right?

>> 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.

Yes, it was because of that.

>    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;

There is a bunch of dockers like that(more or less) here:
https://github.com/aduskett/buildroot-docker-devel
I've also contributed to, and at that time Thomas P. in IRC asked why we 
didn't upstreamed it and I told I would have done like 2/3 years ago and 
I've never done it :-/

And also modifying autobuilder's script to pick random distro and build 
to avoid possible host issues. But it's a bunch of stuff to do.

> - a container for CI;

Do you mean the one we already have but more shrinked?

> - a more complete container you could use for development, though I can't
> immediately think of extra stuff you'd want in there 

Is it really worth it? I mean, I've never seen anybody in IRC(even if 
read few in it) or ML(same) that complain about "I can't have buildroot 
working because I miss host tools". But I've seen recently gitlab-ci 
results that took me like 15-16 hours to fix.

Does Yocto have something like that? And if yes, does someone can give a 
feedback if he really uses it?

>(but then, I wouldn't use a container for development).

Me too, and who would use it? I think nobody, because I don't think a 
newbie is that skilled to use a docker too(or maybe yes), but my first 
try would be using it with my distro and probably same goes for other 
people. But here again, I use Terminator+Midnight Commander as my "IDE", 
so I won't be happy enough with it. Someone else uses real "IDE" and we 
can't add Eclipse or VSCode(I hope), so it will be something that is not 
enough for anybody, thus IMHO useless.

----------------
Going back to this patch:

What I can do with this patch is to rewrite it pointing how to use 
utils/docker-run to check that at least configs/* and board/* patches work.

Another solution to my patch is what Thomas P. pointed in IRC:
"it is probably easier to ask people to use gitlab CI"

But my worry is that lot of people actually fork from github and not 
from gitlab. Who would really do that(both docker and gitlab-CI solutions)?
But also, who would really install docker(if they don't use it) to 
submit a patch for gitlab-CI build failures?

I think that counting the ones who took care about their maintained 
board gives us an idea, very few. BUT for new boards, and I see not few 
of them adding in the last period. It could be a way to force them to 
give a proof of a successfull building with gitlab-CI pipeline log as 
Thomas P. proposed.

So I would modify this patch with instructions to:
- fork Buildroot in gitlab
- trigger gitlab-CI pipeline for a single defconfig

What do you all think?

Best regards
-- 
Giulio Benetti
Benetti Engineering sas
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot