Hello Joe,
Am 04.09.2019 um 01:03 schrieb Joe Hershberger:
On Tue, Sep 3, 2019 at 3:05 AM Wolfgang Denk <w...@denx.de> 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.
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: h...@denx.de
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
