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. > - 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@lists.denx.de > https://lists.denx.de/listinfo/u-boot _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot