[PATCH i-g-t v12 3/3] runner/executor: Integrate igt_facts functionality

[Kamil Konieczny]

Hi Peter,
On 2024-12-12 at 08:15:27 +0100, Peter Senna Tschudin wrote:
> Modifies the igt_runner to include calls to igt_facts() before the
> execution of each test and after the final test concludes. Also adds
> command line options to igt_runner:
>  -f, --facts to enable igt_facts
> 
> igt_facts is disabled by default. If not explicitly enabled with the
> command-line argument -f or --facts, igt_facts() will return
> immediately without performing any operations.
> 
> The test name is obtained by calling
> entry_display_name(&job_list->entries[state->next]).
> 
> CC: Helen Koike <helen.koike at collabora.com>
> CC: Jani Nikula <jani.nikula at linux.intel.com>
> CC: Jani Saarinen <jani.saarinen at intel.com>
> CC: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> CC: Juha-Pekka Heikkila <juha-pekka.heikkila at intel.com>
> CC: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> CC: Lucas De Marchi <lucas.demarchi at intel.com>
> CC: Maíra Canal <mcanal at igalia.com>
> CC: Melissa Wen <mwen at igalia.com>
> CC: Petri Latvala <adrinael at adrinael.net>
> CC: Rob Clark <robdclark at chromium.org>
> CC: Ryszard Knop <ryszard.knop at intel.com>
> CC: Swati Sharma <swati2.sharma at intel.com>
> CC: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> CC: dominik.karol.piatkowski at intel.com
> CC: himal.prasad.ghimiray at intel.com
> CC: igt-dev at lists.freedesktop.org <igt-dev at lists.freedesktop.org>
> CC: katarzyna.piecielska at intel.com
> CC: luciano.coelho at intel.com
> CC: nirmoy.das at intel.com
> CC: stuart.summers at intel.com
> Reviewed-by: Ryszard Knop <ryszard.knop at intel.com>
> Reviewed-by: Dominik Karol Piątkowski <dominik.karol.piatkowski at intel.com>
> Signed-off-by: Peter Senna Tschudin <peter.senna at linux.intel.com>
> ---
>  runner/executor.c | 10 ++++++++++
>  runner/settings.c |  9 ++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/runner/executor.c b/runner/executor.c
> index 49ae8c90d..d4d704a34 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -30,6 +30,7 @@
>  
>  #include "igt_aux.h"
>  #include "igt_core.h"
> +#include "igt_facts.h"
>  #include "igt_taints.h"
>  #include "igt_vec.h"
>  #include "executor.h"
> @@ -2360,12 +2361,15 @@ bool execute(struct execute_state *state,
>  	sigset_t sigmask;
>  	double time_spent = 0.0;
>  	bool status = true;
> +	char *last_test = NULL;
>  
>  	if (state->dry) {
>  		outf("Dry run, not executing. Invoke igt_resume if you want to execute.\n");
>  		return true;
>  	}
>  
> +	igt_facts_lists_init();
> +

This should be guarded by settings option, like:
	if (settings->facts)
		igt_facts_lists_init();

>  	if (state->next >= job_list->size) {
>  		outf("All tests already executed.\n");
>  		return true;
> @@ -2492,6 +2496,10 @@ bool execute(struct execute_state *state,
>  		int result;
>  		bool already_written = false;
>  
> +		/* Calls before running each test */
> +		igt_facts(last_test);

Same here.

> +		last_test = entry_display_name(&job_list->entries[state->next]);
> +
>  		if (should_die_because_signal(sigfd)) {
>  			status = false;
>  			goto end;
> @@ -2580,6 +2588,8 @@ bool execute(struct execute_state *state,
>  			return execute(state, settings, job_list);
>  		}
>  	}
> +	/* Last call to collect facts after the last test runs */
> +	igt_facts(last_test);

Same here.

>  
>  	if ((timefd = openat(resdirfd, "endtime.txt", O_CREAT | O_WRONLY | O_EXCL, 0666)) >= 0) {
>  		dprintf(timefd, "%f\n", timeofday_double());
> diff --git a/runner/settings.c b/runner/settings.c
> index 0d27e7af3..0e8e4572e 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -1,3 +1,4 @@
> +#include "igt_facts.h"
>  #include "igt_hook.h"
>  #include "igt_vec.h"
>  #include "settings.h"
> @@ -40,6 +41,7 @@ enum {
>  	OPT_INCLUDE = 't',
>  	OPT_EXCLUDE = 'x',
>  	OPT_ENVIRONMENT = 'e',
> +	OPT_FACTS = 'f',
>  	OPT_SYNC = 's',
>  	OPT_LOG_LEVEL = 'l',
>  	OPT_OVERWRITE = 'o',
> @@ -230,6 +232,7 @@ static const char *usage_str =
>  	"                                   environment variable IGT_PING_HOSTNAME does\n"
>  	"                                   not respond to ping.\n"
>  	"                         all     - abort for all of the above.\n"
> +	"  -f, --facts           Enable igt_facts tracking\n"

s/igt_//

>  	"  -s, --sync            Sync results to disk after every test\n"
>  	"  -l {quiet,verbose,dummy}, --log-level {quiet,verbose,dummy}\n"
>  	"                        Set the logger verbosity level\n"
> @@ -665,6 +668,7 @@ bool parse_options(int argc, char **argv,
>  		{"environment", required_argument, NULL, OPT_ENVIRONMENT},
>  		{"abort-on-monitored-error", optional_argument, NULL, OPT_ABORT_ON_ERROR},
>  		{"disk-usage-limit", required_argument, NULL, OPT_DISK_USAGE_LIMIT},
> +		{"facts", no_argument, NULL, OPT_FACTS},
>  		{"sync", no_argument, NULL, OPT_SYNC},
>  		{"log-level", required_argument, NULL, OPT_LOG_LEVEL},
>  		{"test-list", required_argument, NULL, OPT_TEST_LIST},
> @@ -695,7 +699,7 @@ bool parse_options(int argc, char **argv,
>  	settings->dmesg_warn_level = -1;
>  	settings->prune_mode = -1;
>  
> -	while ((c = getopt_long(argc, argv, "hn:dt:x:e:sl:omb:L",
> +	while ((c = getopt_long(argc, argv, "hn:dt:x:e:fsl:omb:L",
>  				long_options, NULL)) != -1) {
>  		switch (c) {
>  		case OPT_VERSION:
> @@ -736,6 +740,9 @@ bool parse_options(int argc, char **argv,
>  				goto error;
>  			}
>  			break;
> +		case OPT_FACTS:
> +			igt_facts_config.enabled = true;

This should be:
  			settings->facts = true;

Please create an option for it in settings struct, similar to 'sync'
below. Also save it in dry-run and restore in igt_resume.

Regards,
Kamil

> +			break;
>  		case OPT_SYNC:
>  			settings->sync = true;
>  			break;
> -- 
> 2.34.1
>