Re: [PATCH i-g-t] igt-runner fact checking

["Knop, Ryszard" <ryszard.knop@intel.com>]

On Fri, 2024-11-08 at 07:51 +0100, Peter Senna Tschudin wrote:
> Dear Ryszard,
> 
> Thank you for taking the time to write. Please see my comments bellow.
> 
> On 08.11.2024 02:15, Knop, Ryszard wrote:
> 
> [...]
> 
> > 
> > Extra hooks more or less hardcoded in the command line for a given run
> > are easy, just let us know when to add them. Reading extras from the
> > cover letter is doable, but would require time (think: early next year,
> > not a week or two), as we are switching the Patchwork integration to a
> > new implementation at the moment.
> > 
> > Although, two comments about the "facts" infra:
> > 
> > - I like the simple text format, but if possible, make the output
> > easier to parse with a consistent format for all state changes, like:
> > [FACT <test>] <new|changed|deleted>: <prop>: <value>[ -> <new_value>]
> 
> Thank you for that, it will be on V4. What string do you want for the first
> facts that print before the first test? Currently is "before any test".

Any well-known constant works, "before any test" sounds good.

> Just to make sure I understood what you want, the first '[]' around FACT are
> intended to be printed while the second ones around  -> <new_value> are not to
> be printed and are there to indicate an optional part of the message. Correct?

Yes, that's correct, [FACT ...] is always there, [ -> <new_value>] is
optional, sorry for the confusing description.

> > - While running these facts within hooks could work, it would require
> > extending the hooks with something that can understand that a given
> > command is supposed to output a bunch of facts in a well-known format
> > (ideally JSON, alternatively a "key: value" text block), then diff them
> > against the state from the previou run of the same hook.
> 
> Please tell me what you want. I like to have text output on the terminal with
> igt-runner output, but it is just a convenience. Please tell me how you want the
> output to be printed and/or saved to disk.

I don't particularly mind what happens here, just that if you are going
to use external hooks as fact providers, those external hooks cannot
just output arbitrary text if you want lightweight, easy-to-read diffs.
The simplest way is just to print key: value lines on each hook
invocation, like this:

hardware.pci.drm_card_at_addr.0000:03:00.0: card1
hardware.pci.drm_card_at_addr.0000:00:02.0: card2
kernel.kmod_is_loaded.amdgpu: true
...

The point is to make creating new fact providers easy, without having
to parse their arbitrary output in IGT. If you want something like
Lucas' `--hook 'facts:lspci -vv -d 8086:*:03xx'`, parsing and
converting that to key: value facts needs to happen in the external
script, not in IGTs. (Of course, if using built-in hooks, that happens
in IGTs, no way to avoid that.)

> [...]
> 
> Thank you,
> 
> Peter

Thanks, Ryszard