[U-Boot] U-Boot: Environment flags broken for U-Boot

[Joe Hershberger]

On Tue, Sep 3, 2019 at 3:05 AM Wolfgang Denk <wd at denx.de> wrote:
>
> Dear Tom,
>
> In message <a78f0b04-c3f7-45d5-e9ac-90522dbefc2e at denx.de> Heiko Schocher wrote:
> >
> > I am just testing U-Boot Environment flags and they do not work anymore with
> > current mainline U-Boot ...
> ...
> > reason is your commit:
> >
> > commit 7d4776545b0f8a8827e5d061206faf61c9ba6ea9
> > Author: Patrick Delaunay <patrick.delaunay at st.com>
> > Date:   Thu Apr 18 17:32:49 2019 +0200
> >
> >      env: solve compilation error in SPL
>
>
> Looking into the history of this, I wonder if we could / should
> have prevented this.
>
> As far as I can see, Patrick's patch series has not been reviewed by
> others, probably because general intetest in STM32 is not that big
> at the moment.  I can see no Acked-by:, Reviewed-by: nor Tested-by:
> tags - nothing.
>
> The whole patch series was then pulled from the u-boot-stm
> repository.
>
>
> However, there was not only STM related code in there.  There were
> changes to common code like the environment handling.  common code
> was changed without review and without testing.

It seems this should be unacceptable even if it's in the area of
interest. Isn't an Acked-by generally accepted as required?

> Are there ways to prevent this?
>
> Yes, we can appeal to the custodians to be more careful, but I
> assume they are already doing their best.

It seems the diffstat should be a quick way to see this, so I would
think not quite their best. Maybe a reminder / recommendation that it
be reviewed by custodians?

> It might have even been better if this had been a sub-system with a
> clear maintainer, but there is no such person for the environment
> code.
>
> How can we prevent this in the future?

Is there any tooling around the MAINTAINERS file that can be used to
reg-flag PRs that contain changes outside of the tree's area of
effect?

> Should we define "interested developers" for such areas that have no
> custodian (the "Designated reviewer") entry in the MAINTAINERS file
> could be used for this, for example)?

I like that idea, though in this specific case I think there should be
a maintainer for env.

> Better ideas?
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> To make this work we'd need a patch, as nobody of us tests this.
> - L. Poettering in https://bugs.freedesktop.org/show_bug.cgi?id=74589
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot