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

[Tom Rini <trini@konsulko.com>]

On Wed, Sep 04, 2019 at 01:30:02PM -0500, Joe Hershberger wrote:
> On Wed, Sep 4, 2019 at 1:01 PM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Sep 03, 2019 at 10:04:42AM +0200, Wolfgang Denk 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.
> >
> > Looking over my scripts, yes, I overlooked the problem.  The 'edison'
> > platform shows the same issue that Heiko's platform does but I
> > overlooked the size change.  I'm modifying my script currently so it
> > will show more details and this should jump out more rather than the
> > size noise of "changes in a general area".  Now interesting enough,
> > sandbox didn't blow up here but does also enable the env flags options.
> >
> > > 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.
> >
> > To be clear, it was tested, but sadly the environment flags code is not
> > heavily used / enabled.  More in a moment.
> >
> > > 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 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?
> > >
> > > 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)?
> >
> > This, along with some other environment related patches were things I
> > was keeping an eye on to see if perhaps Joe would have had time to look
> > at before it went in (as the env flag stuff came from him).  I also try
> 
> I wasn't aware of it as I wasn't Cc'ed on this series. I generally
> don't have time to troll the list in general, which is a bit of a
> problem since I also missed the discussions on the UEFI env changes,
> some of which are already in, and are not how I would have implemented
> it. I only found out that it was in work from Grant Likely at his talk
> in San Diego.
> 
> > and make use of the "Needs Review / ACK" flag in patchwork for things
> > that stand out.  Looking over the merge contents again, that particular
> > one would not have.
> >
> > So, things that would help in the future:
> > - An explicit environment maintainer
> 
> I would gladly volunteer for this role if Wolfgang would co-maintain
> to keep me in line. He seems to have an uncanny ability to keep all
> the cases in his head.

Wolfgang, what do you say?  It's certainly an area we could use a
custodian in.

-- 
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/20190909/e697a64c/attachment.sig>