[i-g-t,v6,4/5] runner/settings: Add function to set IGT_DATA_PATH environment variable

[Kamil Konieczny]

On 2025-06-17 at 02:38:39 +0530, Naladala Ramanaidu wrote:
> Add environment variable to obtain absolute path for PNG files
> at runtime.
> 
> v2: Fix review comments. (Kamil)
> v3: Fix review comments. (kamil)
> v4: Fix review comments. (kamil)
> v5: Make code simpler.	 (Kamil)
> v6: Update env only if not exists.    (Kamil)
> 
> Signed-off-by: Naladala Ramanaidu <ramanaidu.naladala at intel.com>

Looks good, I am still not convinced we need it here, maybe
it will be better to have it at load data file function?

We can improve that later if there will be any need to do it,
so I will not block it. See one nit below.

> ---
>  runner/settings.c | 36 ++++++++++++++++++++++++++++++++++++
>  runner/settings.h |  1 +
>  2 files changed, 37 insertions(+)
> 
> diff --git a/runner/settings.c b/runner/settings.c
> index b151218cc..28b031a29 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -90,6 +90,8 @@ static const char settings_filename[] = "metadata.txt";
>  static const char env_filename[] = "environment.txt";
>  static const char hooks_filename[] = "hooks.txt";
>  
> +static bool igt_data_path_set_by_runner;
> +
>  static bool set_log_level(struct settings* settings, const char *level)
>  {
>  	typeof(*log_levels) *it;
> @@ -612,6 +614,31 @@ char *absolute_path(const char *path)
>  	return result;
>  }
>  
> +int set_tests_datadir(void)

Why not static? It is not used anywhere outside this settings.c
so it could just be:

static int set_tests_datadir(void)

> +{
> +	const char *datapath = "../share/igt-gpu-tools";
> +	char *abpath;
> +
> +	if (getenv("IGT_DATA_PATH"))
> +		return 0;
> +
> +	abpath = absolute_path(datapath);
> +	if (!abpath) {
> +		fprintf(stderr, "Error: Failed to get absolute path for '%s'\n",
> +			datapath);
> +		return -EINVAL;
> +	}
> +
> +	if (setenv("IGT_DATA_PATH", abpath, 0) == 0) {
> +		igt_data_path_set_by_runner = true;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +
> +
>  static char *bin_path(char *fname)
>  {
>  	char *path, *p;
> @@ -654,6 +681,10 @@ void init_settings(struct settings *settings)
>  {
>  	memset(settings, 0, sizeof(*settings));
>  	IGT_INIT_LIST_HEAD(&settings->env_vars);
> +
> +	if (set_tests_datadir())
> +		fprintf(stderr, "Data dir path not set\n");
> +
>  	igt_vec_init(&settings->hook_strs, sizeof(char *));
>  }
>  
> @@ -671,6 +702,11 @@ void clear_settings(struct settings *settings)
>  	free_hook_strs(&settings->hook_strs);
>  	free_array_deep((void **)settings->cmdline.argv, settings->cmdline.argc);
>  
> +	if (igt_data_path_set_by_runner) {
> +		unsetenv("IGT_DATA_PATH");
> +		igt_data_path_set_by_runner = false;
> +	}
> +
>  	init_settings(settings);
>  }
>  
> diff --git a/runner/settings.h b/runner/settings.h
> index 6c58c3282..e8e87d3a5 100644
> --- a/runner/settings.h
> +++ b/runner/settings.h
> @@ -160,5 +160,6 @@ bool serialize_settings(struct settings *settings);
>  
>  bool read_settings_from_file(struct settings *settings, FILE* f);
>  bool read_settings_from_dir(struct settings *settings, int dirfd);
> +int set_tests_datadir(void);

This is only used at setting.c so no need for exporting this,
it could be fixed at merge time, with this fixed:

Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>

>  
>  #endif