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

[Heiko Schocher]

Hello Joe,

Am 04.09.2019 um 01:03 schrieb Joe Hershberger:
> On Tue, Sep 3, 2019 at 3:05 AM Wolfgang Denk <wd at denx.de> 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.
>>
>> 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.
> 
> It seems this should be unacceptable even if it's in the area of
> interest. Isn't an Acked-by generally accepted as required?

Yes, but it seems we are not strict enough here.

> 
>> 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 seems the diffstat should be a quick way to see this, so I would
> think not quite their best. Maybe a reminder / recommendation that it
> be reviewed by custodians?

Yes. I recommend to use patman for sending patches, or at least to
do a dry run with it, so you get a cc list (which is sometimes to long)
of people who may interested in the patch.

> 
>> 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?
> 
> Is there any tooling around the MAINTAINERS file that can be used to
> reg-flag PRs that contain changes outside of the tree's area of
> effect?

I do not know.

>> 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)?
> 
> I like that idea, though in this specific case I think there should be
> a maintainer for env.

Wasn;t aware of the the "Designated reviewer" entry in MAINTAINERS,
I think, this is a good idea. And of course, if someone volunteers
for mainting env, this would be good.

But we should monitor (or find a script which checks this), that
patches not acked by a subsystem custodian not go in outside of a
pull request from the custodian. Problem is here, that we have
parts of code, which have no custodian ...

I can only speak for i2c, which often get patches in patchseries
for other custodians. I try to catch such patches and add a Review
or Acked-by ... but I do not catch all... so using patman would help,
as I get added to cc ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de