Proposal to make EccCheck non-destructive

Proposal to make EccCheck non-destructive

[Bret Barkelew]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

In regards to this bug:
2986 – EccCheck should not revert staged and local changes (tianocore.org)<https://bugzilla.tianocore.org/show_bug.cgi?id=2986>

I have a proposal:

  1.  git diff --name-only <starting_commit>..<ending_commit/HEAD>
  2.  Copy list of changed files to a temp directory under Build/
     *   Perhaps something like Build/EccCheck or Build/EccCheckTemp
  3.  Remove files/directories that are in the exclusion list
  4.  Run EccCheck against that directory
  5.  Filter the report to remove the temp directory path so that the reported errors are root-based paths

The trade off of this approach is that EccCheck will run against multiple patches in aggregate, but – frankly – this isn’t any different than the other CI plugins.

The destructive nature of this plugin has caused headaches for everyone on our team that has worked with the new codebase. We all use local CI to validate changes as we make them, and having to make “test commits” just to ensure our work doesn’t go away isn’t a satisfactory process.

Thoughts? If this passes muster, I’m happy to code up some quick patches (if for no other reason than we can stage them in Mu while we’re waiting for full adoption).

- Bret


[-- Attachment #2: Type: text/html, Size: 5210 bytes --]

Re: [edk2-devel] Proposal to make EccCheck non-destructive

[Laszlo Ersek]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 1588 bytes --]

On 10/05/20 01:25, Bret Barkelew via groups.io wrote:
> In regards to this bug:
> 2986 – EccCheck should not revert staged and local changes (tianocore.org)<https://bugzilla.tianocore.org/show_bug.cgi?id=2986>
> 
> I have a proposal:
> 
>   1.  git diff --name-only <starting_commit>..<ending_commit/HEAD>
>   2.  Copy list of changed files to a temp directory under Build/
>      *   Perhaps something like Build/EccCheck or Build/EccCheckTemp
>   3.  Remove files/directories that are in the exclusion list
>   4.  Run EccCheck against that directory
>   5.  Filter the report to remove the temp directory path so that the reported errors are root-based paths
> 
> The trade off of this approach is that EccCheck will run against multiple patches in aggregate, but – frankly – this isn’t any different than the other CI plugins.
> 
> The destructive nature of this plugin has caused headaches for everyone on our team that has worked with the new codebase. We all use local CI to validate changes as we make them, and having to make “test commits” just to ensure our work doesn’t go away isn’t a satisfactory process.
> 
> Thoughts? If this passes muster, I’m happy to code up some quick patches (if for no other reason than we can stage them in Mu while we’re waiting for full adoption).

The idea looks fine to me, but can you please clarify how it would be a
"trade-off"? That is -- does EccCheck (and if so, how) currently check
every patch in a series? I didn't know that.

(I don't think it's a property we "must" preserve in CI, just asking.)

Thanks
Laszlo