Re: [PATCH i-g-t v2 6/7] runner/settings: Serialize command line

[Petri Latvala <adrinael@adrinael.net>]

On Wed, Jan 29, 2025 at 06:23:48PM +0100, Kamil Konieczny wrote:
> Hi Lucas,
> On 2025-01-28 at 13:34:24 -0600, Lucas De Marchi wrote:
> > On Tue, Jan 28, 2025 at 07:37:44PM +0100, Kamil Konieczny wrote:
> > > Hi Lucas,
> > > On 2025-01-21 at 14:57:32 -0800, Lucas De Marchi wrote:
> > > > Serialize the command line to metadata.txt. The expected format in the
> > > > metadata.txt is like below:
> > > > 
> > > > 	cmdline.argc : 6
> > > > 	cmdline.argv[0] : ./build/runner/igt_runner
> > > > 	cmdline.argv[1] : -o
> > > 
> > > Sorry for late reponse but imho it should be saved for debugging
> > > purpose but not for reading this in metadata.txt, especially that
> > > this option '-o' is 'override existing results folder', so it
> > > should be used only once by igt_runner in setup phase but not for
> > > igt_resume.
> > > 
> > > From igt_runner --help:
> > > -o, --overwrite       If the results-path already exists, delete it
> > 
> > if you are using -o it's just because you don't care about the previous
> > results and want to completely overwrite it.
> > 
> > igt_resume doesn't have any option - it doesn't have a -o and it
> > basically does:
> > 
> > "Load the settings from the state dir back into the settings struct"...
> > igt_resume will read it back and create the settings object.
> > 
> > The results.json as architected in igt_runner discards everything and
> > is basically a conversion format from the state dir to a .json file at
> > the end of the execution.
> > 
> > > 
> > > imho we should save it into some different file and include it into
> > > results.json
> > 
> > not sure I follow... metadata.txt has the details of how we are running
> > igt_runner and it's basically a dump of the settings object. I don't
> > follow what's the relation with the -o option you mentioned above.
> > 
> > End goal is to have the value in the results.json, but I see no reason
> > why the intermediary state needs to be in a different file.
> > 
> > Lucas De Marchi
> 
> Well, what I try to comment on a commit description:
> 
> > Serialize the command line to metadata.txt. [...]
> 
> imho this should be written in setup.txt and setup.txt should be
> included in results.json There is no need for igt_runner options
> in metadata.txt, as it is used by igt_resume in shard re-runs
> after a reboot happens in a middle of testlist.

The command line in metadata.txt does no harm as it's just
informational text to be recorded in the final result. It's also
supposed to indeed be a direct dump of the settings object, and thus
should have the command line if that is to be put into the settings
object. But I suppose the pertinent question is: why does it need to
be put into the settings object? There's already things like uname.txt
and starttime.txt and whatnot, written outside of
metadata.txt. Consolidate miscellaneous info dumps to a single file?
Do that when serializing command line _somewhere_, or perhaps later?

I have no opinions to offer in any direction, just drive-by commenting
here...


-- 
Petri Latvala