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

[Tom Rini]

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