Re: [PATCH i-g-t v2 7/7] runner/resultgen: Add cmdline to results.json

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

On Thu, 2025-01-23 at 00:50 -0600, Lucas De Marchi wrote:
> On Wed, Jan 22, 2025 at 09:51:42AM -0300, Gustavo Sousa wrote:
> > Quoting Lucas De Marchi (2025-01-21 19:57:33-03:00)
> > > For easier repro scenarios, add the cmdline to the json: one can see the
> > > exact command executed to try to reproduce a CI failure without needing
> > > extra files.
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > > runner/json_tests_data/aborted-after-a-test/reference.json | 1 +
> > > runner/json_tests_data/aborted-on-boot/reference.json      | 1 +
> > > runner/json_tests_data/dmesg-escapes/reference.json        | 1 +
> > > runner/json_tests_data/dmesg-results/reference.json        | 1 +
> > > .../dmesg-warn-level-one-piglit-style/reference.json       | 1 +
> > > .../dmesg-warn-level-piglit-style/reference.json           | 1 +
> > > runner/json_tests_data/dmesg-warn-level/reference.json     | 1 +
> > > .../reference.json                                         | 1 +
> > > .../dynamic-subtests-keep-all/reference.json               | 3 ++-
> > > .../dynamic-subtests-keep-dynamic/reference.json           | 1 +
> > > .../dynamic-subtests-keep-requested/reference.json         | 3 ++-
> > > .../dynamic-subtests-keep-subtests/reference.json          | 3 ++-
> > > runner/json_tests_data/empty-result-files/reference.json   | 1 +
> > > runner/json_tests_data/graceful-notrun/reference.json      | 1 +
> > > .../incomplete-before-any-subtests/reference.json          | 1 +
> > > runner/json_tests_data/normal-run/reference.json           | 1 +
> > > .../notrun-results-multiple-mode/reference.json            | 1 +
> > > runner/json_tests_data/notrun-results/reference.json       | 1 +
> > > runner/json_tests_data/piglit-style-dmesg/reference.json   | 1 +
> > > .../json_tests_data/unprintable-characters/reference.json  | 1 +
> > > .../warnings-with-dmesg-warns/reference.json               | 1 +
> > > runner/json_tests_data/warnings/reference.json             | 1 +
> > > runner/resultgen.c                                         | 7 ++++++-
> > > 23 files changed, 31 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/runner/json_tests_data/aborted-after-a-test/reference.json b/runner/json_tests_data/aborted-after-a-test/reference.json
> > > index 0776f7582..50ba9e6e1 100644
> > > --- a/runner/json_tests_data/aborted-after-a-test/reference.json
> > > +++ b/runner/json_tests_data/aborted-after-a-test/reference.json
> > > @@ -3,6 +3,7 @@
> > >   "results_version":10,
> > >   "name":"normal-run",
> > >   "uname":"Linux hostname 4.18.0-1-amd64 #1 SMP Debian 4.18.6-1 (2018-09-06) x86_64",
> > > +  "cmdline":[],
> > >   "time_elapsed":{
> > >     "__type__":"TimeAttribute",
> > >     "start":1539953735.1110389,
> > > diff --git a/runner/json_tests_data/aborted-on-boot/reference.json b/runner/json_tests_data/aborted-on-boot/reference.json
> > > index 75f194660..238ec6fff 100644
> > > --- a/runner/json_tests_data/aborted-on-boot/reference.json
> > > +++ b/runner/json_tests_data/aborted-on-boot/reference.json
> > > @@ -3,6 +3,7 @@
> > >   "results_version":10,
> > >   "name":"normal-run",
> > >   "uname":"Linux hostname 4.18.0-1-amd64 #1 SMP Debian 4.18.6-1 (2018-09-06) x86_64",
> > > +  "cmdline":[],
> > >   "time_elapsed":{
> > >     "__type__":"TimeAttribute",
> > >     "start":1539953735.1110389,
> > > diff --git a/runner/json_tests_data/dmesg-escapes/reference.json b/runner/json_tests_data/dmesg-escapes/reference.json
> > > index 91c573106..e74dc89e4 100644
> > > --- a/runner/json_tests_data/dmesg-escapes/reference.json
> > > +++ b/runner/json_tests_data/dmesg-escapes/reference.json
> > > @@ -3,6 +3,7 @@
> > >   "results_version":10,
> > >   "name":"normal-run",
> > >   "uname":"Linux hostname 4.18.0-1-amd64 #1 SMP Debian 4.18.6-1 (2018-09-06) x86_64",
> > > +  "cmdline":[],
> > >   "time_elapsed":{
> > >     "__type__":"TimeAttribute",
> > >     "start":1539953735.1110389,
> > > diff --git a/runner/json_tests_data/dmesg-results/reference.json b/runner/json_tests_data/dmesg-results/reference.json
> > > index e9e011853..81890f7d2 100644
> > > --- a/runner/json_tests_data/dmesg-results/reference.json
> > > +++ b/runner/json_tests_data/dmesg-results/reference.json
> > > @@ -3,6 +3,7 @@
> > >   "results_version":10,
> > >   "name":"normal-run",
> > >   "uname":"Linux hostname 4.18.0-1-amd64 #1 SMP Debian 4.18.6-1 (2018-09-06) x86_64",
> > > +  "cmdline":[],
> > >   "time_elapsed":{
> > >     "__type__":"TimeAttribute",
> > >     "start":1539953735.1110389,
> > > diff --git a/runner/json_tests_data/dmesg-warn-level-one-piglit-style/reference.json b/runner/json_tests_data/dmesg-warn-level-one-piglit-style/reference.json
> > > index 8d266cdfa..16c97d3a1 100644
> > > --- a/runner/json_tests_data/dmesg-warn-level-one-piglit-style/reference.json
> > > +++ b/runner/json_tests_data/dmesg-warn-level-one-piglit-style/reference.json
> > > @@ -3,6 +3,7 @@
> > >   "results_version":10,
> > 
> > Hm... Are we going to continue with 10 as the version after including
> > another field? Are there guidelines on how results_version should be
> > handled?
> 
> good question.... last time it was updated was in 2018, to be compatible
> with piglit.
> 
> commit 2c793666d8c8328733f5769b16ae5858fee97f3f
> Author: Petri Latvala <petri.latvala@intel.com>
> Date:   Wed Dec 5 14:53:43 2018 +0200
> 
>      runner: Produce json with results_version=10
>      
>      New piglit bumped its results_version to 10, making glxinfo and pals
>      optional in practice, not just by accident. Unfortunately reading
>      results with newer piglit attempts to convert the results to version
>      10, reading glxinfo and pals, and thus fails. In a hilarious summary:
>      A commit to piglit making glxinfo optional makes it mandatory for us.
>      
>      v2: json unit tests confirmed to be working...
>      
>      Reported-by: Andi Shyti <andi.shyti@intel.com>
>      Signed-off-by: Petri Latvala <petri.latvala@intel.com>
>      Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>      Cc: Andi Shyti <andi.shyti@intel.com>
>      Tested-by: Andi Shyti <andi.shyti@intel.com>
>      Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> 
> Does this still work with piglit?  something I didn't test yet.
> Thanks for noticing

In the past, there were some tools and CI runs that used piglit, but
they are no longer around for at least 3+ years. Mesa team works with
those now, we don't have a harness or parsing utilities that work with
piglit et al. If it's still compatible, it's only by accident,
considering that deqp-runner et al is under fairly active development.

> 
> > 
> > >   "name":"normal-run",
> > >   "uname":"Linux hostname 4.18.0-1-amd64 #1 SMP Debian 4.18.6-1 (2018-09-06) x86_64",
> > > +  "cmdline":[],
> > 
> > Well, cmdline is not supposed to be empty. Shouldn't we have at least a
> > fake command here? E.g. ["igt_runner"] or even with some common
> > arguments...
> 
> if I remember well, I tried that, but then it tries to match the number
> of members in the array - it won't match since igt_runner may be
> called in different ways. Maybe there's a way to tell the test suite to
> ignore the elements of the array. I will check.
> 
> Lucas De Marchi
> 
> > 
> > --
> > Gustavo Sousa