Re: [Buildroot] [master, next, lts] utils/get-developer: fix DEVELOPERS syntax check on GitLab CI

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Ricardo,

On Fri, 27 May 2022 22:48:32 -0300
Ricardo Martincoski <ricardo.martincoski@gmail.com> wrote:

> Commit "45aabcddc5 utils/get-developers: really make it callable from
> elsewhere than the toplevel directory" ended up making check-DEVELOPERS
> in GitLab-CI to always get a false PASS result, since the job was
> trusting that calling get-developers with no arguments would parse
> DEVELOPERS file and generate error or warning messages.
> 
> There is need to revert that change, we only need to recover the syntax
> check coverage it had before the change.
> 
> Make the option -c from get-developers to generate a non-zero return
> code when there is parsing errors or warnings.
> Adapt the manual accordingly and use the -c argument in the
> check-DEVELOPERS job.
> 
> In the same commit, add a runtime test in order to detect undesired
> changes in behavior of the get-developers script.
> The test uses a .patch file generated against the buildroot tree as a
> fixture to check how get-developers operates when called to check it.
> The test also overrides the DEVELOPERS file in order to be fully
> reproducible and a -d option is added to get-developers in order to
> allow this.
> Since get-developers only looks to already committed files to compare
> against patch files, the fixture uses a package that is very unlikely to
> be removed from buildroot tree: binutils.
> 
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Peter Korsgaard <peter@korsgaard.com>
> Cc: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Thanks a lot for the patch! I ended up applying it, but with some
fairly significant differences.

The most important thing is that I didn't like the idea of re-using the
existing "-c" option as the way of validating the DEVELOPERS file. -c
is used to list files that don't have any associated developer.

So instead, I've added a -v (v for validate) option in get-developers
that simply does the validation step. I didn't introduce the extra
complexity of making parse_developers() return the number of errors and
warnings, since this was anyway not used anywhere. So I simply rely on
parse_developers() returning None when there is a validation failure.

See:

  https://git.buildroot.org/buildroot/commit/?id=47f359a615ae4772ea9d03a5134785aec230c317

Then I used this new -v option in gitlab-ci.yml:

  https://git.buildroot.org/buildroot/commit/?id=4ed7bca6a05a7878fdb73a973d098a6b64586848

Then I re-used your patch adding a -d option to get-developers, to pass
a custom DEVELOPERS file:

  https://git.buildroot.org/buildroot/commit/?id=7082b0585d5a231a062087d3e44007742b9165d4

Finally, I added your test cases, after changing them according to the
modifications I had done. Another change I did is put the binutils
patch not in the br2-external (it's not related to it), but into a
test_get_developers/ fixture sub-directory, like we do for package
tests that have test fixtures.

  https://git.buildroot.org/buildroot/commit/?id=9bb647297a6c48a7a79c20f4403be0dbea811a6c

Thanks a lot for this contribution!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot