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

[Joe Hershberger]

On Wed, Sep 4, 2019 at 1:01 PM Tom Rini <trini at 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 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.
>
> 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.

> - test.py tests for the environment flags, but only if they're also run
>   on some platform(s) that also would have failed here.  Perhaps we need
>   to enable more functionality in something like qemu-x86 that is less
>   of a special case build than sandbox is?  In fact, since I know we
>   have the QEMU targets in for "real" uses and not just testing,
>   and while I worry about adding in more complex logic, we might want to
>   rework the "build and run test.py in QEMU" parts of CI to first make
>   use of scripts/kconfig/merge_config.sh to turn ON a whole bunch of
>   testing related options.
> --
> Tom
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot