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

[Tom Rini <trini@konsulko.com>]

On Tue, Sep 03, 2019 at 06:03:40PM -0500, Joe Hershberger wrote:
> On Tue, Sep 3, 2019 at 3:05 AM Wolfgang Denk <wd@denx.de> wrote:
> >
> > Dear Tom,
> >
> > In message <a78f0b04-c3f7-45d5-e9ac-90522dbefc2e@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@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?

Part of the problem here is that yes, I need to rework my tooling a bit,
and am.  But another part of the problem is that this exact code area is
not widely used.  So the things that cause me concern didn't trigger.
But looking at the code by itself, I would have acked it.  It would have
then been on noticing the size change and function-removal to see
there's a not-so-obvious problem.

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

I do wish we had a dedicated custodian for environment changes, yes.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190904/e3ad85ca/attachment.sig>