Re: [XEN PATCH] automation/eclair: reorganize pipelines

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 25 Apr 2024, Federico Serafini wrote:
> On 25/04/24 02:06, Stefano Stabellini wrote:
> > On Tue, 23 Apr 2024, Federico Serafini wrote:
> > > From: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
> > > 
> > > Introduce accepted_guidelines.sh: a script to autogenerate the
> > > configuration file accepted.ecl from docs/misra/rules.rst which enables
> > > all accepted guidelines.
> > > 
> > > Introduce monitored.ecl: a manual selection of accepted guidelines
> > > which are clean or almost clean, it is intended to be used for the
> > > analyses triggered by commits.
> > > 
> > > Reorganize tagging.ecl:
> > >    -Remove "accepted" tags: keeping track of accepted guidelines tagging
> > >     them as "accepted" in the configuration file tagging.ecl is no
> > >     longer needed since docs/rules.rst is keeping track of them.
> > >    -Tag more guidelines as clean.
> > > 
> > > Reorganize eclair pipelines:
> > >    - Set1, Set2, Set3 are now obsolete: remove the corresponding
> > >      pipelines and ecl files.
> > >    - Amend scheduled eclair pipeline to use accepted.ecl.
> > >    - Amend triggered eclair pipeline to use monitored.ecl.
> > > 
> > > Rename and improve action_check_clean_regressions.sh to print a
> > > diagnostic in case a commit introduces a violation of a clean guideline.
> > > 
> > > An example of diagnostic is the following:
> > > 
> > > Failure: 13 regressions found for clean guidelines
> > >    service MC3R1.R8.2: (required) Function types shall be in prototype
> > > form with named parameters:
> > >     violation: 13
> > > 
> > > Signed-off-by: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
> > > Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> > > Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>
> > 
> > Fantastic work, thank you!
> > 
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > 
> > Is this patch safe to commit now? Or would it cause gitlab-ci breakage?
> 
> Yes, it is safe because the ECLAIR analysis is still allowed to fail.
> Committing this patch wouldn't break the CI but it will highlight some
> regressions with the orange badge and the following messages:

OK thanks, I'll commit it


> arm:
> 
> Failure: 5 regressions found for clean guidelines
>   service MC3R1.R1.1: (required) The program shall contain no violations of
> the standard C syntax and constraints, and shall not exceed the
> implementation's translation limits:
>    violation: 5
> 
> x86:
> 
> Failure: 2 regressions found for clean guidelines
>   service MC3R1.R8.2: (required) Function types shall be in prototype form
> with named parameters:
>    violation: 2
> 
> (George just sent a patch to address the regressions of Rule 8.2.)

As soon as we solve these two issues, I think we should remove the
allow_failure: true from the two ECLAIR jobs scanning clean
guidelines. More on this below.


> > 
> > One question below.
> > 
> > 
> > > -
> > >   ####################
> > >   # Clean guidelines #
> > >   ####################
> > >     -doc_begin="Clean guidelines: new violations for these guidelines are
> > > not accepted."
> > > 
> > > --service_selector={clean_guidelines_common,"MC3R1.D1.1||MC3R1.D2.1||MC3R1.D4.11||MC3R1.D4.14||MC3R1.R1.1||MC3R1.R1.3||MC3R1.R1.4||MC3R1.R2.2||MC3R1.R3.1||MC3R1.R3.2||MC3R1.R4.1||MC3R1.R4.2||MC3R1.R5.1||MC3R1.R5.2||MC3R1.R5.4||MC3R1.R5.6||MC3R1.R6.1||MC3R1.R6.2||MC3R1.R7.1||MC3R1.R8.1||MC3R1.R8.2||MC3R1.R8.5||MC3R1.R8.6||MC3R1.R8.8||MC3R1.R8.10||MC3R1.R8.12||MC3R1.R8.14||MC3R1.R9.2||MC3R1.R9.4||MC3R1.R9.5||MC3R1.R12.5||MC3R1.R17.3||MC3R1.R17.4||MC3R1.R17.6||MC3R1.R20.13||MC3R1.R20.14||MC3R1.R21.13||MC3R1.R21.19||MC3R1.R21.21||MC3R1.R22.2||MC3R1.R22.4||MC3R1.R22.5||MC3R1.R22.6"
> > > +-service_selector={clean_guidelines_common,"MC3R1.D1.1||MC3R1.D2.1||MC3R1.D4.1||MC3R1.D4.11||MC3R1.D4.14||MC3R1.R1.1||MC3R1.R11.7||MC3R1.R11.9||MC3R1.R12.5||MC3R1.R1.3||MC3R1.R1.4||MC3R1.R14.1||MC3R1.R16.7||MC3R1.R17.1||MC3R1.R17.3||MC3R1.R17.4||MC3R1.R17.5||MC3R1.R17.6||MC3R1.R20.13||MC3R1.R20.14||MC3R1.R20.4||MC3R1.R20.9||MC3R1.R21.13||MC3R1.R21.19||MC3R1.R21.21||MC3R1.R2.2||MC3R1.R22.2||MC3R1.R22.4||MC3R1.R22.5||MC3R1.R22.6||MC3R1.R2.6||MC3R1.R3.1||MC3R1.R3.2||MC3R1.R4.1||MC3R1.R4.2||MC3R1.R5.1||MC3R1.R5.2||MC3R1.R5.4||MC3R1.R5.6||MC3R1.R6.1||MC3R1.R6.2||MC3R1.R7.1||MC3R1.R7.4||MC3R1.R8.1||MC3R1.R8.10||MC3R1.R8.12||MC3R1.R8.14||MC3R1.R8.2||MC3R1.R8.5||MC3R1.R8.6||MC3R1.R8.8||MC3R1.R9.2||MC3R1.R9.3||MC3R1.R9.4||MC3R1.R9.5"
> > >   }
> > 
> > Is this list different from monitored.ecl? If so, why? If not, maybe we
> > don't need to repeat the list here as well?
> 
> Quick answer: this list is different from monitored.ecl and the two
> lists must coexist.
> 
> Here, we are "tagging" some guidelines as "clean":
> this list is crucial and will be (manually) updated every time a new
> guideline reaches 0 violations, it shall not be removed because this tag
> allows ECLAIR to print a diagnostic and fail in case unjustified
> violations are found for the tagged guidelines.
> 
> The monitored.ecl is the list of guidelines which are analyzed at each
> commit: the list shall include all the guidelines tagged as "clean"
> (to do the proper regressions checks) but the monitored list can also
> include some accepted guidelines for which it may be interesting to see
> the number of violations at each commit, for example, we put there some
> almost-clean guidelines (guidelines with few violations left but not yet
> tagged as clean yet).
> Introducing new violations of monitored but not-clean guidelines will
> not cause a failure.

I think it is time we remove allow_failure: true from
.eclair-analysis:triggered, affecting both eclair-x86_64 and
eclair-ARM64. With the goal of stopping new MISRA C violations from
entering the tree.

The question is: in which cases we want eclair-x86_64 and eclair-ARM64
to fail the Gitlab job?

It is clear that we want the two jobs to fail if new violations are
found for the clean (zero violations) guidelines.

Should we also fail the Gitlab jobs if new regreassions are found for the
almost-clean rules? I think so, yes. Do you agree?


Less important but related: if you agree, does the list above need to
still be different from the monitored.ecl list?