On Wed, Sep 04, 2019 at 01:30:02PM -0500, Joe Hershberger wrote: > On Wed, Sep 4, 2019 at 1:01 PM Tom Rini <tr...@konsulko.com> wrote: > > > > On Tue, Sep 03, 2019 at 10:04:42AM +0200, Wolfgang Denk wrote: > > > Dear Tom, > > > > > > In message <a78f0b04-c3f7-45d5-e9ac-90522dbef...@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.delau...@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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot