[edk2-devel] [PATCH 1/2] OvmfPkg: start using the ECC plugin exception list

[Laszlo Ersek]

On 12/04/20 19:28, Sean Brogan wrote:
> I would also agree with Ard about shortening and simplifying the commit
> message if this commit is to go forward.
> 
> As a FYI the pytools issue you link to for ci comment is closed as "wont
> fix".  That doesn't change the fact that Edk2 CI runs an edk2 plugin
> that does potentially bad things to your local workspace and if your
> environment is configured in unexpected ways the plugin causes even more
> damage.
> 
> More importantly instead of this commit i ask if the community should
> have a quick value prop discussion of EccCheck and if in its current
> form needs changes or to be disabled...then that would be the change
> rather than this commit.

- Disabling ECC globally: yes please. Much better idea than this patch.

- "quick" value prop discussion: my fear was (and is) that it would not
be "quick"; I foresee either a heated debate or a drawn-out indecisive
process. Meanwhile good patches would still be held back in either case,
which is the only thing I really care about here. So if "quick" is
indeed quick, then I'm OK; I just figured the really quick way would be
this patch.


> I am generally a fan of automation and tools
> based validation for code formatting but there has been a lot of noise
> with this one so it might not yet be ready to be a PR blocker.

Agreed.

> 
> Personally, related to code formatting/conventions i would much much
> rather see the community agree to a profile in clang-format or something
> similar and then just run the tool on all files in the tree and commit
> the changes.  This might mean we have to change a few things as i
> haven't been able to get clang-format to match exactly...but in the end
> auto formatting is in my opinion a better path forward than home grown
> tools to "enforce" formatting.  Auto formatting could be easily enforced
> in CI and is easy/nearly free for a contributor to resolve and help the
> community create consistent code.  I know its not perfect but it gets
> you 95% of the way without huge investment.

I have no experience with auto-formatting. I'm not a fan of sweeping
changes (huge conversion commits). It's unclear how the above would
differ from ECC "policy wise" -- it would be a different set of
formatting opinions, yes, but it would still be a tool to enforce them,
with unclear options for exceptions.

I'd suggest splitting the two: first let's disable ECC (or make it
opt-in -- and not by metafile commits, but by github pull request
labels!), and second investigate auto-formatting (also as an opt-in, so
we could get a taste).

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68411): https://edk2.groups.io/g/devel/message/68411
Mute This Topic: https://groups.io/mt/78702238/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-